Hi Zsolt,
On Tue, Mar 24, 2026 at 3:59 PM Zsolt Parragi <[email protected]> wrote:
> I like the new approach, but doesn't `EXPLAIN (BUFFERS)` leak some
> memory because the resource owner isn't registered on that path? It
> seems to be visible with pg_log_backend_memory_contexts.
Ah, yes, good catch! Its nice how the separate memory context makes
this very clear now.
I think this was actually an existing problem that only surfaced now.
The issue is that EXPLAIN (BUFFERS) will allocate the per-query
instrumentation, but not actually use it, since the buffer tracking is
only relevant for planning, which has its own instrumentation. I can
fix this locally by adding the following to FreeExecutorState:
/*
* Make sure the instrumentation context gets freed. This usually gets
* re-parented under the per-query context in InstrQueryStopFinalize, but
* that won't happen during EXPLAIN (BUFFERS) since ExecutorFinish never
* gets called, so we would otherwise leak it in TopMemoryContext.
*/
if (estate->es_instrument && estate->es_instrument->instr.need_stack)
MemoryContextDelete(estate->es_instrument->instr_cxt);
I'll include this in the next revision unless I come up with a better
idea. FWIW, I also considered just not setting INSTRUMENT_BUFFERS in
ExplainOnePlan unless ANALYZE is active, but I think there might be
other cases where that doesn't work as expected, so I think the
explicit delete is better.
>
> #define INSTR_BUFUSAGE_ADD(fld,val) do { \
> - pgBufferUsage.fld += val; \
> + instr_stack.current->bufusage.fld += val; \
>
> #define INSTR_WALUSAGE_ADD(fld,val) do { \
> pgWalUsage.fld += val; \
> + instr_stack.current->walusage.fld += val; \
>
> Nitpick, but these could use += (val)
Ack, makes sense - I'll adjust in the next revision.
I'll give it a day or so for further feedback before posting the next update.
Thanks,
Lukas
--
Lukas Fittl
