Subject: Re: Stack-based tracking of per-node WAL/buffer usage

From: Lukas Fittl <[email-masked]>
Date: Thu, 15 Jan 2026 14:34:04 -0800

On Thu, Jan 15, 2026 at 2:06 PM Peter Smith <[email-masked]> wrote:
> I happened to be reading the code in this recent push [1] and saw this
> new macro:
>
> +#define INSTR_TIME_LT(x,y) \
> + ((x).ticks > (y).ticks)
>
> Is that macro name OK? It seemed backwards to me. Shouldn't it be
> called INSTR_TIME_GT because it is checking that x is "Greater Than"
> y?

Oh yeah, good catch. I think I must have thought of "larger than"
instead of "less than".

I think adjusting that to INSTR_TIME_GT makes sense, and is consistent
with how "lt" and "gt" is used elsewhere in the source.

Thanks,
Lukas

--
Lukas Fittl

---

From: Lukas Fittl <[email-masked]>
Date: Mon, 23 Feb 2026 20:18:34 -0800

Hi,

Attached v6:

0001 addresses the issue that Peter raised and corrects the macro name
+ adds a missing comment.

0002/0003 are the same as in v5 and are preparatory commits that
should be in good shape to go.

0004 adds new regression tests that pass on both master and the
patched version, and stress test some of the nested instrumentation
handling.

0005 is what was previously 0003, and introduces the stack-based
instrumentation mechanism. I've made several changes here:

- Rename pgInstrStack => CurrentInstrStack
- Always initialize a top-level stack (TopInstrStack) to avoid
condition when writing to stack
- Rename ExecAccumNodeInstrumentation to
ExecFinalizeNodeInstrumentation, and skip it for parallel workers to
avoid double counting (covered by new test cases). The new
InstrFinalizeNode method that accompanies this now also copies the
per-node instrumentation into the per-query context when in the
regular non-abort case. I think this is necessary when we have a
long-running transaction with auto_explain enabled and many fast
statements, so we don't want to keep NodeInstrumentation until
transaction end (since each NodeInstrumentation is ~288 bytes).
- Accumulate unfinalized per-node stacks on abort (so we can rely on
accumulated totals to be correct) - note that this requires an
additional tree walk in the first ExecutorRun because we lazily
initialize the query->totaltime stack
- Moved the non-node instr stack allocations to TopMemoryContext -
after further testing and review I agree that's necessary because of
utility statements. The specific test case that showed that was
pgss_ProcessUtility and how it behaves with the new utility.sql test
case, as well as VACUUM commands in stream regress.
- Combined parallel worker changes into this commit (it didn't really
make sense to keep these separate based on some of the edge cases)
- Explicit handling of orphaned stack entries when we experience
out-of-order cleanup during abort
- Added a lot more comments (too many?) to explain different edge cases.

0006 could probably be merged into 0005, and is the existing
mechanical change to drop the pgBufferUsage global, but kept separate
for easier review for now.

0007 is the stack-mechanism for splitting out "Table Buffers" for
Index Scans. This has been changed to:

- Report combined index/table buffers as "Buffers" and only show
"Table Buffers" broken out. This was suggested off-list by Andres, and
I've realized this is also otherwise confusing when there are child
nodes below that accumulate up.
- Fix a bug relating to parallel query reporting where previously the
table stack contents would not be copied from parallel workers to the
leader.
- Add documentation and update EXPLAIN ANALYZE examples that reference
Index Scan nodes.

0008 adds a pg_session_buffer_usage() module for testing the global
counters. This helps to verify the handling of aborts behave sanely
(and can run on both master and patched), but I don't think we should
commit this.

---

Two questions on my mind:

1) Should we call this "instrumentation context" (InstrContext?)
instead of "instrumentation stack" (InstrStack)? When writing comments
I found the difference between "stack entry" and "stack" confusing at
times, and "context" feels a bit more clear as a term (e.g. as in
"CurrentInstrContext" or "CurrentInstrumentationContext").

2) For 0007, "Table Buffers" feels a bit inconsistent with "Heap
Fetches" and "Heap Blocks" used elsewhere, should we potentially use
"Heap Buffers", or change the existing names to "Table ..." instead?

And one follow-up on a prior note:

On Tue, Jan 13, 2026 at 12:01 AM Lukas Fittl <[email-masked]> wrote:
> Its also worth noting that 2defd0006255 just added a new
> "instrument_node.h". We could consider moving the per-node
> instrumentation structs and associated functions there (but probably
> keep a single "instrument.c" unit file?). Thoughts?

I've consider this again and think that's not a good idea, because
instrument_node.h seems specifically focused on parallel query - so
I've left all new methods/structs in instrument.h for now.

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Lukas Fittl <[email-masked]>
Date: Sat, 7 Mar 2026 20:27:38 -0800

> + adds a missing comment.

This has been pushed by Andres, thanks again Peter for noting this!

See attached v7, with the changes as noted later. But first, some
fresh performance numbers that take into account extra inlining added
in a new v7/0006:

Example (default shared_buffers, runtimes are best out of 3-ish):

CREATE TABLE lotsarows(key int not null);
INSERT INTO lotsarows SELECT generate_series(1, 50000000);
VACUUM FREEZE lotsarows;

