On 12/8/2025 10:48 AM, Robert Haas wrote:
> On Sat, Dec 6, 2025 at 2:08 AM Bryan Green <[email protected]> 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
