Horton

Thread

  1. [PATCH] Allow complex data for GUC extra.

    Bryan Green <dbryan.green@gmail.com> — 2025-11-17T21:17:19Z

    Following up on Tom Lane's suggestion to use MemoryContexts for GUC
extra data [1], I've implemented a working solution that addresses the
design issues Robert Haas identified with my initial approach.

The original problem: GUC check hooks can only return a single chunk for
extra data, making it awkward to use complex structures like Lists or
hash tables. Tom suggested allowing the extra field to point to a
MemoryContext instead, which would enable arbitrary nested structures
with automatic cleanup via MemoryContextDelete().

My first implementation stored the context pointer directly as the extra
data. Robert pointed out the fatal flaw: during transaction rollback,
the assign hook needs to receive a data pointer (to update its global
variable), but if extra contains a context pointer, there's no way to
retrieve the actual data. A global mapping table would work but seemed
unnecessarily complex.

The solution uses a wrapper struct (GucContextExtra) containing both the
MemoryContext and data pointers. Check hooks:
  1. Create a context under CurrentMemoryContext (for error safety)
  2. Allocate their data structures within it
  3. Allocate the wrapper itself within the same context
  4. On success, re-parent the context to TopMemoryContext
  5. Return the wrapper as extra

The GUC machinery manages wrapper pointers in its stack. On rollback,
the assign hook receives the old wrapper and extracts the correct old
data pointer, maintaining proper transaction semantics. When freeing
extra data, we simply delete the context - since the wrapper lives
inside it, everything is freed with one call.

Error handling is automatic: if the check hook errors during parsing,
the context is still under CurrentMemoryContext and gets cleaned up
normally, preventing leaks.

The attached patch adds:
  - GUC_EXTRA_IS_CONTEXT flag
  - GucContextExtra struct definition
  - Modified free_extra_value() to handle both paths
  - Test module (src/test/modules/test_guc) with simple counter
(traditional path, no context) and server pool (context-based path with
List)

Regression tests validate that Lists survive transaction rollback and
savepoint operations correctly.

Thoughts?

Patch attached.

[1] https://discord.com/channels/1258108670710124574/1402360503036285130

-- 
Bryan Green
EDB: https://www.enterprisedb.com

  2. Re: [PATCH] Allow complex data for GUC extra.

    Robert Haas <robertmhaas@gmail.com> — 2025-11-18T17:24:50Z

    On Mon, Nov 17, 2025 at 4:17 PM Bryan Green <dbryan.green@gmail.com> wrote:
> The solution uses a wrapper struct (GucContextExtra) containing both the
> MemoryContext and data pointers. Check hooks:
>   1. Create a context under CurrentMemoryContext (for error safety)
>   2. Allocate their data structures within it
>   3. Allocate the wrapper itself within the same context
>   4. On success, re-parent the context to TopMemoryContext
>   5. Return the wrapper as extra

An alternative design would be to make the check hook simply return a
chunk palloc'd from the new context, and the GUC machinery would use
GetMemoryChunkContext() to recover the context pointer and then
MemoryContextDelete that context. I'm not sure if that's better or
worse.

I think one of the big usability questions around this is how the
check hook is supposed to avoid leaking if it errors out. The approach
you've taken is to have the check hook create the context under
CurrentMemoryContext and then reparent it just before returning, which
may be fine, but is worth discussing. I'm not 100% sure that it's
actually good enough for every case: is there no situation where a
check hook can be called without a CurrentMemoryContext, or with a
very long-lived memory context like TopMemoryContext set to current?
Even if there's technically a leak here, maybe we don't care: it might
be limited enough not to matter.

A whole different way of doing this would be to make the GUC machinery
responsible for spinning up and tearing down the contexts. Then, the
check hook could just be called with CurrentMemoryContext already set
to the new context, and the caller would know about it. Then, the
check hook doesn't need any special precautions to make sure the
context gets destroyed; instead, the GUC machinery takes care of that.
Here again, I'm not sure if this is better or worse than what you
have.

Thanks for working on this.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  3. Re: [PATCH] Allow complex data for GUC extra.

    Bryan Green <dbryan.green@gmail.com> — 2025-11-18T17:47:17Z

    On 11/18/2025 11:24 AM, Robert Haas wrote:
> On Mon, Nov 17, 2025 at 4:17 PM Bryan Green <dbryan.green@gmail.com> wrote:
>> The solution uses a wrapper struct (GucContextExtra) containing both the
>> MemoryContext and data pointers. Check hooks:
>>   1. Create a context under CurrentMemoryContext (for error safety)
>>   2. Allocate their data structures within it
>>   3. Allocate the wrapper itself within the same context
>>   4. On success, re-parent the context to TopMemoryContext
>>   5. Return the wrapper as extra
> 
> An alternative design would be to make the check hook simply return a
> chunk palloc'd from the new context, and the GUC machinery would use
> GetMemoryChunkContext() to recover the context pointer and then
> MemoryContextDelete that context. I'm not sure if that's better or
> worse.
> 

This one is better I believe, but it still requires the check hook to
manage context.

> I think one of the big usability questions around this is how the
> check hook is supposed to avoid leaking if it errors out. The approach
> you've taken is to have the check hook create the context under
> CurrentMemoryContext and then reparent it just before returning, which
> may be fine, but is worth discussing. I'm not 100% sure that it's
> actually good enough for every case: is there no situation where a
> check hook can be called without a CurrentMemoryContext, or with a
> very long-lived memory context like TopMemoryContext set to current?
> Even if there's technically a leak here, maybe we don't care: it might
> be limited enough not to matter.
> 

You are correct, the reparenting approach could still leak memory. I
found examples where the current memory context is already TopMemoryContext.

> A whole different way of doing this would be to make the GUC machinery
> responsible for spinning up and tearing down the contexts. Then, the
> check hook could just be called with CurrentMemoryContext already set
> to the new context, and the caller would know about it. Then, the
> check hook doesn't need any special precautions to make sure the
> context gets destroyed; instead, the GUC machinery takes care of that.
> Here again, I'm not sure if this is better or worse than what you
> have.
> 

At first blush, I am leaning towards this solution because it seems
cleaner and not leaky.  The GUC machinery would:

  1. Create a temporary context (child of TopMemoryContext)
  2. Set it as CurrentMemoryContext
  3. Call check_hook (allocates freely with palloc)
  4. Restore previous CurrentMemoryContext
  5. On success: keep context, store pointer to data
  6. On error: delete context automatically

Check hooks then become trivial - you just palloc what you need, no
context management at all. The machinery handles everything.

The flag (GUC_EXTRA_IS_CONTEXT) handles that will still handle
distinquishing between plain extra data and context as extra data.

Combined with your GetMemoryChunkContext() idea, we could eliminate the
wrapper entirely:

In GUC machinery (set_config_option):
  if (gconf->flags & GUC_EXTRA_IS_CONTEXT)
  {
      extra_cxt = AllocSetContextCreate(TopMemoryContext, ...);
      old_context = MemoryContextSwitchTo(extra_cxt);
  }

  /* Call check hook - just pallocs what it needs */
  if (!call_check_hook(..., &extra))
  {
      if (gconf->flags & GUC_EXTRA_IS_CONTEXT)
          MemoryContextDelete(extra_cxt);
      return false;
  }

  if (gconf->flags & GUC_EXTRA_IS_CONTEXT)
      MemoryContextSwitchTo(old_context);

In free_extra_value():
  if (gconf->flags & GUC_EXTRA_IS_CONTEXT)
      MemoryContextDelete(GetMemoryChunkContext(extra));
  else
      guc_free(extra);

This is significantly cleaner. The downside is more complexity in the
GUC machinery itself, but it makes check hooks much simpler to write
and reduces the chances of getting them wrong.

Thoughts? I'm happy to rework the patch along these lines if this
approach seems better-- which it does to me.

> Thanks for working on this.
> 


-- 
Bryan Green
EDB: https://www.enterprisedb.com

  4. Re: [PATCH] Allow complex data for GUC extra.

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-11-18T18:21:22Z

    Bryan Green <dbryan.green@gmail.com> writes:
> On 11/18/2025 11:24 AM, Robert Haas wrote:
>> A whole different way of doing this would be to make the GUC machinery
>> responsible for spinning up and tearing down the contexts. Then, the
>> check hook could just be called with CurrentMemoryContext already set
>> to the new context, and the caller would know about it. Then, the
>> check hook doesn't need any special precautions to make sure the
>> context gets destroyed; instead, the GUC machinery takes care of that.

I like this in principle, but I don't think Bryan's implementation
sketch is right:

>   1. Create a temporary context (child of TopMemoryContext)

If the check_hook throws an error, you'll have leaked a long-lived
context.  You must *not* make it a child of TopMemoryContext until
after successful assignment.  I take Robert's point that we don't
know whether the GUC logic will be called in a context that is
short-lived or long-lived, so maybe making the context transiently
a child of CurrentMemoryContext isn't good enough ... but
TopMemoryContext is most definitely not good enough.

(Actually, these things should be children of GUCMemoryContext
not directly of TopMemoryContext.  But that doesn't affect this
point, since those are equally long-lived.)

I'm really still dubious that this entire project is worthwhile.
I think it is basically building support for GUCs whose values
are unreasonably complicated, and would be better off if they
got redesigned.  Also, right now might be a bad time to be
adding complexity to guc.c, in view of discussions such as [1].

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/2ff46ac9-b46c-4210-8f0c-0f5365b36db9%40eisentraut.org

  5. Re: [PATCH] Allow complex data for GUC extra.

    Robert Haas <robertmhaas@gmail.com> — 2025-11-18T19:56:22Z

    On Tue, Nov 18, 2025 at 1:21 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm really still dubious that this entire project is worthwhile.
> I think it is basically building support for GUCs whose values
> are unreasonably complicated, and would be better off if they
> got redesigned.  Also, right now might be a bad time to be
> adding complexity to guc.c, in view of discussions such as [1].

What motivated me to care about this was pg_plan_advice.advice, which
is indeed arguably too complicated to be a GUC, but I don't have a
better idea right now. I thought about using a pg_upgrade-support
style thing, like SELECT pg_plan_advice.next_advice_is('text value')
-- but this seems really awkward to me in the context of code running
inside of a function, because we don't know whether the query we're
about to see is going to get planned or not. And while someone may
think "just pass it through via the SQL comments" is a solution to
this problem, I find I cannot agree for a whole long list of reasons.
It seems to most naturally fit as a GUC, even though that's not a
great fit.

Also, that's not the only case we have of something like this.
check_synchronous_standby_names and check_synchronized_standby_slots
are good, existing examples of where substantial parsing is required
and then effort must be expended to get it back into a single palloc'd
chunk. check_backtrace_functions is an interesting example too: would
we really pick this particular representation if the GUC
infrastructure didn't require it? I have my doubts.

In general, I don't think that whether or not a GUC's parsed value can
be serialized easily into a single palloc'd chunk is a good measure of
whether it's too complicated. I agree, of course, that we shouldn't
randomly sandwhich a bunch of disparate values into a single GUC --
several separate GUCs is better. However, what about a value that
intrinsically has some internal structure? We originally thought that
we wanted synchronous_standby_names to just be a list of standbys,
which barely qualifies as internal structure and so fits with the idea
of a single palloc'd chunk, but then we decided we wanted to allow
prefixing that list stuff like ANY 2 or FIRST 3. Does that make it no
longer suitable to be a GUC? What if we had instead decided to allow
nested structure, like synchronous_standby_names = a, (b, c), d? That
definitely isn't nice for a flat structure, but I doubt anyone would
like it if that adjustment suddenly meant it had to be some other kind
of thing rather than a GUC, and what would the other thing be, anyway?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  6. Re: [PATCH] Allow complex data for GUC extra.

    Andrew Dunstan <andrew@dunslane.net> — 2025-11-21T22:26:52Z

    On 2025-11-18 Tu 2:56 PM, Robert Haas wrote:
>
> In general, I don't think that whether or not a GUC's parsed value can
> be serialized easily into a single palloc'd chunk is a good measure of
> whether it's too complicated.


+1


> I agree, of course, that we shouldn't
> randomly sandwhich a bunch of disparate values into a single GUC --
> several separate GUCs is better. However, what about a value that
> intrinsically has some internal structure? We originally thought that
> we wanted synchronous_standby_names to just be a list of standbys,
> which barely qualifies as internal structure and so fits with the idea
> of a single palloc'd chunk, but then we decided we wanted to allow
> prefixing that list stuff like ANY 2 or FIRST 3. Does that make it no
> longer suitable to be a GUC? What if we had instead decided to allow
> nested structure, like synchronous_standby_names = a, (b, c), d? That
> definitely isn't nice for a flat structure, but I doubt anyone would
> like it if that adjustment suddenly meant it had to be some other kind
> of thing rather than a GUC, and what would the other thing be, anyway?
>

If GUC A depends for sanity on the value of GUC B, it seems rather odd 
to force them to be independent at the grammar level. A structured GUC 
would make more sense in such a case.

One of the things that bothers me a bit here is that we seem to be 
inventing a bunch of micro-languages to deal with structured GUC data. 
<asbestos-mode> Maybe they could all be JSON?</>


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  7. Re: [PATCH] Allow complex data for GUC extra.

    Robert Haas <robertmhaas@gmail.com> — 2025-11-22T18:55:18Z

    On Fri, Nov 21, 2025 at 5:26 PM Andrew Dunstan <andrew@dunslane.net> wrote:
> One of the things that bothers me a bit here is that we seem to be
> inventing a bunch of micro-languages to deal with structured GUC data.
> <asbestos-mode> Maybe they could all be JSON?</>

I can understand why you (or anyone) would suggest this, but I don't
think it would actually be better, for three reasons. First, changing
the format for something like synchronous_standby_names would be a
backward compatibility break. Second, I suspect the resulting format
would be more long-winded. Instead of ANY 2 (foo, bar, baz) you'd have
to write something like { 'op' => 'ANY', 'num' => 2, 'servers' => [
'foo', 'bar', 'baz' ]}. Third, if my experience with using JSON for
backup manifests is any indication, it would actually add
significantly more code. The JSON parser doesn't do all the work for
you, because you have to do semantic validation of the JSON
afterwards. See parse_manifest.c for an example of what I mean.

I think the real strength of JSON is not that it's any easier to use
or objectively better than a mini-language, but that it's a standard.
If we threw out the entire postgresql.conf format and replaced it with
a big JSON document whose keys were GUC names and whose values were
all JSON values, then somebody could manipulate that whole document
using any JSON tool that they like, and that would probably be handy
for some people. Same if we made the whole thing XML or whatever. We
would probably make life harder for ourselves, but if it had enough
benefit for users, it might worth it. I don't think users would
actually be happy about such a change and I'm not proposing it, but
it's the kind of thing I can imagine making sense hypothetically. If
we were starting from scratch rather than trying to maintain
compatibility with our previous releases, it would be worth thinking
about.

Absent that, I think ad-hoc mini-languages are a pretty good idea. By
designing something that does exactly what you need and nothing else,
you can often create something that is clear, succinct, and easy for
users to learn. synchronous_standby_names is a good example: it's not
that hard to understand how it works, it does what we need, and it
doesn't become any easier to use if you make it JSON or XML or
whatever. The plan advice mini-language is a more arguable case, but I
based that on existing mini-languages that do similar things and then
adapted it for what I was trying to do, which means that people
familiar with those other things might have an easier learning curve.
I am of course open to alternate ideas of how that language should
work, and there are definitely things about it that I don't like. But
as far as I can see, none of the things I'm unhappy about would be
fixed by using JSON.

In response to your asbestos-mode tag, let me say that none of this is
intended to flame you, or to express outrage. I know a lot of people
would say that my appreciation for a well-chosen mini-language is
wrong-headed, and you may be one of them, and that's fair enough. But
I see it differently, so this is just to explain my view of it.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  8. Re: [PATCH] Allow complex data for GUC extra.

    Bruce Momjian <bruce@momjian.us> — 2025-11-22T19:00:05Z

    On Fri, Nov 21, 2025 at 05:26:52PM -0500, Andrew Dunstan wrote:
> > I agree, of course, that we shouldn't
> > randomly sandwhich a bunch of disparate values into a single GUC --
> > several separate GUCs is better. However, what about a value that
> > intrinsically has some internal structure? We originally thought that
> > we wanted synchronous_standby_names to just be a list of standbys,
> > which barely qualifies as internal structure and so fits with the idea
> > of a single palloc'd chunk, but then we decided we wanted to allow
> > prefixing that list stuff like ANY 2 or FIRST 3. Does that make it no
> > longer suitable to be a GUC? What if we had instead decided to allow
> > nested structure, like synchronous_standby_names = a, (b, c), d? That
> > definitely isn't nice for a flat structure, but I doubt anyone would
> > like it if that adjustment suddenly meant it had to be some other kind
> > of thing rather than a GUC, and what would the other thing be, anyway?
> > 
> 
> If GUC A depends for sanity on the value of GUC B, it seems rather odd to
> force them to be independent at the grammar level. A structured GUC would
> make more sense in such a case.
> 
> One of the things that bothers me a bit here is that we seem to be inventing
> a bunch of micro-languages to deal with structured GUC data. <asbestos-mode>
> Maybe they could all be JSON?</>

As long as you didn't say XML, we are good.  ;-)

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

  9. Re: [PATCH] Allow complex data for GUC extra.

    Bryan Green <dbryan.green@gmail.com> — 2025-12-05T05:45:16Z

    On 11/18/2025 12:21 PM, Tom Lane wrote:
> Bryan Green <dbryan.green@gmail.com> writes:
>> On 11/18/2025 11:24 AM, Robert Haas wrote:
>>> A whole different way of doing this would be to make the GUC machinery
>>> responsible for spinning up and tearing down the contexts. Then, the
>>> check hook could just be called with CurrentMemoryContext already set
>>> to the new context, and the caller would know about it. Then, the
>>> check hook doesn't need any special precautions to make sure the
>>> context gets destroyed; instead, the GUC machinery takes care of that.
> 
> I like this in principle, but I don't think Bryan's implementation
> sketch is right:
> 
>>   1. Create a temporary context (child of TopMemoryContext)
> 
> If the check_hook throws an error, you'll have leaked a long-lived
> context.  You must *not* make it a child of TopMemoryContext until
> after successful assignment.  I take Robert's point that we don't
> know whether the GUC logic will be called in a context that is
> short-lived or long-lived, so maybe making the context transiently
> a child of CurrentMemoryContext isn't good enough ... but
> TopMemoryContext is most definitely not good enough.
> 
> (Actually, these things should be children of GUCMemoryContext
> not directly of TopMemoryContext.  But that doesn't affect this
> point, since those are equally long-lived.)
> 
> I'm really still dubious that this entire project is worthwhile.
> I think it is basically building support for GUCs whose values
> are unreasonably complicated, and would be better off if they
> got redesigned.  Also, right now might be a bad time to be
> adding complexity to guc.c, in view of discussions such as [1].
> 
> 			regards, tom lane
> 
> [1] https://www.postgresql.org/message-id/flat/2ff46ac9-b46c-4210-8f0c-0f5365b36db9%40eisentraut.org

I tried implementing a PG_TRY/PG_CATCH approach and it doesn't work.
The switch statement in set_config_with_handle() has multiple early
returns (parse failures, prohibitValueChange checks, etc.) that bypass
both the success path and the PG_CATCH handler. If we've switched into
extra_cxt before entering the switch, these early returns leave
CurrentMemoryContext pointing at a temp context.

If the GUC machinery switches contexts before calling the check hook, we
have to switch back on every exit path, but the early returns make that
difficult without refactoring the entire switch statement-- it's quite
the switch statement.

Maybe an alternative: create the temp context under
CurrentMemoryContext, but don't switch into it. The check hook switches
if it needs complex structures, and switches back before returning. On
success, we reparent to GUCMemoryContext. On error or early return, the
context is automatically deleted with its parent.

I think there may still be a small leak in the case if
CurrentMemoryContext is long-lived.

The check hook API would be:

  MemoryContext oldcxt = MemoryContextSwitchTo(extra_cxt);
  /* allocate complex structures with palloc */
  MemoryContextSwitchTo(oldcxt);
  *extra = my_data_pointer;

Not as automatic as Robert's suggestion, but it avoids the early return
problem entirely.

Thoughts?

-- 
Bryan Green
EDB: https://www.enterprisedb.com

  10. Re: [PATCH] Allow complex data for GUC extra.

    Robert Haas <robertmhaas@gmail.com> — 2025-12-05T20:48:32Z

    On Fri, Dec 5, 2025 at 12:45 AM Bryan Green <dbryan.green@gmail.com> wrote:
> I tried implementing a PG_TRY/PG_CATCH approach and it doesn't work.
> The switch statement in set_config_with_handle() has multiple early
> returns (parse failures, prohibitValueChange checks, etc.) that bypass
> both the success path and the PG_CATCH handler. If we've switched into
> extra_cxt before entering the switch, these early returns leave
> CurrentMemoryContext pointing at a temp context.

I'm pretty sure it's not intended that you can return out of a
PG_CATCH() block. You could, however, modify the control flow so that
you stash the return value in a variable and the actual return happens
after you exit the PG_CATCH() block.

But I also don't understand why you want to use a PG_CATCH() block
here in the first place. At first glance, I'm inclined to wonder why
this wouldn't be a new wrinkle for the existing logic in
call_string_check_hook.

> The check hook API would be:
>
>   MemoryContext oldcxt = MemoryContextSwitchTo(extra_cxt);
>   /* allocate complex structures with palloc */
>   MemoryContextSwitchTo(oldcxt);
>   *extra = my_data_pointer;
>
> Not as automatic as Robert's suggestion, but it avoids the early return
> problem entirely.

This wouldn't be terrible or anything, and someone may prefer it on
stylistic grounds, but I don't really think I believe your argument
that this is the only way it can work.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  11. Re: [PATCH] Allow complex data for GUC extra.

    Bryan Green <dbryan.green@gmail.com> — 2025-12-05T21:24:46Z

    On 12/5/2025 2:48 PM, Robert Haas wrote:
> On Fri, Dec 5, 2025 at 12:45 AM Bryan Green <dbryan.green@gmail.com> wrote:
>> I tried implementing a PG_TRY/PG_CATCH approach and it doesn't work.
>> The switch statement in set_config_with_handle() has multiple early
>> returns (parse failures, prohibitValueChange checks, etc.) that bypass
>> both the success path and the PG_CATCH handler. If we've switched into
>> extra_cxt before entering the switch, these early returns leave
>> CurrentMemoryContext pointing at a temp context.
> 
> I'm pretty sure it's not intended that you can return out of a
> PG_CATCH() block. You could, however, modify the control flow so that
> you stash the return value in a variable and the actual return happens
> after you exit the PG_CATCH() block.
> 

I should have been more clear, I was referring to trying the following:


  if (GUC_EXTRA_IS_CONTEXT && value != NULL)
  {
      extra_cxt = AllocSetContextCreate(CurrentMemoryContext, ...);
      old_context = MemoryContextSwitchTo(extra_cxt);
  }

  PG_TRY();
  {
      switch (record->vartype) { ... }   /* DIFFERENT RETURN PATHS */

      /* Success path */
      if (extra_cxt)
      {
          MemoryContextSwitchTo(old_context);
          MemoryContextSetParent(extra_cxt, GUCMemoryContext);
      }
  }
  PG_CATCH();
  {
      if (extra_cxt)
          MemoryContextDelete(extra_cxt);
      PG_RE_THROW();
  }
  PG_END_TRY();

The early returns are inside the PG_TRY block (in the switch
statement), not in PG_CATCH. But I see your point - I could refactor
to use a result variable and only return after PG_END_TRY.

Some of the "return 0" paths happen after the check hook has already
run and allocated into extra_cxt. If I just break out of the switch
to avoid the return, I'd still need to distinguish "should I reparent
this context (success) or delete it (failure)" before exiting PG_TRY.

> But I also don't understand why you want to use a PG_CATCH() block
> here in the first place. At first glance, I'm inclined to wonder why
> this wouldn't be a new wrinkle for the existing logic in
> call_string_check_hook.
>

I think I'm missing something obvious here. call_string_check_hook
doesn't do any memory context management - it just calls the hook.

Are you suggesting the context creation/switching should be factored
into the call_*_check_hook functions themselves? That would keep it
out of the main switch statement entirely. Something like:

  if (record->flags & GUC_EXTRA_IS_CONTEXT)
      return call_string_check_hook_with_context(...);
  else
      return call_string_check_hook(...);

Where the _with_context version handles creating the temp context,
switching into it, calling the hook, switching back, and cleaning up
on failure?

That would avoid touching the switch statement at all. Is that what
you had in mind?

>> The check hook API would be:
>>
>>   MemoryContext oldcxt = MemoryContextSwitchTo(extra_cxt);
>>   /* allocate complex structures with palloc */
>>   MemoryContextSwitchTo(oldcxt);
>>   *extra = my_data_pointer;
>>
>> Not as automatic as Robert's suggestion, but it avoids the early return
>> problem entirely.
> 
> This wouldn't be terrible or anything, and someone may prefer it on
> stylistic grounds, but I don't really think I believe your argument
> that this is the only way it can work.
> 

I did not mean to imply that this is the ONLY way it could work-- it was
just the solution that was in my mind currently.  I always assume there
are multiple ways.

Thanks

-- 
Bryan Green
EDB: https://www.enterprisedb.com

  12. Re: [PATCH] Allow complex data for GUC extra.

    Bryan Green <dbryan.green@gmail.com> — 2025-12-06T07:08:47Z

    On 12/5/2025 3:24 PM, Bryan Green wrote:
> On 12/5/2025 2:48 PM, Robert Haas wrote:
>> On Fri, Dec 5, 2025 at 12:45 AM Bryan Green <dbryan.green@gmail.com> wrote:
>>> I tried implementing a PG_TRY/PG_CATCH approach and it doesn't work.
>>> The switch statement in set_config_with_handle() has multiple early
>>> returns (parse failures, prohibitValueChange checks, etc.) that bypass
>>> both the success path and the PG_CATCH handler. If we've switched into
>>> extra_cxt before entering the switch, these early returns leave
>>> CurrentMemoryContext pointing at a temp context.
>>
>> I'm pretty sure it's not intended that you can return out of a
>> PG_CATCH() block. You could, however, modify the control flow so that
>> you stash the return value in a variable and the actual return happens
>> after you exit the PG_CATCH() block.
>>
> 
> I should have been more clear, I was referring to trying the following:
> 
> 
>   if (GUC_EXTRA_IS_CONTEXT && value != NULL)
>   {
>       extra_cxt = AllocSetContextCreate(CurrentMemoryContext, ...);
>       old_context = MemoryContextSwitchTo(extra_cxt);
>   }
> 
>   PG_TRY();
>   {
>       switch (record->vartype) { ... }   /* DIFFERENT RETURN PATHS */
> 
>       /* Success path */
>       if (extra_cxt)
>       {
>           MemoryContextSwitchTo(old_context);
>           MemoryContextSetParent(extra_cxt, GUCMemoryContext);
>       }
>   }
>   PG_CATCH();
>   {
>       if (extra_cxt)
>           MemoryContextDelete(extra_cxt);
>       PG_RE_THROW();
>   }
>   PG_END_TRY();
> 
> The early returns are inside the PG_TRY block (in the switch
> statement), not in PG_CATCH. But I see your point - I could refactor
> to use a result variable and only return after PG_END_TRY.
> 
> Some of the "return 0" paths happen after the check hook has already
> run and allocated into extra_cxt. If I just break out of the switch
> to avoid the return, I'd still need to distinguish "should I reparent
> this context (success) or delete it (failure)" before exiting PG_TRY.
> 
>> But I also don't understand why you want to use a PG_CATCH() block
>> here in the first place. At first glance, I'm inclined to wonder why
>> this wouldn't be a new wrinkle for the existing logic in
>> call_string_check_hook.
>>
> 
> I think I'm missing something obvious here. call_string_check_hook
> doesn't do any memory context management - it just calls the hook.
> 
> Are you suggesting the context creation/switching should be factored
> into the call_*_check_hook functions themselves? That would keep it
> out of the main switch statement entirely. Something like:
> 
>   if (record->flags & GUC_EXTRA_IS_CONTEXT)
>       return call_string_check_hook_with_context(...);
>   else
>       return call_string_check_hook(...);
> 
> Where the _with_context version handles creating the temp context,
> switching into it, calling the hook, switching back, and cleaning up
> on failure?
> 
> That would avoid touching the switch statement at all. Is that what
> you had in mind?
> 
>>> The check hook API would be:
>>>
>>>   MemoryContext oldcxt = MemoryContextSwitchTo(extra_cxt);
>>>   /* allocate complex structures with palloc */
>>>   MemoryContextSwitchTo(oldcxt);
>>>   *extra = my_data_pointer;
>>>
>>> Not as automatic as Robert's suggestion, but it avoids the early return
>>> problem entirely.
>>
>> This wouldn't be terrible or anything, and someone may prefer it on
>> stylistic grounds, but I don't really think I believe your argument
>> that this is the only way it can work.
>>
> 
> I did not mean to imply that this is the ONLY way it could work-- it was
> just the solution that was in my mind currently.  I always assume there
> are multiple ways.
> 
> Thanks
> 
Robert,

I've implemented the GUC_EXTRA_IS_CONTEXT approach I believe you were
suggesting. The basic idea is straightforward: the check hook wrapper
creates a temporary AllocSetContext, switches to it before calling the
hook, then either reparents the context to GUCMemoryContext on success
or deletes it on failure. Cleanup in set_extra_field() uses
GetMemoryChunkContext() to locate and delete the old context.
This required modifications to all five call_*_check_hook() functions
(bool, int, real, string, enum) to follow the same pattern. I also had
to keep the context operations outside the PG_TRY block.
One additional fix: if a check hook succeeds but returns NULL for extra,
we delete the empty context rather than reparenting it to avoid leaking
contexts that would never be cleaned up.

Does this match what you had in mind?

Patch attached.

-- 
Bryan Green
EDB: https://www.enterprisedb.com

  13. Re: [PATCH] Allow complex data for GUC extra.

    Bryan Green <dbryan.green@gmail.com> — 2025-12-08T15:23:00Z

    On 12/6/2025 1:08 AM, Bryan Green wrote:
> On 12/5/2025 3:24 PM, Bryan Green wrote:
>> On 12/5/2025 2:48 PM, Robert Haas wrote:
>>> On Fri, Dec 5, 2025 at 12:45 AM Bryan Green <dbryan.green@gmail.com> wrote:
>>>> I tried implementing a PG_TRY/PG_CATCH approach and it doesn't work.
>>>> The switch statement in set_config_with_handle() has multiple early
>>>> returns (parse failures, prohibitValueChange checks, etc.) that bypass
>>>> both the success path and the PG_CATCH handler. If we've switched into
>>>> extra_cxt before entering the switch, these early returns leave
>>>> CurrentMemoryContext pointing at a temp context.
>>>
>>> I'm pretty sure it's not intended that you can return out of a
>>> PG_CATCH() block. You could, however, modify the control flow so that
>>> you stash the return value in a variable and the actual return happens
>>> after you exit the PG_CATCH() block.
>>>
>>
>> I should have been more clear, I was referring to trying the following:
>>
>>
>>   if (GUC_EXTRA_IS_CONTEXT && value != NULL)
>>   {
>>       extra_cxt = AllocSetContextCreate(CurrentMemoryContext, ...);
>>       old_context = MemoryContextSwitchTo(extra_cxt);
>>   }
>>
>>   PG_TRY();
>>   {
>>       switch (record->vartype) { ... }   /* DIFFERENT RETURN PATHS */
>>
>>       /* Success path */
>>       if (extra_cxt)
>>       {
>>           MemoryContextSwitchTo(old_context);
>>           MemoryContextSetParent(extra_cxt, GUCMemoryContext);
>>       }
>>   }
>>   PG_CATCH();
>>   {
>>       if (extra_cxt)
>>           MemoryContextDelete(extra_cxt);
>>       PG_RE_THROW();
>>   }
>>   PG_END_TRY();
>>
>> The early returns are inside the PG_TRY block (in the switch
>> statement), not in PG_CATCH. But I see your point - I could refactor
>> to use a result variable and only return after PG_END_TRY.
>>
>> Some of the "return 0" paths happen after the check hook has already
>> run and allocated into extra_cxt. If I just break out of the switch
>> to avoid the return, I'd still need to distinguish "should I reparent
>> this context (success) or delete it (failure)" before exiting PG_TRY.
>>
>>> But I also don't understand why you want to use a PG_CATCH() block
>>> here in the first place. At first glance, I'm inclined to wonder why
>>> this wouldn't be a new wrinkle for the existing logic in
>>> call_string_check_hook.
>>>
>>
>> I think I'm missing something obvious here. call_string_check_hook
>> doesn't do any memory context management - it just calls the hook.
>>
>> Are you suggesting the context creation/switching should be factored
>> into the call_*_check_hook functions themselves? That would keep it
>> out of the main switch statement entirely. Something like:
>>
>>   if (record->flags & GUC_EXTRA_IS_CONTEXT)
>>       return call_string_check_hook_with_context(...);
>>   else
>>       return call_string_check_hook(...);
>>
>> Where the _with_context version handles creating the temp context,
>> switching into it, calling the hook, switching back, and cleaning up
>> on failure?
>>
>> That would avoid touching the switch statement at all. Is that what
>> you had in mind?
>>
>>>> The check hook API would be:
>>>>
>>>>   MemoryContext oldcxt = MemoryContextSwitchTo(extra_cxt);
>>>>   /* allocate complex structures with palloc */
>>>>   MemoryContextSwitchTo(oldcxt);
>>>>   *extra = my_data_pointer;
>>>>
>>>> Not as automatic as Robert's suggestion, but it avoids the early return
>>>> problem entirely.
>>>
>>> This wouldn't be terrible or anything, and someone may prefer it on
>>> stylistic grounds, but I don't really think I believe your argument
>>> that this is the only way it can work.
>>>
>>
>> I did not mean to imply that this is the ONLY way it could work-- it was
>> just the solution that was in my mind currently.  I always assume there
>> are multiple ways.
>>
>> Thanks
>>
> Robert,
> 
> I've implemented the GUC_EXTRA_IS_CONTEXT approach I believe you were
> suggesting. The basic idea is straightforward: the check hook wrapper
> creates a temporary AllocSetContext, switches to it before calling the
> hook, then either reparents the context to GUCMemoryContext on success
> or deletes it on failure. Cleanup in set_extra_field() uses
> GetMemoryChunkContext() to locate and delete the old context.
> This required modifications to all five call_*_check_hook() functions
> (bool, int, real, string, enum) to follow the same pattern. I also had
> to keep the context operations outside the PG_TRY block.
> One additional fix: if a check hook succeeds but returns NULL for extra,
> we delete the empty context rather than reparenting it to avoid leaking
> contexts that would never be cleaned up.
> 
> Does this match what you had in mind?
> 
> Patch attached.
> 
Actually, I realized I still allocated in CurrentMemoryContext-- I think
instead I should just allocate the extra_cxt under GUCMemoryContext and
then there is no need to reparent.

if (conf->flags & GUC_EXTRA_IS_CONTEXT)
{
	/* Create directly under GUCMemoryContext - it's already where we want
it */
	extra_cxt = AllocSetContextCreate(GUCMemoryContext,							     		
"GUC check_hook extra context",
			ALLOCSET_DEFAULT_SIZES);
			old_cxt = MemoryContextSwitchTo(extra_cxt);
}

// ...

if (result)
{
	/* Already under GUCMemoryContext, just leave it there */
	/* Delete if unused */
	if (*extra == NULL)
		MemoryContextDelete(extra_cxt);
}
else
{
	MemoryContextDelete(extra_cxt);
}

-- 
Bryan Green
EDB: https://www.enterprisedb.com

  14. Re: [PATCH] Allow complex data for GUC extra.

    Bryan Green <dbryan.green@gmail.com> — 2025-12-08T15:55:41Z

    On 12/8/2025 9:23 AM, Bryan Green wrote:
> On 12/6/2025 1:08 AM, Bryan Green wrote:
...
> Actually, I realized I still allocated in CurrentMemoryContext-- I think
> instead I should just allocate the extra_cxt under GUCMemoryContext and
> then there is no need to reparent.
> 
> if (conf->flags & GUC_EXTRA_IS_CONTEXT)
> {
> 	/* Create directly under GUCMemoryContext - it's already where we want
> it */
> 	extra_cxt = AllocSetContextCreate(GUCMemoryContext,							     		
> "GUC check_hook extra context",
> 			ALLOCSET_DEFAULT_SIZES);
> 			old_cxt = MemoryContextSwitchTo(extra_cxt);
> }
> 
> // ...
> 
> if (result)
> {
> 	/* Already under GUCMemoryContext, just leave it there */
> 	/* Delete if unused */
> 	if (*extra == NULL)
> 		MemoryContextDelete(extra_cxt);
> }
> else
> {
> 	MemoryContextDelete(extra_cxt);
> }
> 
Apologies for the unneeded email above.  Upon more reflection, I need to
walk back my previous change from CurrentMemoryContext to
GUCMemoryContext. I think CurrentMemoryContext was correct.
The issue is the ERROR path. If a check hook throws ERROR, we longjmp
out without hitting the cleanup code, leaving extra_cxt orphaned under
whatever parent we gave it.

With CurrentMemoryContext as parent (typically MessageContext or
similar), error recovery resets those contexts, and MemoryContextReset()
deletes all children via MemoryContextDeleteChildren(). So the orphaned
context gets cleaned up automatically.

With GUCMemoryContext as parent, it never gets reset during normal error
recovery, so the orphaned context just sits there leaking memory.
So the original code was actually relying on PostgreSQL's error recovery
to handle the ERROR case, which is the right approach here.

Hopefully my understanding of this is correct.

The last attached patches are still the correct ones.

-- 
Bryan Green
EDB: https://www.enterprisedb.com

  15. Re: [PATCH] Allow complex data for GUC extra.

    Robert Haas <robertmhaas@gmail.com> — 2025-12-08T16:48:35Z

    On Sat, Dec 6, 2025 at 2:08 AM Bryan Green <dbryan.green@gmail.com> wrote:
> > I think I'm missing something obvious here. call_string_check_hook
> > doesn't do any memory context management - it just calls the hook.

No, it does do memory management. It has a PG_TRY()/PG_CATCH() block
to ensure that we don't forget to GUC_free(*newval) in case of error.
I was trying to figure out where we were doing the relevant memory
management today, and then extend that to handle the new thing. But I
am guilty of fuzzy thinking here, because we're talking about where
the "extra" memory is managed, not the memory for "newval". So the
logic we care about is in set_config_with_handle() just as you said:

                /* Perhaps we didn't install newextra anywhere */
                if (newextra && !extra_field_used(record, newextra))
                    guc_free(newextra);

What I hadn't quite internalized previously was that there's no
PG_TRY/PG_CATCH block here right now because we assume that (1) we
assume the check hook won't allocate the extra value until it's ready
to return, so it will never leak a value by allocating it and then
erroring out and (2) we take care to ensure that no errors can happen
in the GUC code itself after the extra value has been returned and
before we either free it or save a pointer to it someplace.

But having said that, I'm inclined to think that handling the memory
management concerns inside call_WHATEVER_check_hook() still makes some
sense. It seems to me that if we do that, set_config_with_handle()
needs very little change. All it needs to do differently is: wherever
it would guc_free(newextra), it can call some new helper function that
will either just guc_free() or alternatively
MemoryContextDelete(GetMemoryChunkContext()) depending on flags. I
think this is good, because set_config_with_handle() is already pretty
complicated, and I'd rather not inject more complexity into that
function.

For this to work, each call_WHATEVER_check_hook() function would need
a PG_TRY()/PG_CATCH() block, rather than only call_string_check_hook()
as currently. Or alternatively, and I think this might be an appealing
option, we could say that this feature is only available for string
values, and the other call_WHATEVER_check_hook() functions just assert
that the GUC_EXTRA_IS_CONTEXT flag is not set. I don't see why you'd
need a complex "extra" value for a bool or int or enum or real-valued
GUC -- how much complex parsing can you need to do on a non-string
value?

I think the call_string_check_hook logic in the v2 patch is
approximately correct. This can be tightened up:

        if (result)
        {
            if (*extra != NULL)
                MemoryContextSetParent(extra_cxt, GUCMemoryContext);
            else
                MemoryContextDelete(extra_cxt);
        }
        else
        {
            MemoryContextDelete(extra_cxt);
        }

You can instead write:

        if (result  != NULL && *extra != NULL)
            MemoryContextSetParent(extra_cxt, GUCMemoryContext);
        else
            MemoryContextDelete(extra_cxt);

> One additional fix: if a check hook succeeds but returns NULL for extra,
> we delete the empty context rather than reparenting it to avoid leaking
> contexts that would never be cleaned up.

Yeah, avoiding leaking contexts seems like one of the key challenges
here. I'm not sure whether we would ever have a check hook that either
returns a null or non-null *extra depending on the situation, but it
seems good to be prepared for that case. I notice that guc_free()
silently accepts a null pointer, so presumably a similar case with a
"flat" GUC extra could exist and work today.

Also, to respond to your later emails, I agree that the new context
shouldn't be created under GUCMemoryContext. As discussed with Tom
earlier, we don't want it to be a long-lived context. I think
CurrentMemoryContext is OK provided that CurrentMemoryContext is
always a child of TopTransactionContext, because even if we leak
something, it will only survive until the end of the transaction at
latest. However, if CurrentMemoryContext can be something like
TopMemoryContext or CacheMemoryContext, then we might want to think a
little harder. I'm not sure whether that's possible -- perhaps you
would like to investigate? Think particularly about GUCs set during
server startup -- maybe in the postmaster, maybe in a backend very
early during initialization. Also maybe configuration reloads while
the backend is idle.

I think we ought to make this patch use MemoryContextSetIdentifier()
to make any leaks easier to debug. If a memory context dump shows that
you've got a whole bunch of contexts floating around, or one really
big one, and they're all just named "GUC extra context" or whatever,
that's going to be pretty unhelpful. If the patch does
MemoryContextSetIdentifier(extra_cxt, conf->name), you'll be able to
see which GUC is responsible.

I think you should port a couple of the core GUCs use this new
mechanism. I suggest specifically check_synchronous_standby_names and
check_synchronized_standby_slots. That should give us some better
insight into how well this mechanism really works and whether it is
more convenient in practice than what we're making check hooks do
today. I thought about proposing that if you do that, you might be
able to just drop this test module, but both of those GUCs are
PGC_SIGHUP, so they wouldn't be good for testing the behavior with
SET, SET LOCAL, RESET, etc. So we might need to either find a case
that can benefit from this mechanism that is PGC_USERSET or PGC_SUSET,
or keep the test module in some form. backtrace_functions is a
possibility, but it's not altogether clear that a non-flat
representation is better in that case, and it doesn't seem great in
terms of being able to write simple tests, either.

--
Robert Haas
EDB: http://www.enterprisedb.com

  16. Re: [PATCH] Allow complex data for GUC extra.

    Bryan Green <dbryan.green@gmail.com> — 2025-12-10T21:06:36Z

    On 12/8/2025 10:48 AM, Robert Haas wrote:
> On Sat, Dec 6, 2025 at 2:08 AM Bryan Green <dbryan.green@gmail.com> wrote:
>>> I think I'm missing something obvious here. call_string_check_hook
>>> doesn't do any memory context management - it just calls the hook.
> 
> No, it does do memory management. It has a PG_TRY()/PG_CATCH() block
> to ensure that we don't forget to GUC_free(*newval) in case of error.
> I was trying to figure out where we were doing the relevant memory
> management today, and then extend that to handle the new thing. But I
> am guilty of fuzzy thinking here, because we're talking about where
> the "extra" memory is managed, not the memory for "newval". So the
> logic we care about is in set_config_with_handle() just as you said:
> 
>                 /* Perhaps we didn't install newextra anywhere */
>                 if (newextra && !extra_field_used(record, newextra))
>                     guc_free(newextra);
> 
> What I hadn't quite internalized previously was that there's no
> PG_TRY/PG_CATCH block here right now because we assume that (1) we
> assume the check hook won't allocate the extra value until it's ready
> to return, so it will never leak a value by allocating it and then
> erroring out and (2) we take care to ensure that no errors can happen
> in the GUC code itself after the extra value has been returned and
> before we either free it or save a pointer to it someplace.
> 
> But having said that, I'm inclined to think that handling the memory
> management concerns inside call_WHATEVER_check_hook() still makes some
> sense. It seems to me that if we do that, set_config_with_handle()
> needs very little change. All it needs to do differently is: wherever
> it would guc_free(newextra), it can call some new helper function that
> will either just guc_free() or alternatively
> MemoryContextDelete(GetMemoryChunkContext()) depending on flags. I
> think this is good, because set_config_with_handle() is already pretty
> complicated, and I'd rather not inject more complexity into that
> function.
> 
> For this to work, each call_WHATEVER_check_hook() function would need
> a PG_TRY()/PG_CATCH() block, rather than only call_string_check_hook()
> as currently. Or alternatively, and I think this might be an appealing
> option, we could say that this feature is only available for string
> values, and the other call_WHATEVER_check_hook() functions just assert
> that the GUC_EXTRA_IS_CONTEXT flag is not set. I don't see why you'd
> need a complex "extra" value for a bool or int or enum or real-valued
> GUC -- how much complex parsing can you need to do on a non-string
> value?

I agree.  I was just thinking there might be edge cases I had not
thought of-- such as using an enum to indicate a sorting algorithm that
has some setup based on which one you choose (maybe locale
characteristics) and the result of that setup is needed in the assign
hook.  That is somewhat of a contrived example and just underscores you
comment about not ever really needing that. I will gladly just add the
asserts and focus on the call_string_check_hook(...).

> 
> I think the call_string_check_hook logic in the v2 patch is
> approximately correct. This can be tightened up:
> 
>         if (result)
>         {
>             if (*extra != NULL)
>                 MemoryContextSetParent(extra_cxt, GUCMemoryContext);
>             else
>                 MemoryContextDelete(extra_cxt);
>         }
>         else
>         {
>             MemoryContextDelete(extra_cxt);
>         }
> 
> You can instead write:
> 
>         if (result  != NULL && *extra != NULL)
>             MemoryContextSetParent(extra_cxt, GUCMemoryContext);
>         else
>             MemoryContextDelete(extra_cxt);
> 

Agreed.

>> One additional fix: if a check hook succeeds but returns NULL for extra,
>> we delete the empty context rather than reparenting it to avoid leaking
>> contexts that would never be cleaned up.
> 
> Yeah, avoiding leaking contexts seems like one of the key challenges
> here. I'm not sure whether we would ever have a check hook that either
> returns a null or non-null *extra depending on the situation, but it
> seems good to be prepared for that case. I notice that guc_free()
> silently accepts a null pointer, so presumably a similar case with a
> "flat" GUC extra could exist and work today.
> 
> Also, to respond to your later emails, I agree that the new context
> shouldn't be created under GUCMemoryContext. As discussed with Tom
> earlier, we don't want it to be a long-lived context. I think
> CurrentMemoryContext is OK provided that CurrentMemoryContext is
> always a child of TopTransactionContext, because even if we leak
> something, it will only survive until the end of the transaction at
> latest. However, if CurrentMemoryContext can be something like
> TopMemoryContext or CacheMemoryContext, then we might want to think a
> little harder. I'm not sure whether that's possible -- perhaps you
> would like to investigate? Think particularly about GUCs set during
> server startup -- maybe in the postmaster, maybe in a backend very
> early during initialization. Also maybe configuration reloads while
> the backend is idle.
> 

Will investigate this.


> I think we ought to make this patch use MemoryContextSetIdentifier()
> to make any leaks easier to debug. If a memory context dump shows that
> you've got a whole bunch of contexts floating around, or one really
> big one, and they're all just named "GUC extra context" or whatever,
> that's going to be pretty unhelpful. If the patch does
> MemoryContextSetIdentifier(extra_cxt, conf->name), you'll be able to
> see which GUC is responsible.
> 

Agreed.

> I think you should port a couple of the core GUCs use this new
> mechanism. I suggest specifically check_synchronous_standby_names and
> check_synchronized_standby_slots. That should give us some better
> insight into how well this mechanism really works and whether it is
> more convenient in practice than what we're making check hooks do
> today. I thought about proposing that if you do that, you might be
> able to just drop this test module, but both of those GUCs are
> PGC_SIGHUP, so they wouldn't be good for testing the behavior with
> SET, SET LOCAL, RESET, etc. So we might need to either find a case
> that can benefit from this mechanism that is PGC_USERSET or PGC_SUSET,
> or keep the test module in some form.

I agree it would be nice to drop the test module.  I will port the ones
you suggested and search for a PGC_USERSET or PGC_SUSET to port.

 backtrace_functions is a
> possibility, but it's not altogether clear that a non-flat
> representation is better in that case, and it doesn't seem great in
> terms of being able to write simple tests, either.
> 
> --
> Robert Haas
> EDB: http://www.enterprisedb.com

Thank you for your continued time and help on this.



-- 
Bryan Green
EDB: https://www.enterprisedb.com

  17. Re: [PATCH] Allow complex data for GUC extra.

    Bryan Green <dbryan.green@gmail.com> — 2025-12-15T06:03:44Z

    On 12/8/2025 10:48 AM, Robert Haas wrote:
> On Sat, Dec 6, 2025 at 2:08 AM Bryan Green <dbryan.green@gmail.com> wrote:
>>> I think I'm missing something obvious here. call_string_check_hook
>>> doesn't do any memory context management - it just calls the hook.
> 
> No, it does do memory management. It has a PG_TRY()/PG_CATCH() block
> to ensure that we don't forget to GUC_free(*newval) in case of error.
> I was trying to figure out where we were doing the relevant memory
> management today, and then extend that to handle the new thing. But I
> am guilty of fuzzy thinking here, because we're talking about where
> the "extra" memory is managed, not the memory for "newval". So the
> logic we care about is in set_config_with_handle() just as you said:
> 
>                 /* Perhaps we didn't install newextra anywhere */
>                 if (newextra && !extra_field_used(record, newextra))
>                     guc_free(newextra);
> 
> What I hadn't quite internalized previously was that there's no
> PG_TRY/PG_CATCH block here right now because we assume that (1) we
> assume the check hook won't allocate the extra value until it's ready
> to return, so it will never leak a value by allocating it and then
> erroring out and (2) we take care to ensure that no errors can happen
> in the GUC code itself after the extra value has been returned and
> before we either free it or save a pointer to it someplace.
> 
> But having said that, I'm inclined to think that handling the memory
> management concerns inside call_WHATEVER_check_hook() still makes some
> sense. It seems to me that if we do that, set_config_with_handle()
> needs very little change. All it needs to do differently is: wherever
> it would guc_free(newextra), it can call some new helper function that
> will either just guc_free() or alternatively
> MemoryContextDelete(GetMemoryChunkContext()) depending on flags. I
> think this is good, because set_config_with_handle() is already pretty
> complicated, and I'd rather not inject more complexity into that
> function.
> 
> For this to work, each call_WHATEVER_check_hook() function would need
> a PG_TRY()/PG_CATCH() block, rather than only call_string_check_hook()
> as currently. Or alternatively, and I think this might be an appealing
> option, we could say that this feature is only available for string
> values, and the other call_WHATEVER_check_hook() functions just assert
> that the GUC_EXTRA_IS_CONTEXT flag is not set. I don't see why you'd
> need a complex "extra" value for a bool or int or enum or real-valued
> GUC -- how much complex parsing can you need to do on a non-string
> value?
> 
> I think the call_string_check_hook logic in the v2 patch is
> approximately correct. This can be tightened up:
> 
>         if (result)
>         {
>             if (*extra != NULL)
>                 MemoryContextSetParent(extra_cxt, GUCMemoryContext);
>             else
>                 MemoryContextDelete(extra_cxt);
>         }
>         else
>         {
>             MemoryContextDelete(extra_cxt);
>         }
> 
> You can instead write:
> 
>         if (result  != NULL && *extra != NULL)
>             MemoryContextSetParent(extra_cxt, GUCMemoryContext);
>         else
>             MemoryContextDelete(extra_cxt);
> 
>> One additional fix: if a check hook succeeds but returns NULL for extra,
>> we delete the empty context rather than reparenting it to avoid leaking
>> contexts that would never be cleaned up.
> 
> Yeah, avoiding leaking contexts seems like one of the key challenges
> here. I'm not sure whether we would ever have a check hook that either
> returns a null or non-null *extra depending on the situation, but it
> seems good to be prepared for that case. I notice that guc_free()
> silently accepts a null pointer, so presumably a similar case with a
> "flat" GUC extra could exist and work today.
> 
> Also, to respond to your later emails, I agree that the new context
> shouldn't be created under GUCMemoryContext. As discussed with Tom
> earlier, we don't want it to be a long-lived context. I think
> CurrentMemoryContext is OK provided that CurrentMemoryContext is
> always a child of TopTransactionContext, because even if we leak
> something, it will only survive until the end of the transaction at
> latest. However, if CurrentMemoryContext can be something like
> TopMemoryContext or CacheMemoryContext, then we might want to think a
> little harder. I'm not sure whether that's possible -- perhaps you
> would like to investigate? Think particularly about GUCs set during
> server startup -- maybe in the postmaster, maybe in a backend very
> early during initialization. Also maybe configuration reloads while
> the backend is idle.
> 
> I think we ought to make this patch use MemoryContextSetIdentifier()
> to make any leaks easier to debug. If a memory context dump shows that
> you've got a whole bunch of contexts floating around, or one really
> big one, and they're all just named "GUC extra context" or whatever,
> that's going to be pretty unhelpful. If the patch does
> MemoryContextSetIdentifier(extra_cxt, conf->name), you'll be able to
> see which GUC is responsible.
> 
> I think you should port a couple of the core GUCs use this new
> mechanism. I suggest specifically check_synchronous_standby_names and
> check_synchronized_standby_slots. That should give us some better
> insight into how well this mechanism really works and whether it is
> more convenient in practice than what we're making check hooks do
> today. I thought about proposing that if you do that, you might be
> able to just drop this test module, but both of those GUCs are
> PGC_SIGHUP, so they wouldn't be good for testing the behavior with
> SET, SET LOCAL, RESET, etc. So we might need to either find a case
> that can benefit from this mechanism that is PGC_USERSET or PGC_SUSET,
> or keep the test module in some form. backtrace_functions is a
> possibility, but it's not altogether clear that a non-flat
> representation is better in that case, and it doesn't seem great in
> terms of being able to write simple tests, either.
> 
> --
> Robert Haas
> EDB: http://www.enterprisedb.com
Robert,

TLDR; There is a possibility of GUCs using contexts that do not reset;
combined with a small window of opportunity this would lead to a leak
given correct timing. We could accept the leak risk, only allow extra
data as a context for PGC_USERSET and PGC_SUSET GUCs, create a new
context that is global but resets, or look for a different solution.

I've incorporated your suggestions and investigated the memory context
safety concerns you raised.

I selected check_temp_tablespaces (PGC_USERSET) for testing SET and SET
LOCAL behavior, and ported check_synchronous_standby_names and
check_synchronized_standby_slots as you suggested. With working examples
in core, I removed the test module from this patch.

I traced through the code paths to determine which memory context is
active when check hooks execute. PGC_POSTMASTER uses PostmasterContext
during postmaster startup. PGC_SIGHUP uses PostmasterContext during
postmaster startup or TopMemoryContext during backend init, though
interestingly it uses MessageContext during actual SIGHUP reload in a
running backend. PGC_BACKEND uses TopMemoryContext during backend init.
PGC_USERSET and PGC_SUSET use MessageContext during interactive SET
commands.

The key distinction is whether these contexts are reset during error
recovery. PostmasterContext and TopMemoryContext are never reset, while
MessageContext is reset at the command loop and TopTransactionContext is
reset during transaction abort.

There's a small window after PG_END_TRY where we've completed the check
hook successfully but haven't yet reparented the context to
GUCMemoryContext or deleted it.

For PGC_USERSET and PGC_SUSET, this isn't a problem. Even if an error
occurs in that window, the context is a child of MessageContext, which
gets reset at the command loop, cleaning up the context automatically.
No leak occurs.

For PGC_POSTMASTER, PGC_SIGHUP (during startup), and PGC_BACKEND, an
error in that window would orphan a context under PostmasterContext or
TopMemoryContext, which are never reset. This would leak memory for the
backend's lifetime. The probability of hitting this timing is extremely
low, perhaps 1 in 100 million, but it's not zero.

We could restrict the feature to PGC_USERSET and PGC_SUSET only with a
simple assertion, which eliminates the leak possibility entirely.
Alternatively, we could create a dedicated GUCCheckContext that's
explicitly reset during error recovery, allowing all GUC contexts to use
the feature. Finally, we could document the limitation and accept the
extremely low probability of a leak for GUCs using contexts that don't
reset-- which seems...unseemly.

The attached patch works as described but would require either
restricting to specific GUC contexts, accepting the leak, or
implementing a more invasive solution. I'd appreciate your thoughts on
the appropriate path forward.

-- 
Bryan Green
EDB: https://www.enterprisedb.com

  18. Re: [PATCH] Allow complex data for GUC extra.

    Robert Haas <robertmhaas@gmail.com> — 2025-12-16T21:12:35Z

    On Mon, Dec 15, 2025 at 1:03 AM Bryan Green <dbryan.green@gmail.com> wrote:
> TLDR; There is a possibility of GUCs using contexts that do not reset;
> combined with a small window of opportunity this would lead to a leak
> given correct timing. We could accept the leak risk, only allow extra
> data as a context for PGC_USERSET and PGC_SUSET GUCs, create a new
> context that is global but resets, or look for a different solution.

I think the last two solutions are best.

One idea is to create a context, maybe a child of GUCMemoryContext,
that is the initial parent for all temporary check-hook contexts. When
the check-hook completes, the context is reparented to
GUCMemoryContext. At some appropriate point in the code, we delete any
leftover children of that context. "Some appropriate point in the
code" could mean "right before we create a new temporary check-hook
context" if it's the case that one check-hook can never be reached
from another check-hook, or it could be elsewhere if reentrancy is a
problem.

Another idea is to just keep track of which contexts need to be
deleted if we error out. Let's say that when we create a temporary
check-hook context, we add it to a linked list, similar to
guc_stack_list, but each element of the list is just a memory context
pointer and the current GUCNestLevel. When AtEOXact_GUC is called,
after doing the stuff it does today, it can iterate over this new list
and filter on nestLevel. Entries that need to get blown away are
subjected to MemoryContextDelete() and also deleted from the list.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  19. Re: [PATCH] Allow complex data for GUC extra.

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-12-16T21:49:44Z

    Robert Haas <robertmhaas@gmail.com> writes:
> Another idea is to just keep track of which contexts need to be
> deleted if we error out. Let's say that when we create a temporary
> check-hook context, we add it to a linked list, similar to
> guc_stack_list, but each element of the list is just a memory context
> pointer and the current GUCNestLevel. When AtEOXact_GUC is called,
> after doing the stuff it does today, it can iterate over this new list
> and filter on nestLevel. Entries that need to get blown away are
> subjected to MemoryContextDelete() and also deleted from the list.

I think something like this could work, but you'd have to be careful
about how you manage the list: you can't just palloc something, nor
lappend it to a list, for fear of OOM right there.  An slist or dlist
removes the lappend hazard, but you still need storage to hold the
list entry.

One way could be to allocate the list entry inside the new
as-yet-transient context, expecting that there's guaranteed to be
enough space in it for that to work.  (If guc.c controls the
initial parameters for the new context, that's a safe assumption.)
The downside to this is that the check_hook could not be allowed
to reset the context ... but it's hard to see why it'd need to.

Alternatively: I don't see any really good reason for a check_hook
to be setting other GUCs, in fact it's probably a seriously bad idea.
(It's a *check* hook, it's not supposed to be causing any
side-effects.)  Therefore, there can be at most one of these
operations in flight at a time, so you don't need any dynamic data
structure.  A simple static variable remembering a not-yet-reparented
context would do it.

			regards, tom lane

  20. Re: [PATCH] Allow complex data for GUC extra.

    Robert Haas <robertmhaas@gmail.com> — 2025-12-17T01:38:01Z

    On Tue, Dec 16, 2025 at 4:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alternatively: I don't see any really good reason for a check_hook
> to be setting other GUCs, in fact it's probably a seriously bad idea.
> (It's a *check* hook, it's not supposed to be causing any
> side-effects.)  Therefore, there can be at most one of these
> operations in flight at a time, so you don't need any dynamic data
> structure.  A simple static variable remembering a not-yet-reparented
> context would do it.

Oh, yeah, I actually wondered if that would be an acceptable
restriction and had it in an earlier version of the email, but it got
lost in the final draft. Maybe with this design you just do something
like:

if (TempCheckHookConteck != NULL)
     MemoryContextReset(TempCheckHookConteck);
else
     TempCheckHookConteck = AllocSetContextCreate(...);

So then if the context survives, you just reset and reuse it, but if
it gets reparented, you set the variable to NULL and create a new
context the next time. Then you don't need any integration with
(sub)transaction abort at all, which seems nice.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  21. Re: [PATCH] Allow complex data for GUC extra.

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-12-17T03:04:53Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Dec 16, 2025 at 4:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ... Therefore, there can be at most one of these
>> operations in flight at a time, so you don't need any dynamic data
>> structure.  A simple static variable remembering a not-yet-reparented
>> context would do it.

> Oh, yeah, I actually wondered if that would be an acceptable
> restriction and had it in an earlier version of the email, but it got
> lost in the final draft. Maybe with this design you just do something
> like:
> if (TempCheckHookConteck != NULL)
>      MemoryContextReset(TempCheckHookConteck);
> else
>      TempCheckHookConteck = AllocSetContextCreate(...);
> So then if the context survives, you just reset and reuse it, but if
> it gets reparented, you set the variable to NULL and create a new
> context the next time. Then you don't need any integration with
> (sub)transaction abort at all, which seems nice.

You could do it like that, but I'd prefer a setup that would give
an assertion failure if someone did try to invoke it recursively.
So I'd opt for allocation like

	Assert(TempCheckHookContext == NULL);
	TempCheckHookContext = AllocSetContextCreate(...);

and then you would need cleanup in AtEOXact_GUC, but that's
hardly complicated.

			regards, tom lane

  22. Re: [PATCH] Allow complex data for GUC extra.

    Bryan Green <dbryan.green@gmail.com> — 2025-12-30T01:05:53Z

    On 12/16/2025 9:04 PM, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Dec 16, 2025 at 4:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> ... Therefore, there can be at most one of these
>>> operations in flight at a time, so you don't need any dynamic data
>>> structure.  A simple static variable remembering a not-yet-reparented
>>> context would do it.
> 
>> Oh, yeah, I actually wondered if that would be an acceptable
>> restriction and had it in an earlier version of the email, but it got
>> lost in the final draft. Maybe with this design you just do something
>> like:
>> if (TempCheckHookConteck != NULL)
>>      MemoryContextReset(TempCheckHookConteck);
>> else
>>      TempCheckHookConteck = AllocSetContextCreate(...);
>> So then if the context survives, you just reset and reuse it, but if
>> it gets reparented, you set the variable to NULL and create a new
>> context the next time. Then you don't need any integration with
>> (sub)transaction abort at all, which seems nice.
> 
> You could do it like that, but I'd prefer a setup that would give
> an assertion failure if someone did try to invoke it recursively.
> So I'd opt for allocation like
> 
> 	Assert(TempCheckHookContext == NULL);
> 	TempCheckHookContext = AllocSetContextCreate(...);
> 
> and then you would need cleanup in AtEOXact_GUC, but that's
> hardly complicated.
> 
> 			regards, tom lane

Robert, Tom,

Following your feedback, I've implemented the static context variable
approach. The attached v3 patch uses a single TempCheckHookContext
that gets created on first use and cleaned up during error recovery.

To catch recursive use, I've added Assert(TempCheckHookContext == NULL)
before creating the context.

One notable behavioral change: check hooks using GUC_EXTRA_IS_CONTEXT
now use palloc() instead of guc_malloc(). The old approach with
guc_malloc() allowed check hooks to return false on OOM, letting the
caller handle it at the appropriate error level. With palloc() an OOM
throws an immediate ERROR. This seemed like an acceptable tradeoff - if
we can't allocate memory for a small temporary context, we're likely
in dire straits anyway. However, this does mean check hooks lose the
ability to gracefully handle OOM by returning false.

Not all code paths flow through AtEOXact_GUC(). To handle orphaned
contexts in these cases, I've added CleanupTempCheckHookContext() as
a public function. AtEOXact_GUC() calls it at the end, and other
contexts can call it as needed to ensure cleanup happens regardless of
the execution path.

I've restricted GUC_EXTRA_IS_CONTEXT to string GUCs only, since that's
where complex parsing actually makes sense. The other check hook types
now assert the flag isn't set.

To demonstrate the mechanism works in practice, I've ported three GUCs:
check_synchronous_standby_names and check_synchronized_standby_slots
(both PGC_SIGHUP), plus check_temp_tablespaces (PGC_USERSET) for testing
SET and SET LOCAL behavior.

-- 
Bryan Green
EDB: https://www.enterprisedb.com

  23. Re: [PATCH] Allow complex data for GUC extra.

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-12-30T01:44:45Z

    Bryan Green <dbryan.green@gmail.com> writes:
> One notable behavioral change: check hooks using GUC_EXTRA_IS_CONTEXT
> now use palloc() instead of guc_malloc(). The old approach with
> guc_malloc() allowed check hooks to return false on OOM, letting the
> caller handle it at the appropriate error level. With palloc() an OOM
> throws an immediate ERROR. This seemed like an acceptable tradeoff

Why?  It seems both inconsistent and unsafe.

			regards, tom lane

  24. Re: [PATCH] Allow complex data for GUC extra.

    Bryan Green <dbryan.green@gmail.com> — 2025-12-30T02:06:30Z

    On 12/29/2025 7:44 PM, Tom Lane wrote:
> Bryan Green <dbryan.green@gmail.com> writes:
>> One notable behavioral change: check hooks using GUC_EXTRA_IS_CONTEXT
>> now use palloc() instead of guc_malloc(). The old approach with
>> guc_malloc() allowed check hooks to return false on OOM, letting the
>> caller handle it at the appropriate error level. With palloc() an OOM
>> throws an immediate ERROR. This seemed like an acceptable tradeoff
> 
> Why?  It seems both inconsistent and unsafe.
> 
> 			regards, tom lane
Fair enough to call me on that.  I mainly thought that if we are having
problems allocating what is usually a few bytes then throwing an error
would have been acceptable.  Based on your comment about unsafe and a
bit deeper thinking I can see where this is probably not a welcome
change in behavior.  I suppose we could catch the error and convert it
to a false return.

-- 
Bryan Green
EDB: https://www.enterprisedb.com

  25. Re: [PATCH] Allow complex data for GUC extra.

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-12-30T02:24:20Z

    Bryan Green <dbryan.green@gmail.com> writes:
> On 12/29/2025 7:44 PM, Tom Lane wrote:
>> Bryan Green <dbryan.green@gmail.com> writes:
>>> One notable behavioral change: check hooks using GUC_EXTRA_IS_CONTEXT
>>> now use palloc() instead of guc_malloc().

>> Why?  It seems both inconsistent and unsafe.

> Fair enough to call me on that.  I mainly thought that if we are having
> problems allocating what is usually a few bytes then throwing an error
> would have been acceptable.

The key reason I'm allergic to this is that throwing elog(ERROR) in
the postmaster process will take down the postmaster.  So we really
do not want code that will execute during SIGHUP configuration
reloads to be doing that.  I grant that there will probably always
be edge cases where that happens, but I'm not okay with building
such a hazard into the GUC APIs.

> Based on your comment about unsafe and a
> bit deeper thinking I can see where this is probably not a welcome
> change in behavior.  I suppose we could catch the error and convert it
> to a false return.

Does

	palloc_extended(..., MCXT_ALLOC_NO_OOM)

help?

			regards, tom lane

  26. Re: [PATCH] Allow complex data for GUC extra.

    Bryan Green <dbryan.green@gmail.com> — 2025-12-30T05:07:58Z

    On 12/29/2025 8:24 PM, Tom Lane wrote:
> Bryan Green <dbryan.green@gmail.com> writes:
>> On 12/29/2025 7:44 PM, Tom Lane wrote:
>>> Bryan Green <dbryan.green@gmail.com> writes:
>>>> One notable behavioral change: check hooks using GUC_EXTRA_IS_CONTEXT
>>>> now use palloc() instead of guc_malloc().
> 
>>> Why?  It seems both inconsistent and unsafe.
> 
>> Fair enough to call me on that.  I mainly thought that if we are having
>> problems allocating what is usually a few bytes then throwing an error
>> would have been acceptable.
> 
> The key reason I'm allergic to this is that throwing elog(ERROR) in
> the postmaster process will take down the postmaster.  So we really
> do not want code that will execute during SIGHUP configuration
> reloads to be doing that.  I grant that there will probably always
> be edge cases where that happens, but I'm not okay with building
> such a hazard into the GUC APIs.
> 
>> Based on your comment about unsafe and a
>> bit deeper thinking I can see where this is probably not a welcome
>> change in behavior.  I suppose we could catch the error and convert it
>> to a false return.
> 
> Does
> 
> 	palloc_extended(..., MCXT_ALLOC_NO_OOM)
> 
> help?
> 
> 			regards, tom lane
I think it does.  We could write a wrapper to make it a bit more obvious
that you should use this instead of palloc for GUC hooks. It could be
modeled after guc_malloc.

void *
guc_palloc(int elevel, size_t size)
{
    void *data = palloc_extended(size, MCXT_ALLOC_NO_OOM);
    if (unlikely(data == NULL))
        ereport(elevel,
                (errcode(ERRCODE_OUT_OF_MEMORY),
                 errmsg("out of memory")));

    return data;
}

....check hook code ....

data = guc_palloc(LOG, sizeof...);
if (data == NULL)
    return false;

....

Given the use case for guc_palloc...should elevel just be LOG with no
option to change?

Thoughts?

-- 
Bryan Green
EDB: https://www.enterprisedb.com