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

Hi Tomas and Zsolt,

On Mon, Mar 16, 2026 at 4:50 PM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
> On 3/14/26 21:49, Lukas Fittl wrote:
> > 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.
> >
>
> 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.

On Mon, Mar 16, 2026 at 11:21 PM Zsolt Parragi
<zsolt(dot)parragi(at)percona(dot)com> wrote:
>
> + 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.

Good catch, fixed!

> +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?

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/41d87d75-d126-438b-a02e-6671142d39b7@vondra.me

--
Lukas Fittl