| From: | Lukas Fittl <lukas(at)fittl(dot)com> |
|---|---|
| To: | Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> |
| Cc: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Andres Freund <andres(at)anarazel(dot)de>, Tomas Vondra <tomas(at)vondra(dot)me>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Peter Smith <smithpb2250(at)gmail(dot)com> |
| Subject: | Re: Stack-based tracking of per-node WAL/buffer usage |
| Date: | 2026-03-25 05:34:29 |
| Message-ID: | CAP53PkwTxo5R8_x76PmixXBV2YBGGf+7miRP28Asr7u-CLhQfA@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Zsolt,
On Tue, Mar 24, 2026 at 3:59 PM Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> 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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | John Naylor | 2026-03-25 05:38:58 | Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc? |
| Previous Message | Michael Paquier | 2026-03-25 05:25:01 | Re: Adding locks statistics |