Message-ID: <44cbdf50807b0232d0a01f0fababc7cbeeb33eb0.camel@j-davis.com>
Subject: Re: pgsql: CREATE SUBSCRIPTION ... SERVER.
From: Jeff Davis <pgsql@j-davis.com>
To: Andres Freund <andres@anarazel.de>
Cc: Peter Eisentraut <peter@eisentraut.org>, Jeff Davis
 <jdavis@postgresql.org>,  pgsql-committers@lists.postgresql.org
Date: Fri, 13 Mar 2026 13:37:29 -0700
In-Reply-To: <yag3eztufaacuofuwgq3wa5em5ey55kutbd7mdecrtrak3echz@3nnhmpz5ppga>
References: <E1vyYHQ-002hOV-0R@gemulon.postgresql.org>
	 <1bb21c7d-885f-4f07-a3ed-21b60d7c92c6@eisentraut.org>
	 <xvdjrdqnpap3uq7owbaox3r7p5gf7sv62aaqf2ju3vb6yglatr@kvvwhoudrlxq>
	 <03d52b80fd264bb683fb2c46ad7a2934cb656f1a.camel@j-davis.com>
	 <yag3eztufaacuofuwgq3wa5em5ey55kutbd7mdecrtrak3echz@3nnhmpz5ppga>
Content-Type: multipart/mixed; boundary="=-+XXbg69ZNhwY6LqIor5S"
User-Agent: Evolution 3.52.3-0ubuntu1.1 
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/44cbdf50807b0232d0a01f0fababc7cbeeb33eb0.camel%40j-davis.com>
Precedence: bulk

--=-+XXbg69ZNhwY6LqIor5S
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, 2026-03-13 at 09:54 -0400, Andres Freund wrote:
> GetSubscription() does a dozen allocations or so, so I'm not sure it
> matters
> that ForeignServerConnectionString() would do another few. There is
> FreeSubscription(), but it only seems to free a subset of the
> allocations, and
> none of the temporary allocations that are surely made below
> GetSubscription().

That by itself seems like a problem. If FreeSubscription() is needed,
then it should do its job; and if it's not needed, then it should be
eliminated.

To me it looks like it's needed. Otherwise there will be leaks for
every invalidation.

> But regardless of that, even if you want to make
> ForeignServerConnectionString() not leak, I don't think that means
> you need to
> use PG_TRY/FINALLY. If there's an error, afaict the callers will just
> throw
> away the apply context, no? Otherwise there'd likely be way more
> problems.

I believe that's correct. While there's no delete or reset, it looks
like an error will cause the worker to exit/restart.

>  So
> just create the context without a PG_TRY and delete it in the success
> case.
> In the failure case it'll be cleaned up by error handling.

Thank you, patch attached.

> And if you do need the PG_TRY for some reason, why not do the
> text_to_cstring() call inside the PG_TRY, since it never can have a
> value if
> an error was thrown?

That's what I did in the patch I posted here:

https://www.postgresql.org/message-id/f48610c90e69de4b30841361c568c3765e8f3=
dfe.camel%40j-davis.com

but I think you are right that we don't need the try/catch at all.

Regards,
	Jeff Davis


--=-+XXbg69ZNhwY6LqIor5S
Content-Disposition: attachment;
	filename*0=v22-0001-Eliminate-PG_TRY-PG_CATCH-in-ForeignServerConnec.pat;
	filename*1=ch
Content-Type: text/x-patch;
	name="v22-0001-Eliminate-PG_TRY-PG_CATCH-in-ForeignServerConnec.patch";
	charset="UTF-8"
Content-Transfer-Encoding: base64

