Thread

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

Bryan Green <dbryan.green@gmail.com> — 2025-12-08T15:23:00Z

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.green@gmail.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