pgsql: CREATE SUBSCRIPTION ... SERVER.

On 06.03.26 17:44, Jeff Davis wrote:

CREATE SUBSCRIPTION ... SERVER.

In src/backend/foreign/foreign.c, this

volatile text *connection_text = NULL;

should probably be

text *volatile connection_text = NULL;

similar to commit 6307b096e25.

Hi,

On 2026-03-11 08:37:13 +0100, Peter Eisentraut wrote:

On 06.03.26 17:44, Jeff Davis wrote:

CREATE SUBSCRIPTION ... SERVER.

In src/backend/foreign/foreign.c, this

volatile text *connection_text = NULL;

should probably be

text *volatile connection_text = NULL;

similar to commit 6307b096e25.

Seems like the issue is a bit bigger to me. Isn't the whole way the function
uses PG_TRY / PG_FINALLY just plain odd?

The only reason connection_text needs to be volatile is because it's modified
in PG_TRY and then accessed in PG_FINALLY. But what's the point of the
latter? If an error was thrown, why would we want to construct the 'result'
string, as the error isn't caught, there's no PG_CATCH. So now the function
builds the result string in case of an error, just to throw it away.

Am I missing something?

I'm also rather unconvinced by ForeignServerConnectionString() creating a
temporary memory context. When would you ever use the function in a long lived
memory context?

Greetings,

Andres

On Wed, 2026-03-11 at 17:49 -0400, Andres Freund wrote:

The only reason connection_text needs to be volatile is because it's
modified
in PG_TRY and then accessed in PG_FINALLY.  But what's the point of
the
latter? If an error was thrown, why would we want to construct the
'result'
string, as the error isn't caught, there's no PG_CATCH.  So now the
function
builds the result string in case of an error, just to throw it away.

I believe it would be correct if the variable was not volatile at all,
because it's only set in the last statement of PG_TRY(), so the
PG_FINALLY() will never need to read the modified variable on the error
path. I added the volatile to follow the expected pattern, in case the
code is adjusted later.

That leaves some awkwardness, but I don't immediately see a cleaner way
to do it. Suggestions welcome.

I'm also rather unconvinced by ForeignServerConnectionString()
creating a
temporary memory context. When would you ever use the function in a
long lived
memory context?

It's called by GetSubscription(), and the context is ApplyContext.

Regards,
Jeff Davis

On Wed, 2026-03-11 at 17:59 -0700, Jeff Davis wrote:

That leaves some awkwardness, but I don't immediately see a cleaner
way
to do it. Suggestions welcome.

I did see a way to avoid the volatile entirely, it just needs a
redundant MemoryContextSwtichTo(). I think this is cleaner overall,
attached.

Regards,
Jeff Davis

Hi,

On 2026-03-11 17:59:40 -0700, Jeff Davis wrote:

On Wed, 2026-03-11 at 17:49 -0400, Andres Freund wrote:

The only reason connection_text needs to be volatile is because it's
modified
in PG_TRY and then accessed in PG_FINALLY.� But what's the point of
the
latter? If an error was thrown, why would we want to construct the
'result'
string, as the error isn't caught, there's no PG_CATCH.� So now the
function
builds the result string in case of an error, just to throw it away.

I believe it would be correct if the variable was not volatile at all,
because it's only set in the last statement of PG_TRY(), so the
PG_FINALLY() will never need to read the modified variable on the error
path. I added the volatile to follow the expected pattern, in case the
code is adjusted later.

That leaves some awkwardness, but I don't immediately see a cleaner way
to do it. Suggestions welcome.

I'm also rather unconvinced by ForeignServerConnectionString()
creating a
temporary memory context. When would you ever use the function in a
long lived
memory context?

It's called by GetSubscription(), and the context is ApplyContext.

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().

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. 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.

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?

Greetings,

Andres Freund

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:

/messages/by-id/f48610c90e69de4b30841361c568c3765e8f3dfe.camel@j-davis.com

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

Regards,
Jeff Davis

Hi,

On 2026-03-13 13:37:29 -0700, Jeff Davis wrote:

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.

No argument there.

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

It does indeed look like it'd leak.

I suspect the more scalable approach would be to create a dedicated memory
context for each GetSubscription() call that's then torn down during
invalidation.

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.

LGTM on a quick scan.

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:

/messages/by-id/f48610c90e69de4b30841361c568c3765e8f3dfe.camel@j-davis.com

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

I was behind on my email, so I hadn't yet seen that :)

Greetings,

Andres

On Fri, 2026-03-13 at 16:48 -0400, Andres Freund wrote:

I suspect the more scalable approach would be to create a dedicated
memory
context for each GetSubscription() call that's then torn down during
invalidation.

Attached. This is posted as an alternative to the previous patch.

Regards,
Jeff Davis