250ms actual runtime (no instrumentation)

BUFFERS OFF, TIMING OFF:
295ms master
295ms with stack-based instrumentation only (v7/0005) -- no change
because BUFFERS OFF
260ms with ExecProcNodeInstr inlining work (v7/0006)

BUFFERS ON, TIMING OFF:
380ms master
305ms with stack-based instrumentation only (v7/0005)
280ms with ExecProcNodeInstr inlining work (v7/0006)

In summary: For BUFFERS ON, we're going from 52% overhead in this
stress test, to 12% overhead (22% without the ExecProcNodeInstr
change). With rows instrumentation only, we go from 18% to 3%
overhead.

> should be in good shape to go.

I split out the trigger instrumentation refactoring into its own
commit (0001) per Andres' request off-list.

0002 is the node instrumentation refactoring, which changed slightly,
because Instrumentation is now treated as a base struct and
NodeInstrumentation now contains an Instrumentation "instr" field to
reduce duplication.

0003 replaces pgBufferUsage/pgWalUsage with INSTR_* macros, pretty
much unchanged from v6/0002.

> patched version, and stress test some of the nested instrumentation

This patch is the same in v7 as in v6.

> instrumentation mechanism.

0005 is still the stack-based instrumentation commit, but has several
improvements over v6:

- Andres suggested a different stack structure in a conversation
off-list, where we track the stack entries in a separately kept array
instead of with inline pointers on the Instrumentation structure -
that ends up making the whole change a lot easier to reason about, and
clarifies the abort handling as well
- I've separated out the ResOwner handling into a new
"QueryInstrumentation" struct - this makes it clearer that one can use
Instrumentation directly with certain caveats (i.e. one must deal with
aborts directly), and feels better now that we're including
Instrumentation in NodeInstrumentation as well

> for easier review for now.

I've merged that together now into 0005 because it feels better to
reason about this as a whole and drop pgBufferUsage within the same
commit where we're adding the stack-based instrumentation.

There is now a new 0006 patch that further optimizes ExecProcNodeInstr
(with performance numbers as noted in the beginning of this mail).
During my testing I realized that the conditional checks in
InstrStartNode/InstrStopNode are quite unnecessary - we never change
what gets instrumented during execution, and so we can create
specialized functions for the different instrumentation combinations,
giving the compiler a much better chance at generating optimal
instructions. The assembly here looks a lot better now.

> Index Scans.

This stayed pretty much the same from the earlier version, but I
reworked this a bit to avoid special Instr functions for dealing with
this case, instead it re-uses NodeInstrumentation and
InstrStartNode/InstrStopNode.

> (and can run on both master and patched), but I don't think we should

This is identical with v6/0008, but continues to prove very useful in
testing the refactorings.

> "CurrentInstrContext" or "CurrentInstrumentationContext").

With the refactoring of the stack-mechanism done now, I think we don't
need to change the naming here, since "InstrStack" as a commonly used
struct is gone now (its just Instrumentation now), so I'd suggest
keeping the "stack" terminology to describe the approach itself.

> "Heap Buffers", or change the existing names to "Table ..." instead?

That is still an open question.

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Lukas Fittl <[email-masked]>
Date: Sat, 7 Mar 2026 20:31:07 -0800

> change). With rows instrumentation only, we go from 18% to 3%

Erm, and I forgot the query here, this is testing "SELECT count(*)
FROM lotsarows;", just like over in [0].

Thanks,
Lukas

[0]: https://www.postgresql.org/message-id/flat/[email-masked]

--
Lukas Fittl

---

From: Zsolt Parragi <[email-masked]>
Date: Mon, 9 Mar 2026 21:55:22 +0000

Hello

+ if (queryDesc->totaltime && estate->es_instrument && !IsParallelWorker())
+ {
+ ExecFinalizeNodeInstrumentation(queryDesc->planstate);
+
+ ExecFinalizeTriggerInstrumentation(estate);
+ }
+
  if (queryDesc->totaltime)
- InstrStop(queryDesc->totaltime);
+ queryDesc->totaltime = InstrQueryStopFinalize(queryDesc->totaltime);

In ExecFinalizeNodeInstrumentation InstrFinalizeNode pfrees the
original instrumentation, but doesn't remove it from the
unfinalized_children list. In normal execution in
InstrQueryStopFinalize ResourceOwnerForgetInstrumentation handles
this, but what about the error path, if something happens between the
two?
Won't we end up in ResOwnerReleaseInstrumentation and do use after
free reads and then a double free on the now invalid pointer?

+ if (myState->es->timing || myState->es->buffers)
+ instr = InstrQueryStopFinalize(instr);
+

Is it okay to leak 1 instrumentation copy per tuple in the query
context? This freshly palloced object will be out of scope a few lines
after this.

@@ -128,8 +130,22 @@ IndexNext(IndexScanState *node)

IndexNextWithReorder doesn't need the same handling?

+ if (scandesc->xs_heap_continue)
+ elog(ERROR, "non-MVCC snapshots are not supported in index-only scans");

Shouldn't this say index scans? (there's another preexisting
indexonylscan mention in this file, but that also seems wrong)

