Horton
Menu

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

Lukas Fittl <lukas@fittl.com>

View thread
From: Lukas Fittl <lukas@fittl.com>
To: Andres Freund <andres@anarazel.de>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Zsolt Parragi <zsolt.parragi@percona.com>, Tomas Vondra <tomas@vondra.me>, Peter Smith <smithpb2250@gmail.com>
Date: 2026-04-05T19:38:58Z
Lists: pgsql-hackers

Attachments

On Sun, Apr 5, 2026 at 11:13 AM Andres Freund <andres@anarazel.de> wrote:
> Unfortunately I think 0001 on its own doesn't actually work correctly. I
> luckily tried an EXPLAIN ANALYZE with triggers and noticed that the time is
> reported as zeroes.
>
> The only reason I tried is because I misread the diff and though you'd changed
> the calls=%.3f to calls=%d, even though the old state is calls=%.0f...
>
>
> The reason it doesn't work is that explain shows tginstr->instr.total, but
> with the patch the trigger instrumentation just computes
> tginstr->instr.{counter,firsttuple}.

Argh, good catch. That's on me for not manually testing it when I
factored it out.

I've confirmed this works now, both with 0001 only, and with 0001+0002.

> But probably the least bad solution is to add an InstrEndLoop() to in 0001 and
> remove it again in 0002.

Yeah, I've done that for now.

>
> Re 0002
>
>     In passing, drop the "n" argument to InstrAlloc, as all remaining callers
>     need exactly one Instrumentation struct.
>
> I think that probably should be in 0001?

Ack, done.

>
>
> I'm kinda wondering whether, to keep the line lenghts manageable,
> --- a/src/backend/commands/explain.c
> +++ b/src/backend/commands/explain.c
> @@ -1837,7 +1837,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
>      {
>          double      nloops = planstate->instrument->nloops;
>          double      startup_ms = INSTR_TIME_GET_MILLISEC(planstate->instrument->startup) / nloops;
> -        double      total_ms = INSTR_TIME_GET_MILLISEC(planstate->instrument->total) / nloops;
> +        double      total_ms = INSTR_TIME_GET_MILLISEC(planstate->instrument->instr.total) / nloops;
>          double      rows = planstate->instrument->ntuples / nloops;
>
> Should store planstate->instrument in a local var and wrap after =.
>
> But not sure it's worth bothering with.

Sure, seems easy enough.

See attached v14 with changes to 0001 and 0002 only. I've also moved
the PBHS/PIOS patches to their own thread [0].

Thanks,
Lukas

[0]: https://www.postgresql.org/message-id/CAP53PkxRrRKLXECGNFTVOtUusBoWLutZiPfnbejX40ocLuFMQA@mail.gmail.com

-- 
Lukas Fittl

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. instrumentation: Allocate query level instrumentation in ExecutorStart

  2. instrumentation: Move ExecProcNodeInstr to allow inlining

  3. instrumentation: Separate per-node logic from other uses

  4. instrumentation: Separate trigger logic from other uses

  5. instrumentation: Rename INSTR_TIME_LT macro to INSTR_TIME_GT

  6. instrumentation: Keep time fields as instrtime, convert in callers