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

On 12/6/2025 1:08 AM, Bryan Green wrote:
> 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.
>
Actually, I realized I still allocated in CurrentMemoryContext-- I think
instead I should just allocate the extra_cxt under GUCMemoryContext and
then there is no need to reparent.

if (conf->flags & GUC_EXTRA_IS_CONTEXT)
{
/* Create directly under GUCMemoryContext - it's already where we want
it */
extra_cxt = AllocSetContextCreate(GUCMemoryContext,
"GUC check_hook extra context",
ALLOCSET_DEFAULT_SIZES);
old_cxt = MemoryContextSwitchTo(extra_cxt);
}

// ...

if (result)
{
/* Already under GUCMemoryContext, just leave it there */
/* Delete if unused */
if (*extra == NULL)
MemoryContextDelete(extra_cxt);
}
else
{
MemoryContextDelete(extra_cxt);
}

--
Bryan Green
EDB: https://www.enterprisedb.com