+#if HAVE_INSTR_STACK
+ usage = &instr_top.bufusage;
+#else
+ usage = &pgBufferUsage;
+#endif

I don't see pgBufferUsage anywhere else in the current code, it was
probably removed between rebases?

+ char    *prefix = title ? psprintf("%s ", title) : pstrdup("");
+
+ ExplainPropertyInteger(psprintf("%sShared Hit Blocks", prefix), NULL,
     usage->shared_blks_hit, es);

(And many similar ExplainPropery after this)

title is NULL most of the time, and this results in 16 allocations for
that common case - isn't there a better solution like using
ExplainOpenGroup or something?

+ * Callers must ensure that no intermediate stack entries are skipped, to
+ * handle aborts correctly. If you're thinking of calling this in a PG_FINALLY
+ * block, instead call InstrPopAndFinalizeStack which can skip intermediate
+ * stack entries, or instead use InstrStart/InstrStop.

InstrPopAndFinalizeStack doesn't exists in the latest patch version

---

From: Lukas Fittl <[email-masked]>
Date: Mon, 9 Mar 2026 16:45:55 -0700

Hi Zsolt,

Thanks for reviewing!

> free reads and then a double free on the now invalid pointer?

ResourceOwnerForgetInstrumentation directly follows the call to
ExecFinalizeNodeInstrumentation in standard_ExecutorFinish, so I'm not
sure which error case you're thinking of?

We could explicitly zap the unfinalized_children list at the end of
ExecFinalizeNodeInstrumentation (and have it take a
QueryInstrumentation argument) to protect against this, but I don't
think that makes much of a difference with the code as currently
written.

Maybe I'm misunderstanding which situation you're thinking of?

> context? This freshly palloced object will be out of scope a few lines

I don't think that's a permanent leak, since it would be in the memory
context of the caller, i.e. the per-query memory context, but yeah,
this doesn't seem ideal, since we're keeping this memory around for
each tuple processed.

First of all, we could just do a pfree in serializeAnalyzeReceive
after we added the stats, but that said, this extra instrumentation
for EXPLAIN (SERIALIZE) is a bit of a curious edge case I suppose,
since serializeAnalyzeReceive gets called once per tuple - and so
doing the ResOwner dance for each tuple is probably not ideal.

I wonder if maybe we shouldn't treat this as a case of
NodeInstrumentation instead, and attach to the query's totaltime
instrumentation for abort safety. The only thing that's a bit
inconvenient about that is that calling
InstrQueryRememberNode/InstrFinalizeNode requires passing in that
parent QueryInstrumentation, and the SerializeDestReceiver doesn't
currently have easy access to that. If we brute-force solve that we
could just add an extra method to set it after CreateQueryDesc is
called, but:

Stepping back, from an overall API design perspective, we could also
track the current QueryInstrumentation in a global variable - that'd
make it easier to attach/finalize extra instrumentation on it even if
we don't have direct access to querydesc->totaltime, or we're in a
non-executor context that uses QueryInstrumentation (like some utility
commands) for future use cases of extra sub-query-level
instrumentation. For example, that could also come in handy if we
wanted VACUUM VERBOSE break out the buffer access it does on indexes
vs tables.

> IndexNextWithReorder doesn't need the same handling?

Yes - good point, that's an unintentional omission.

For context, I hadn't spent substantial amounts of time on this part
of the series (rather on getting the design of the stack mechanism
right), but will fix this in the next revision to also handle
IndexNextWithReorder.

I've also been thinking whether we should teach Index-only Scans to
break out the per-table buffer stats for the (rare) heap fetches. I
guess we should? (it'd be fairly easy to instrument that
index_fetch_heap there now)

> indexonylscan mention in this file, but that also seems wrong)

Hmm. Looking at this again, I think the handling of xs_heap_continue
isn't right altogether. Basically it should be this instead, I think,
so we correctly call the table AM's table_index_fetch_tuple again if
call_again gets set:

    while ((tid = index_getnext_tid(scandesc, direction)) != NULL)
    {
        if (node->iss_InstrumentTable)
            InstrPushStack(&node->iss_InstrumentTable->instr);

        for (;;)
        {
            found = index_fetch_heap(scandesc, slot);
            if (found || !scandesc->xs_heap_continue)
                break;
        }

        if (node->iss_InstrumentTable)
            InstrPopStack(&node->iss_InstrumentTable->instr);

        if (unlikely(!found))
            continue;

Which matches the loop in index_getnext_slot (i.e keep calling
index_fetch_heap if xs_heap_continue=true). Based on that, we wouldn't
need the elog at all.

> probably removed between rebases?

That's intentional to allow testing pg_session_buffer_usage both on
master (HAVE_INSTR_STACK=0) and with the patched version
(HAVE_INSTR_STACK=1) - mainly to ensure we're matching behavior for
anyone interested in the top-line buffer or WAL numbers. I don't think
we should commit pg_session_buffer_usage (at least not as-is), its
just there for testing the earlier changes.

> ExplainOpenGroup or something?

Yeah, I guess that's fair - I don't know if the extra allocations
really matter, but I can see your point.

If we used ExplainOpenGroup that would make it a nested structure in
the JSON/etc representation, so it'd look like this:

"Shared Read Blocks:" 123.0,
...
"Temp I/O Write Time:" 42.0,
"Table": {
  "Shared Read Blocks:" 123.0,
  ...
  "Temp I/O Write Time:" 42.0,
}

compared to text representation (simplified):

Buffers: shared read=123.0
I/O Timings: temp write=42.0
Table Buffers: shared read=123.0
Table I/O Timings: temp write=42.0

I think that can work, and we have precedence for using groups in a
node like this, e.g. with "Workers".

If we go with this, I do wonder if we should clarify the JSON/etc
group key as "Table Access" (one group) or "Table Buffers" / "Table
I/O" (two groups), instead of just "Table"?

> InstrPopAndFinalizeStack doesn't exists in the latest patch version

Ah, yeah, that should say InstrStopFinalize (basically direct use of
InstrPushStack/InstrPopStack is discouraged over use of the
Instrumentation functions), I'll fix it in the next revision.

Thanks,
Lukas

--
Lukas Fittl

---

From: Zsolt Parragi <[email-masked]>
Date: Tue, 10 Mar 2026 08:12:56 +0000

> sure which error case you're thinking of?

There are a few pallocs between them, so OOM is possible, even if
unlikely. I mainly mentioned this because even if unlikely it can
happen in theory, and the fix seems simple to me.

> context of the caller, i.e. the per-query memory context

Yes, it's definitely not permanent, but could be bad with many tuples.

> and so

These approaches are interesting, but also add complexity, so I'm
unsure which is better for this, the pfree calls add one line and
solve the main issue with the current code.

> Basically it should be this instead, I think,

Right, this code will be better.

> I don't know if the extra allocations

Yeah, probably doesn't matter that much, but the code also wasn't that
nice in that form. I didn't try to actually modify it, but by just
looking at it the grouped option seemed cleaner to me, and the output
should also be self-explanatory and logical to users.

---

From: Lukas Fittl <[email-masked]>
Date: Sat, 14 Mar 2026 13:49:23 -0700

> happen in theory, and the fix seems simple to me.

Ah, yeah, I didn't consider the pallocs in InstrFinalizeNode causing
an OOM that would cause an abort - good thinking!

I've adjusted this to use a dlist instead of an slist and
InstrFinalizeNode now deletes the node from the list.

> solve the main issue with the current code.

Yeah, fair point on complexity - I've just added the pfree for now.

> Right, this code will be better.

Implemented this fix in IndexNext, and also expanded the tracking of
table access to IndexNextWithReorder.

Regarding Index-Only Scans, I did not add instrumentation for table
access yet - I might add that in a follow-up revision or we could also
do it in a follow-on patch.

> should also be self-explanatory and logical to users.

Yep, fair point - I've now added two groups "Table Buffers" and "Table
I/O Timings" that get used in structured output.

---

See attached v8 rebased on latest master, that also fixes the issues
Zsolt pointed out in 0005 and 0007.

Additionally, two other minor changes in 0005 (the commit that adds
stack-based instrumentation):

1) Change the allocation for query/node instrumentation so that we
only use top-level memory context when WAL/buffer usage is requested
(i.e. instrumentation stack is needed) - easy enough to do, and makes
the timing/rows only case a bit cheaper.

2) Fix a missing addition to the remaining pgWalUsage global in the
parallel query case, when instrumentation is used. For context, to
avoid double counting we don't have the parallel workers call
ExecFinalizeNodeInstrumentation (instead the per-node numbers get
reported back to the leader, and that one bubbles them up), but that
also means that InstrAccumParallelQuery doesn't see the per-node
activity. This now gets added in ExecParallelRetrieveInstrumentation.

All other patches as before.

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Zsolt Parragi <[email-masked]>
Date: Tue, 17 Mar 2026 06:21:18 +0000

+ TriggerInstrumentation *ti = rInfo->ri_TrigInstrument;
+
+ if (ti && (ti->instr.need_bufusage || ti->instr.need_walusage))
+ InstrAccum(instr_stack.current, &ti->instr);

I think there's one more bug here, isn't ti an array? This seems to
only process the first entry, not all of them.

+InstrPopStackTo(Instrumentation *prev)
+{
+ Assert(instr_stack.stack_size > 0);
+ instr_stack.stack_size--;
+ instr_stack.current = prev;
+}
+

Shouldn't this have the same additional assertion as InstrPopStack?

---

From: Lukas Fittl <[email-masked]>
Date: Tue, 17 Mar 2026 01:18:38 -0700

Hi Tomas and Zsolt,

On Mon, Mar 16, 2026 at 4:50 PM Tomas Vondra <[email-masked]> wrote:
> On 3/14/26 21:49, Lukas Fittl wrote:
> I think we should, and it probably should be done in the same commit as
> for plain index scans. Mostly for consistency / less confusion.
>
> Every now and then there's an index-only scan that has to do a lot of
> heap fetches, possibly just as many as the plain index scan. But the IOS
> version would not say how many buffer accesses are for table, and users
> might assume an index-only scan does not access table. Confusing.

Makes sense - added support for index-only scans in the same commit now.

> I only started to look at the patch today, so I don't have any real
> review comments. But I noticed the pg_session_buffer_usage is added only
> to the contrib/meson.build and not to the Makefile. I assume that's not
> intentional.

Yeah, that was missed, thanks!

For context, pg_session_buffer_usage is not necessarily meant to be
committed, its just testing a few edge cases around aborts that can't
be easily tested otherwise.

> only process the first entry, not all of them.

Good catch, fixed!

> Shouldn't this have the same additional assertion as InstrPopStack?

Yep, added. It has to be slightly different because we're passing the
previous stack entry (hence the "To" in the function name), vs the
current one (to save instructions), but probably helpful to have that
assurance.

---

See attached v9, with 0001 to 0004 the same as before.

0005 has the following additional changes beyond what's mentioned above:

- Replaced individual need_bufusage/need_walusage flags on
Instrumentation with a single "need_stack" (to fix duplication of
checks noted by Tomas over in [0])
- Renamed InstrAccum to InstrAccumStack to clarify it only accumulates
stack entries, not total time (which is also part of Instrumentation,
but doesn't use the stack mechanism)
- Fixed a stale comment on InstrPushStack regarding resource owner /
PG_FINALLY use

0006 is the patch moved here now that I shared over in [0] to help
with the IOUsage case. I think up to 0006 would be the required
patches to allow the work in that thread to use the stack-based
instrumentation.

0007 (ExecProcNodeInstr optimization) has minor change to adjust
ExecProcNodeInstr function naming to be less specific to WAL/buffer
usage, in anticipation of potentially adding IOUsage.

0008 (Index scans: Show table buffer accesses) added Index Only Scan
support, and switched IndexScanInstrumentation to use Instrumentation
struct instead of individual Buffer/WALUsage for passing back parallel
worker info - that avoids accidentally missing new types of
stack-based instrumentation being passed back, which would then be
missing from query totals.

Thanks,
Lukas

[0]: https://www.postgresql.org/message-id/[email-masked]

-- 
Lukas Fittl

---

From: Zsolt Parragi <[email-masked]>
Date: Wed, 18 Mar 2026 20:49:37 +0000

+ instr_stack.stack_space *= 2;
+ instr_stack.entries = repalloc_array(instr_stack.entries,
Instrumentation *, instr_stack.stack_space);

Can't this also cause issues with OOM? repalloc_array failing, but we
already doubled stack_space.
The initialization above uses the same order, but that should be safe
as entries is initially NULL.

+ * 2) Accumulate all instrumentation to the currently active instrumentation,
+ *    so that callers get a complete picture of activity, even after an abort
...
+ if (idx >= 0)
+ {
+ while (instr_stack.stack_size > idx + 1)
+ instr_stack.stack_size--;
+
+ InstrPopStack(instr);
+ }

There seems to be one more bug in this:

1. EXPLAIN ANALYZE fires a trigger
2. The trigger function throws ERROR, InstrStopTrigger never runs
3. ResOwnerReleaseInstrumentation runs but only checks
unfinalized_children, not triggers
4. InstrStopFinalize discards the trigger entry
5. Trigger instrumentation information shows 0

---

From: Lukas Fittl <[email-masked]>
Date: Wed, 18 Mar 2026 16:36:03 -0700

> as entries is initially NULL.

Yeah, that's fair, I think its reasonable to do the update of
instr_stack.stack_space after the allocation succeeded. I also think
its good to do that even for the initialization case - it seems
extremely unlikely, but the fact that entries is NULL doesn't protect
us against a subsequent InstrPushStack running after an OOM and the
check whether InstrStackGrow should be called runs based on the stack
space, not the entries array.

I have a fix for that staged for the next iteration.

> 5. Trigger instrumentation information shows 0

Hmm, so I think you're correct that a trigger function error would
cause any stack-based instrumentation from the trigger to get lost.

In practice that doesn't matter today, since triggers never capture
WAL/buffer usage data (only timing), but its maybe a design flaw
because trigger instrumentation is its own thing without a defined
relationship with the stack, unlike NodeInstrumentation which is
registered to the query that handles the aborts.

In a sense this is a similar situation to the EXPLAIN (SERIALIZE)
per-tuple handling we talked about previously - we have
instrumentation that's related to a query, but its not per-node, so
using NodeInstrumentation doesn't really make sense.

I could imagine three ways forward, if we want to address that now (vs
documenting that this isn't handled but effectively not a problem):

1) We add a second list for unfinalized trigger instrumentations on
QueryInstrumentation, and accumulate that too in
ResOwnerReleaseInstrumentation

2) We call InstrStopFinalize in ExecCallTriggerFunc if an error was
thrown from the trigger function (i.e. turn the existing PG_FINALLY
block into a PG_CATCH)

3) We generalize the QueryInstrumentation children handling to allow
other types of Instrumentation (i.e. not just NodeInstrumentation)

I think (3) would be ideal and would let us deal with EXPLAIN
(SERIALIZE) too, but is complicated by the fact that
ResOwnerReleaseInstrumentation needs to have reference to the full
allocated struct (not just the Instrumentation contained within) so it
can call pfree to avoid leaking memory until top transaction end.

In a prior iteration we had the Instrumentation allocated separately
inside NodeInstrumentation (so its a pointer and can thus be freed
independently / replaced with a copy on clean exit), which allows
ResOwnerReleaseInstrumentation to just deal with Instrumentation, but
that becomes inconvenient when dealing with parallel workers.

There is an alternate design I had considered, which is to basically
keep two copies of Instrumentation in NodeInstrumentation: One is a
pointer to the running instrumentation (allocated in a memory context
that survives long enough in an abort, and which will be freed upon
abort or clean exit), and one is a direct member of the containing
struct (like we have it today), and gets updated via a memcpy() upon a
clean exit. I think that'd make the API easier to use and the same
concept could then be applied to TriggerInstrumentation, but the big
downside is that we'd be doubling memory usage because whilst we're
running we'd both have the allocation in the higher memory context,
and the direct member of the containing struct (to return the result
to the caller).

Because of that, I feel like we should do (1) or (2) for now - but
I'll also wait if Andres or others have additional feedback on 0005
before proceeding with further changes.

I also do think that the 0001-0004 patches are good to be committed
unless anyone had additional feedback there.

Thanks,
Lukas

--
Lukas Fittl

---

From: Lukas Fittl <[email-masked]>
Date: Wed, 18 Mar 2026 17:45:49 -0700

On Wed, Mar 18, 2026 at 4:36 PM Lukas Fittl <[email-masked]> wrote:
> WAL/buffer usage data (only timing),

After twisting and turning this in my head more, I realize that's
actually not correct - as it stands, trigger instrumentation is
inheriting the instrumentation options from the overall query, and so
that will cause a typical EXPLAIN (ANALYZE) to also capture Buffer/WAL
usage for triggers - it just won't be shown in EXPLAIN.

Since its not used in practice, we could fix that by explicitly
setting INSTRUMENT_TIMER for triggers, but AFAIR Andres had noted on a
prior iteration that special casing this doesn't seem right, since we
should probably output buffer/WAL usage for triggers anyway.

So I guess that brings us back to, we should fix it with one of the
ways I mentioned. FWIW, I was able to create a test case in the
pg_session_buffer_usage module to that effect, so there is indeed a
current issue where activity during triggers gets lost and won't be
added to the overall totals on abort.

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Heikki Linnakangas <[email-masked]>
Date: Mon, 23 Mar 2026 16:41:18 +0200

> added to the overall totals on abort.

I'm looking at this finalize at resowner part of this patch, and this 
maybe a stupid question, but:

Why does the instrumentation need to be "finalized" on abort? If you run 
EXPLAIN ANALYZE and the query aborts, you don't get to see the stats anyway.

- Heikki

---

From: Zsolt Parragi <[email-masked]>
Date: Mon, 23 Mar 2026 19:07:02 +0000

> EXPLAIN ANALYZE and the query aborts, you don't get to see the stats anyway.

The pg_session_buffer_usage in 0009 makes the information available, I
was able to see the issue with failing triggers with that. Even if
that part doesn't get committed in the end, a 3rd party extension
could still implement the same thing, and notice the missing
statistics. (And maybe it is useful to see some statistics about
failing queries?)

---

From: Lukas Fittl <[email-masked]>
Date: Mon, 23 Mar 2026 13:03:21 -0700

> statistics. (And maybe it is useful to see some statistics about

Right, there are basically two reasons we need the resource owner
logic, or something equivalent:

1) To ensure the active stack entry gets reset correctly to where it
was before, so that we don't corrupt it after an abort (if that's not
done, we'll fail during regular regression tests)

2) To accumulate statistics to parent stack entries that did not
participate in the abort

As Zsolt noted, (2) matters for the pg_session_buffer_usage module,
which is mainly intended to ensure that the logic we're adding keeps a
top-level instrumentation available that includes the activity of
aborted transactions/queries, since we're removing pgBufferUsage. It
also matters for some edge cases in-tree today, e.g. procedures being
tracked in pg_stat_statements. And I do see us being interested in
tracking failed query activities in the future as Zsolt noted.

---

FWIW, on the topic of resource owners and allocations, I've done a
test over the weekend, and here is a question:

It seems we could switch the Instrumentation allocations we're doing
when inside a portal to PortalContext, and CurrentMemoryContext when
outside a portal - instead of allocating in
TopMemoryContext/TopTransactionContext. That works in practice,
because resource owner cleanup happens before PortalContext cleanup,
and simplifies the code a bit since we can skip copying into the
current memory context (because the caller wants to be able to use the
result after the finalize call). And if we leak we'd only leak until
PortalContext gets cleaned up, instead of TopMemoryContext.

To expand on that, in the previously posted v9 we have the following
allocations:

A) InstrStackState allocated under TopMemoryContext (long-lived, never freed)
B) QueryInstrumentation allocated under TopMemoryContext (short-lived
during query execution, explicitly freed up on abort or finalize call)
C) NodeInstrumentation allocated under TopTransactionContext
(short-lived during query execution, explicitly freed up on abort or
finalize call)
D) In other use cases, e.g. ANALYZE command that logs buffer usage,
QueryInstrumentation allocated under TopMemoryContext (short-lived
during command execution, explicitly freed up on abort or finalize
call)

And we could switch it instead to:

A) InstrStackState allocated under TopMemoryContext (long-lived, never freed)
B) QueryInstrumentation allocated under PortalContext (short-lived
during query execution, *automatically* freed up on abort, manually on
ExecutorEnd to avoid waiting for holdable cursors to free
PortalContext)
C) NodeInstrumentation allocated under PortalContext (short-lived
during query execution, *automatically* freed up on abort, manually on
ExecutorEnd to avoid waiting for holdable cursors to free
PortalContext)
D) In other use cases, e.g. ANALYZE command that logs buffer usage,
QueryInstrumentation allocated under CurrentMemoryContext (short-lived
during command execution, *automatically* freed up on abort and
success case)

However, this goes against the principle noted by Heikki over in [0]
that ResOwners should use TopMemoryContext to avoid relying on the
ordering of clean up operations.

Thoughts?

Thanks,
Lukas

[0]: https://www.postgresql.org/message-id/flat/a3197b31-f40d-4164-872d-906d8e9b374a%40iki.fi#526984c1be0662a7526bcfe0b358564b

--
Lukas Fittl

---

From: Lukas Fittl <[email-masked]>
Date: Mon, 23 Mar 2026 23:03:16 -0700

> ordering of clean up operations.

I've pondered this question more today, and I think maybe this
complexity isn't the right way to approach this.

Instead I've tried introducing a memory context for instrumentation
managed as a resource owner, and I am now (for now) convinced that
this is the right trade-off for the problem at hand.

The benefit of using our own memory context is that we can free it all
at once (which is a lot less brittle when different types of
instrumentation are involved), *and* we can re-assign the context
parent to be that of the current context on finalize, cleanly moving
it out of TopMemoryContext without doing a copy. It also makes it
easier for callers to allocate in the right context, without having to
introduce a bunch more "Alloc" methods (e.g. relevant for the table
stack tracking for index scans). We also have precedence for the use
of small memory contexts in the executor with the existence of
per-tuple memory contexts.

The main downside is that for the cases where we don't have child
instrumentation, but want the resource owner logic (e.g. ANALYZE
command, or regular query execution with pg_stat_statements enabled),
we have more memory overhead: 1kB (ALLOCSET_SMALL_SIZES minimum) for
what could otherwise be ~200B. I think that's probably okay for
current use cases, but we could avoid that by only using the separate
contexts when we have child instrumentations that will be tracked.

See attached v10, rebased, with these additional changes:

In 0001/0002 I've added forward declarations in execnodes.h, which are
necessary since fba4233c8328.

In 0005 (stack-based instrumentation) I've also addressed the
previously raised concerns about trigger and EXPLAIN (SERIALIZE)
handling, and it now treats both kinds as children of the query's
instrumentation context. To assist with initializing that, we have to
add a query instrumentation reference to EState, but I think that's
acceptable. To reduce code churn I've repurposed the existing
es_instrument field for that, and we now remember the instrumentation
options on QueryInstrumentation.

In 0007 (Optimize ExecProcNodeInstr instructions by inlining) I've
adjusted the ExecProcNodeInstr logic to use a single function that
contains the logic, with separate callers that pass in fixed
constants, to let the compiler figure out the different variants with
less code duplication, per an off-list suggestion from Andres.

In 0008 (Index scans: Show table buffer accesses) this now utilizes
the fact that f026fbf059f2 made IndexScanInstrumentation a heap
allocation, and puts that allocation in the instrumentation memory
context, so it can participate directly in the stack with an inlined
Instrumentation field to track table access, avoiding a duplicate
field previously necessary.

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Zsolt Parragi <[email-masked]>
Date: Tue, 24 Mar 2026 22:59:45 +0000

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.

 #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)

---

From: Lukas Fittl <[email-masked]>
Date: Tue, 24 Mar 2026 22:34:29 -0700

Hi Zsolt,

> 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.

> 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: Heikki Linnakangas <[email-masked]>
Date: Wed, 25 Mar 2026 12:47:01 +0200

> this is the right trade-off for the problem at hand.

Yes, that seems better.

This patch could use an overview README file, I'm struggling to 
understand how the this all works. Here's my understanding so far, 
please correct me if I'm wrong:

There are *two* data structures tracking the Instrumentation nodes. The 
patch only talks about a stack, but I think there's also implicitly a 
tree in there.

Tree
----

All Instrumentation nodes are part of a tree. For example, if you have 
two portals open, the tree might look like this:

Session - Query A - NestLoop - Seq Scan A
                              - Seq Scan B

         - Query B - Seq Scan C

When a node is "finalized", its counters are added to its parent.

This tree is a somewhat implicit in the patch. Each QueryInstrumentation 
has a list of child nodes, but only unfinalized ones. Don't we need that 
at the session level too? When a Query is released on abort, its 
counters need to be added to the parent too. If I understand correctly, 
the patch tries to use the stack for that, but it's confusing.

I think it would make the patch more clear to talk explicitly about the 
tree, and represent it explicitly in the Instrumentation nodes. I.e. add 
a "parent" pointer, or a "children" list, or both to the Instrumentation 
struct.

Stack
-----

At all times, there's a stack that tracks what is the Instrumentation in 
the tree that is *currently* executing. For example, while executing the 
Seq Scan B, the stack would look like this:

