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

On Sat, Dec 6, 2025 at 2:08 AM Bryan Green <dbryan(dot)green(at)gmail(dot)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