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