RnJvbSBiZjA5OTMyZGJjMGMzYWI3M2JmZDU1NjZmNWY2OWEzMmQ3MDkyNzhhIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBKZWZmIERhdmlzIDxqZWZmQGotZGF2aXMuY29tPgpEYXRlOiBG
cmksIDEzIE1hciAyMDI2IDEzOjI0OjE4IC0wNzAwClN1YmplY3Q6IFtQQVRDSCB2MjJdIEVsaW1p
bmF0ZSBQR19UUlkoKS9QR19DQVRDSCgpIGluCiBGb3JlaWduU2VydmVyQ29ubmVjdGlvblN0cmlu
ZygpLgoKVGhlIGVycm9yIHBhdGggaXMgY2xlYW5lZCB1cCBmb3IgYWxsIGNhbGxlcnMsIHNvIG9u
bHkgd29ycnkgYWJvdXQgdGhlCnN1Y2Nlc3MgcGF0aC4gSW4gdGhlIGNhc2Ugb2YgYSBsb2dpY2Fs
IHdvcmtlciwgaXQgd2lsbCBqdXN0IGV4aXQuCgpTdWdnZXN0ZWQtYnk6IEFuZHJlcyBGcmV1bmQg
PGFuZHJlc0BhbmFyYXplbC5kZT4KRGlzY3Vzc2lvbjogaHR0cHM6Ly9wb3N0Z3IuZXMvbS94dmRq
cmRxbnBhcDN1cTdvd2Jhb3gzcjdwNWdmN3N2NjJhYXFmMmp1M3ZiNnlnbGF0ciU0MGt2dndob3Vk
cmx4cQotLS0KIHNyYy9iYWNrZW5kL2ZvcmVpZ24vZm9yZWlnbi5jIHwgNTMgKysrKysrKysrKysr
KystLS0tLS0tLS0tLS0tLS0tLS0tLS0KIDEgZmlsZSBjaGFuZ2VkLCAyMSBpbnNlcnRpb25zKCsp
LCAzMiBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9zcmMvYmFja2VuZC9mb3JlaWduL2ZvcmVp
Z24uYyBiL3NyYy9iYWNrZW5kL2ZvcmVpZ24vZm9yZWlnbi5jCmluZGV4IGNmMTBhOGMwMGY5Li5j
ZDJkODVkZjgxMSAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvZm9yZWlnbi9mb3JlaWduLmMKKysr
IGIvc3JjL2JhY2tlbmQvZm9yZWlnbi9mb3JlaWduLmMKQEAgLTIyNSw3ICsyMjUsMTEgQEAgRm9y
ZWlnblNlcnZlckNvbm5lY3Rpb25TdHJpbmcoT2lkIHVzZXJpZCwgT2lkIHNlcnZlcmlkKQogewog
CU1lbW9yeUNvbnRleHQgdGVtcENvbnRleHQ7CiAJTWVtb3J5Q29udGV4dCBvbGRjeHQ7Ci0JY2hh
cgkgICAqcmVzdWx0ID0gTlVMTDsKKwlGb3JlaWduU2VydmVyICpzZXJ2ZXI7CisJRm9yZWlnbkRh
dGFXcmFwcGVyICpmZHc7CisJdGV4dAkgICAqY29ubmVjdGlvbl90ZXh0OworCURhdHVtCQljb25u
ZWN0aW9uX2RhdHVtOworCWNoYXIJICAgKnJlc3VsdDsKIAogCS8qCiAJICogR2V0Rm9yZWlnblNl
cnZlciwgR2V0Rm9yZWlnbkRhdGFXcmFwcGVyLCBhbmQgdGhlIGNvbm5lY3Rpb24gZnVuY3Rpb24K
QEAgLTIzOCw0MiArMjQyLDI3IEBAIEZvcmVpZ25TZXJ2ZXJDb25uZWN0aW9uU3RyaW5nKE9pZCB1
c2VyaWQsIE9pZCBzZXJ2ZXJpZCkKIAogCW9sZGN4dCA9IE1lbW9yeUNvbnRleHRTd2l0Y2hUbyh0
ZW1wQ29udGV4dCk7CiAKLQlQR19UUlkoKTsKLQl7Ci0JCUZvcmVpZ25TZXJ2ZXIgKnNlcnZlcjsK
LQkJRm9yZWlnbkRhdGFXcmFwcGVyICpmZHc7Ci0JCXRleHQJICAgKmNvbm5lY3Rpb25fdGV4dCA9
IE5VTEw7Ci0JCURhdHVtCQljb25uZWN0aW9uX2RhdHVtOwotCi0JCXNlcnZlciA9IEdldEZvcmVp
Z25TZXJ2ZXIoc2VydmVyaWQpOwotCQlmZHcgPSBHZXRGb3JlaWduRGF0YVdyYXBwZXIoc2VydmVy
LT5mZHdpZCk7Ci0KLQkJaWYgKCFPaWRJc1ZhbGlkKGZkdy0+ZmR3Y29ubmVjdGlvbikpCi0JCQll
cmVwb3J0KEVSUk9SLAotCQkJCQkoZXJyY29kZShFUlJDT0RFX0ZFQVRVUkVfTk9UX1NVUFBPUlRF
RCksCi0JCQkJCSBlcnJtc2coImZvcmVpZ24gZGF0YSB3cmFwcGVyIFwiJXNcIiBkb2VzIG5vdCBz
dXBwb3J0IHN1YnNjcmlwdGlvbiBjb25uZWN0aW9ucyIsCi0JCQkJCQkJZmR3LT5mZHduYW1lKSwK
LQkJCQkJIGVycmRldGFpbCgiRm9yZWlnbiBkYXRhIHdyYXBwZXIgbXVzdCBiZSBkZWZpbmVkIHdp
dGggQ09OTkVDVElPTiBzcGVjaWZpZWQuIikpKTsKKwlzZXJ2ZXIgPSBHZXRGb3JlaWduU2VydmVy
KHNlcnZlcmlkKTsKKwlmZHcgPSBHZXRGb3JlaWduRGF0YVdyYXBwZXIoc2VydmVyLT5mZHdpZCk7
CiAKKwlpZiAoIU9pZElzVmFsaWQoZmR3LT5mZHdjb25uZWN0aW9uKSkKKwkJZXJlcG9ydChFUlJP
UiwKKwkJCQkoZXJyY29kZShFUlJDT0RFX0ZFQVRVUkVfTk9UX1NVUFBPUlRFRCksCisJCQkJIGVy
cm1zZygiZm9yZWlnbiBkYXRhIHdyYXBwZXIgXCIlc1wiIGRvZXMgbm90IHN1cHBvcnQgc3Vic2Ny
aXB0aW9uIGNvbm5lY3Rpb25zIiwKKwkJCQkJCWZkdy0+ZmR3bmFtZSksCisJCQkJIGVycmRldGFp
bCgiRm9yZWlnbiBkYXRhIHdyYXBwZXIgbXVzdCBiZSBkZWZpbmVkIHdpdGggQ09OTkVDVElPTiBz
cGVjaWZpZWQuIikpKTsKIAotCQljb25uZWN0aW9uX2RhdHVtID0gT2lkRnVuY3Rpb25DYWxsMyhm
ZHctPmZkd2Nvbm5lY3Rpb24sCi0JCQkJCQkJCQkJCU9iamVjdElkR2V0RGF0dW0odXNlcmlkKSwK
LQkJCQkJCQkJCQkJT2JqZWN0SWRHZXREYXR1bShzZXJ2ZXJpZCksCi0JCQkJCQkJCQkJCVBvaW50
ZXJHZXREYXR1bShOVUxMKSk7CisJY29ubmVjdGlvbl9kYXR1bSA9IE9pZEZ1bmN0aW9uQ2FsbDMo
ZmR3LT5mZHdjb25uZWN0aW9uLAorCQkJCQkJCQkJCU9iamVjdElkR2V0RGF0dW0odXNlcmlkKSwK
KwkJCQkJCQkJCQlPYmplY3RJZEdldERhdHVtKHNlcnZlcmlkKSwKKwkJCQkJCQkJCQlQb2ludGVy
R2V0RGF0dW0oTlVMTCkpOwogCi0JCWNvbm5lY3Rpb25fdGV4dCA9IERhdHVtR2V0VGV4dFBQKGNv
bm5lY3Rpb25fZGF0dW0pOworCWNvbm5lY3Rpb25fdGV4dCA9IERhdHVtR2V0VGV4dFBQKGNvbm5l
Y3Rpb25fZGF0dW0pOwogCi0JCU1lbW9yeUNvbnRleHRTd2l0Y2hUbyhvbGRjeHQpOwotCQlyZXN1
bHQgPSB0ZXh0X3RvX2NzdHJpbmcoKHRleHQgKikgY29ubmVjdGlvbl90ZXh0KTsKLQl9Ci0JUEdf
RklOQUxMWSgpOwotCXsKLQkJLyogbm8tb3Agb24gc3VjY2VzcyBwYXRoICovCi0JCU1lbW9yeUNv
bnRleHRTd2l0Y2hUbyhvbGRjeHQpOworCU1lbW9yeUNvbnRleHRTd2l0Y2hUbyhvbGRjeHQpOwor
CXJlc3VsdCA9IHRleHRfdG9fY3N0cmluZyhjb25uZWN0aW9uX3RleHQpOwogCi0JCU1lbW9yeUNv
bnRleHREZWxldGUodGVtcENvbnRleHQpOwotCX0KLQlQR19FTkRfVFJZKCk7CisJTWVtb3J5Q29u
dGV4dERlbGV0ZSh0ZW1wQ29udGV4dCk7CiAKIAlyZXR1cm4gcmVzdWx0OwogfQotLSAKMi40My4w
Cgo=


--=-+XXbg69ZNhwY6LqIor5S--