Thread

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

Bryan Green <dbryan.green@gmail.com> — 2025-12-30T01:05:53Z

On 12/16/2025 9:04 PM, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Dec 16, 2025 at 4:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> ... Therefore, there can be at most one of these
>>> operations in flight at a time, so you don't need any dynamic data
>>> structure.  A simple static variable remembering a not-yet-reparented
>>> context would do it.
> 
>> Oh, yeah, I actually wondered if that would be an acceptable
>> restriction and had it in an earlier version of the email, but it got
>> lost in the final draft. Maybe with this design you just do something
>> like:
>> if (TempCheckHookConteck != NULL)
>>      MemoryContextReset(TempCheckHookConteck);
>> else
>>      TempCheckHookConteck = AllocSetContextCreate(...);
>> So then if the context survives, you just reset and reuse it, but if
>> it gets reparented, you set the variable to NULL and create a new
>> context the next time. Then you don't need any integration with
>> (sub)transaction abort at all, which seems nice.
> 
> You could do it like that, but I'd prefer a setup that would give
> an assertion failure if someone did try to invoke it recursively.
> So I'd opt for allocation like
> 
> 	Assert(TempCheckHookContext == NULL);
> 	TempCheckHookContext = AllocSetContextCreate(...);
> 
> and then you would need cleanup in AtEOXact_GUC, but that's
> hardly complicated.
> 
> 			regards, tom lane

Robert, Tom,

Following your feedback, I've implemented the static context variable
approach. The attached v3 patch uses a single TempCheckHookContext
that gets created on first use and cleaned up during error recovery.

To catch recursive use, I've added Assert(TempCheckHookContext == NULL)
before creating the context.

One notable behavioral change: check hooks using GUC_EXTRA_IS_CONTEXT
now use palloc() instead of guc_malloc(). The old approach with
guc_malloc() allowed check hooks to return false on OOM, letting the
caller handle it at the appropriate error level. With palloc() an OOM
throws an immediate ERROR. This seemed like an acceptable tradeoff - if
we can't allocate memory for a small temporary context, we're likely
in dire straits anyway. However, this does mean check hooks lose the
ability to gracefully handle OOM by returning false.

Not all code paths flow through AtEOXact_GUC(). To handle orphaned
contexts in these cases, I've added CleanupTempCheckHookContext() as
a public function. AtEOXact_GUC() calls it at the end, and other
contexts can call it as needed to ensure cleanup happens regardless of
the execution path.

I've restricted GUC_EXTRA_IS_CONTEXT to string GUCs only, since that's
where complex parsing actually makes sense. The other check hook types
now assert the flag isn't set.

To demonstrate the mechanism works in practice, I've ported three GUCs:
check_synchronous_standby_names and check_synchronized_standby_slots
(both PGC_SIGHUP), plus check_temp_tablespaces (PGC_USERSET) for testing
SET and SET LOCAL behavior.

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