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/f48610c90e69de4b30841361c568c3765e8f3dfe.camel%40j-davis.com but I think you are right that we don't need the try/catch at all. Regards, Jeff Davis
From bf09932dbc0c3ab73bfd5566f5f69a32d709278a Mon Sep 17 00:00:00 2001 From: Jeff Davis <[email protected]> Date: Fri, 13 Mar 2026 13:24:18 -0700 Subject: [PATCH v22] Eliminate PG_TRY()/PG_CATCH() in ForeignServerConnectionString(). The error path is cleaned up for all callers, so only worry about the success path. In the case of a logical worker, it will just exit. Suggested-by: Andres Freund <[email protected]> Discussion: https://postgr.es/m/xvdjrdqnpap3uq7owbaox3r7p5gf7sv62aaqf2ju3vb6yglatr%40kvvwhoudrlxq --- src/backend/foreign/foreign.c | 53 ++++++++++++++--------------------- 1 file changed, 21 insertions(+), 32 deletions(-) diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c index cf10a8c00f9..cd2d85df811 100644 --- a/src/backend/foreign/foreign.c +++ b/src/backend/foreign/foreign.c @@ -225,7 +225,11 @@ ForeignServerConnectionString(Oid userid, Oid serverid) { MemoryContext tempContext; MemoryContext oldcxt; - char *result = NULL; + ForeignServer *server; + ForeignDataWrapper *fdw; + text *connection_text; + Datum connection_datum; + char *result; /* * GetForeignServer, GetForeignDataWrapper, and the connection function @@ -238,42 +242,27 @@ ForeignServerConnectionString(Oid userid, Oid serverid) oldcxt = MemoryContextSwitchTo(tempContext); - PG_TRY(); - { - ForeignServer *server; - ForeignDataWrapper *fdw; - text *connection_text = NULL; - Datum connection_datum; - - server = GetForeignServer(serverid); - fdw = GetForeignDataWrapper(server->fdwid); - - if (!OidIsValid(fdw->fdwconnection)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("foreign data wrapper \"%s\" does not support subscription connections", - fdw->fdwname), - errdetail("Foreign data wrapper must be defined with CONNECTION specified."))); + server = GetForeignServer(serverid); + fdw = GetForeignDataWrapper(server->fdwid); + if (!OidIsValid(fdw->fdwconnection)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("foreign data wrapper \"%s\" does not support subscription connections", + fdw->fdwname), + errdetail("Foreign data wrapper must be defined with CONNECTION specified."))); - connection_datum = OidFunctionCall3(fdw->fdwconnection, - ObjectIdGetDatum(userid), - ObjectIdGetDatum(serverid), - PointerGetDatum(NULL)); + connection_datum = OidFunctionCall3(fdw->fdwconnection, + ObjectIdGetDatum(userid), + ObjectIdGetDatum(serverid), + PointerGetDatum(NULL)); - connection_text = DatumGetTextPP(connection_datum); + connection_text = DatumGetTextPP(connection_datum); - MemoryContextSwitchTo(oldcxt); - result = text_to_cstring((text *) connection_text); - } - PG_FINALLY(); - { - /* no-op on success path */ - MemoryContextSwitchTo(oldcxt); + MemoryContextSwitchTo(oldcxt); + result = text_to_cstring(connection_text); - MemoryContextDelete(tempContext); - } - PG_END_TRY(); + MemoryContextDelete(tempContext); return result; } -- 2.43.0
