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

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

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