Thread

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

Bryan Green <dbryan.green@gmail.com> — 2025-12-30T05:07:58Z

On 12/29/2025 8:24 PM, Tom Lane wrote:
> Bryan Green <dbryan.green@gmail.com> writes:
>> On 12/29/2025 7:44 PM, Tom Lane wrote:
>>> Bryan Green <dbryan.green@gmail.com> writes:
>>>> One notable behavioral change: check hooks using GUC_EXTRA_IS_CONTEXT
>>>> now use palloc() instead of guc_malloc().
> 
>>> Why?  It seems both inconsistent and unsafe.
> 
>> Fair enough to call me on that.  I mainly thought that if we are having
>> problems allocating what is usually a few bytes then throwing an error
>> would have been acceptable.
> 
> The key reason I'm allergic to this is that throwing elog(ERROR) in
> the postmaster process will take down the postmaster.  So we really
> do not want code that will execute during SIGHUP configuration
> reloads to be doing that.  I grant that there will probably always
> be edge cases where that happens, but I'm not okay with building
> such a hazard into the GUC APIs.
> 
>> Based on your comment about unsafe and a
>> bit deeper thinking I can see where this is probably not a welcome
>> change in behavior.  I suppose we could catch the error and convert it
>> to a false return.
> 
> Does
> 
> 	palloc_extended(..., MCXT_ALLOC_NO_OOM)
> 
> help?
> 
> 			regards, tom lane
I think it does.  We could write a wrapper to make it a bit more obvious
that you should use this instead of palloc for GUC hooks. It could be
modeled after guc_malloc.

void *
guc_palloc(int elevel, size_t size)
{
    void *data = palloc_extended(size, MCXT_ALLOC_NO_OOM);
    if (unlikely(data == NULL))
        ereport(elevel,
                (errcode(ERRCODE_OUT_OF_MEMORY),
                 errmsg("out of memory")));

    return data;
}

....check hook code ....

data = guc_palloc(LOG, sizeof...);
if (data == NULL)
    return false;

....

Given the use case for guc_palloc...should elevel just be LOG with no
option to change?

Thoughts?

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