On 12/29/2025 8:24 PM, Tom Lane wrote:
> Bryan Green <[email protected]> writes:
>> On 12/29/2025 7:44 PM, Tom Lane wrote:
>>> Bryan Green <[email protected]> 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
