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

On 11/18/2025 11:24 AM, Robert Haas wrote:
> On Mon, Nov 17, 2025 at 4:17 PM Bryan Green <dbryan(dot)green(at)gmail(dot)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