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
