From: Jeff Davis <pgsql@j-davis.com>
To: Andres Freund <andres@anarazel.de>
Cc: Peter Eisentraut <peter@eisentraut.org>
Cc: Jeff Davis <jdavis@postgresql.org>
Cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: CREATE SUBSCRIPTION ... SERVER.
Date: Fri, 13 Mar 2026 13:37:29 -0700
Message-ID: <44cbdf50807b0232d0a01f0fababc7cbeeb33eb0.camel@j-davis.com> (raw)
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>
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
Attachments:v22-0001-Eliminate-PG_TRY-PG_CATCH-in-ForeignServerConnec.patch (text/x-patch, 3.0K) download
From bf09932dbc0c3ab73bfd5566f5f69a32d709278a Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
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 <andres@anarazel.de>
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.cindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-committers@postgresql.org
Cc: pgsql@j-davis.com, andres@anarazel.de, peter@eisentraut.org, jdavis@postgresql.org, pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: CREATE SUBSCRIPTION ... SERVER.
In-Reply-To: <44cbdf50807b0232d0a01f0fababc7cbeeb33eb0.camel@j-davis.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox