[PATCH] Allow complex data for GUC extra.
Following up on Tom Lane's suggestion to use MemoryContexts for GUC
extra data [1]https://discord.com/channels/1258108670710124574/1402360503036285130, 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
Attachments:
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
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 extraAn 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
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]/messages/by-id/2ff46ac9-b46c-4210-8f0c-0f5365b36db9@eisentraut.org.
regards, tom lane
[1]: /messages/by-id/2ff46ac9-b46c-4210-8f0c-0f5365b36db9@eisentraut.org
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
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
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
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.
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] /messages/by-id/2ff46ac9-b46c-4210-8f0c-0f5365b36db9@eisentraut.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
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
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
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
Attachments:
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
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
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
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