On 12/8/2025 9:23 AM, Bryan Green wrote:
> On 12/6/2025 1:08 AM, Bryan Green wrote:
...
> 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);
> }
>
Apologies for the unneeded email above. Upon more reflection, I need to
walk back my previous change from CurrentMemoryContext to
GUCMemoryContext. I think CurrentMemoryContext was correct.
The issue is the ERROR path. If a check hook throws ERROR, we longjmp
out without hitting the cleanup code, leaving extra_cxt orphaned under
whatever parent we gave it.
With CurrentMemoryContext as parent (typically MessageContext or
similar), error recovery resets those contexts, and MemoryContextReset()
deletes all children via MemoryContextDeleteChildren(). So the orphaned
context gets cleaned up automatically.
With GUCMemoryContext as parent, it never gets reset during normal error
recovery, so the orphaned context just sits there leaking memory.
So the original code was actually relying on PostgreSQL's error recovery
to handle the ERROR case, which is the right approach here.
Hopefully my understanding of this is correct.
The last attached patches are still the correct ones.
--
Bryan Green
EDB: https://www.enterprisedb.com