0: Session
1: Query A
2: NestLoop
3: Seq Scan B

And when the code is sending a result row back to the client, while the 
query is being executed, the stack would be just:

0: Session

In the patch, the stack is represented by an array. It could also be 
implemented with a CurrentInstrumentation global variable, similar to 
CurrentMemoryContext and CurrentResourceOwner.

Abort handling
--------------

On abort, two things need to happen:

1. Reset the stack to the appropriate level. This ensures that any we 
don't later try to update the counters on an Instrumentation nodes that 
is going away with the abort. In the above example, the stack would be 
reset to the 0: Session level.

2. Finalize all the Instrumentation nodes as part of the ResourceOwner 
cleanup. All Instrumentation nodes that are released roll up their 
counters to their parents.

Questions:

Is the stack always a path from the root of the tree, down to some node? 
Or could you have e.g. recursion like A -> B -> C -> A? (I don't know if 
it makes a difference, just wondering)

What happens if you release e.g. the NestLoop before its children? All 
the Instrumentation nodes belonging to a query would usually be part of 
the same ResourceOwner and there's no guarantee on what order the 
resources are released.

- Heikki

---

From: Lukas Fittl <[email-masked]>
Date: Wed, 25 Mar 2026 17:41:00 -0700

Hi Heikki,

Thanks for reviewing!

> please correct me if I'm wrong:

Sure, happy to put this together - I wonder where would place that
best - probably src/backend/executor/README.instrument ?

> the patch tries to use the stack for that, but it's confusing.

If I follow you correctly, we're talking about the work that
InstrStopFinalize is doing (both in regular flow, and in abort):

void
InstrStopFinalize(Instrumentation *instr)
{
    ...

    InstrAccumStack(instr_stack.current, instr);
}

The "instr_stack.current" global referenced here is effectively the
instrumentation that was active before InstrStart was called, and
would be a parent in the tree in that sense.

Its worth noting that on abort we don't care about the tree structure
below the aborted activity, i.e. each QueryInstrumentation acts as a
finalization point of sorts, with any tree structure below (e.g. that
of executor nodes) not being finalized to their respective parents,
but instead just getting added to the QueryInstrumentation they were
attached to (which then gets finalized into "instr_stack.current").

> a "parent" pointer, or a "children" list, or both to the Instrumentation

I'm happy to clarify the mechanism, but I'm hesitant to add more
pointers to Instrumentation, since its a base struct that gets re-used
in different places, and also gets copied to parallel workers (so any
pointer requires extra scrutiny to avoid mis-use) - and I don't think
we actually need to track the parent pointer, since in practice it
will always be the current stack entry.

> CurrentMemoryContext and CurrentResourceOwner.

It could be, and in fact earlier iterations were closer to that, but I
modified that to the current version based on Andres' feedback - The
array structure is a lot easier to work with during abort when things
execute out-of-order (as you also note later).

> Abort handling

Correct, but just to clarify, the main problem we deal with in terms
of reset is making sure that we cancel out any InstrPushStack that was
done, but will now no longer have a matching InstrPopStack getting
called.

We need to make sure that we get to the stack entry that was active
before the aborted activity started.

> it makes a difference, just wondering)

I don't think this happens in practice - I think for the stack itself
that'd probably be fine (since you're just putting the entry back on
that was on before, in a sense), but it'd e.g. result in timer
instrumentation behaving incorrectly.

We could probably add an assert to explicitly prevent that if we're
worried about it, but the existing Start/Stop instrumentation calls
haven't seen this issue I think, and they'd already have had a problem
with that.

> the same ResourceOwner and there's no guarantee on what order the

Correct, and in fact prior versions of the patch struggled with that
exact problem. Its both an issue for resource owner managed cleanup,
and when you have PG_FINALLY in the picture (e.g. pg_stat_statements).

But that's exactly why its not really a full tree - in the abort case
we do not care about the relationship of child instrumentations
underneath the QueryInstrumentation - we just make sure that the stack
is reset to the entry that was active before the QueryInstrumentation
started, and that all activity that occurred is added to the
QueryInstrumentation.

If you had a situation that had two QueryInstrumentations active (i.e.
both registered as resource owner), we go up to whichever one of the
two is higher up in the stack, per logic in InstrStopFinalize.

Thanks for thinking this through & hopefully this clarifies things a bit?

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Lukas Fittl <[email-masked]>
Date: Fri, 27 Mar 2026 00:21:23 -0700

> best - probably src/backend/executor/README.instrument ?

I've gone ahead and added that in
src/backend/executor/README.instrument for now, trying to take some of
your prior email as inspiration, whilst not fully committing to
describing it as a tree - but happy to revise that if we feel its
important for clarity.

See attached v11, rebased after the pgBufferUsage calls were moved in
df09452c3209, and also fixing the issues that Zsolt noted in a
previous email reviewing v10.

I've also moved pg_session_buffer_usage to be a
test_session_buffer_usage module instead, since its not intended as a
user accessible module. If we wanted to commit that (not sure if its
worth the cycles), we could potentially merge it with the 0004 commit
that expands the main regression tests.

Thanks,
Lukas

-- 
Lukas Fittl