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

On 11/18/2025 12:21 PM, Tom Lane wrote:
> Bryan Green <dbryan(dot)green(at)gmail(dot)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