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

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(dot)green(at)gmail(dot)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