Feedback-ID: id4a34324:Fastmail
Date: Sun, 5 Apr 2026 14:13:51 -0400
From: Andres Freund <andres@anarazel.de>
To: Lukas Fittl <lukas@fittl.com>
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>
Subject: Re: Stack-based tracking of per-node WAL/buffer usage
Message-ID: <mtjyijvuv7xavvcdy3rosg43ycy3t5sluioxkef46se25ajtxp@e3e2xvesqhzu>
References: <da7ad516-37d1-47e5-afae-26a7ef7b60f8@iki.fi>
 <CAN4CZFOUTHdXuEAwrST8ueDxGJRe69zVgeVAY0osTjVkRZi_Lw@mail.gmail.com>
 <CAP53Pkx+HZ2OSGDHmNgSDisXdjGp4KFygaX+GaQcFEuXHRXg=g@mail.gmail.com>
 <CAP53PkznofNg+ii363QQGoje30nhssuSz_hV5U4YANAt-Yr_Yg@mail.gmail.com>
 <a1edb578-8a54-4f7a-ad74-11ce9cef291a@iki.fi>
 <CAP53PkwCk7X_ryOak_3x6ek2f+4kCgGjWe8aVy39D59Q8y9wTg@mail.gmail.com>
 <CAP53Pky2=31B1AS9vg=Ca9308_hb0H4g968cSKxiFgT0moJfYg@mail.gmail.com>
 <CAP53PkybGzJTAo7V07ssUae5047pBY3jc_F07tGY13cNhqe+AQ@mail.gmail.com>
 <57biou6l65r7gr4nunoe6lignz2x6m3w45gihoypaez4pc46di@txj3bakhj66l>
 <CAP53PkxDupm5U6TV0LF_YWHQ2wfcSiLsgnDBcO6b5AchLJhp=A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAP53PkxDupm5U6TV0LF_YWHQ2wfcSiLsgnDBcO6b5AchLJhp=A@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/mtjyijvuv7xavvcdy3rosg43ycy3t5sluioxkef46se25ajtxp%40e3e2xvesqhzu>
Precedence: bulk

Hi,

Not a real reply to your email, just looking at committing 0001/0002 to get
them out of the way.


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

And of course we don't have any tests even showing trigger output. Not that
such a test would have been likely to catch this issue, as something like the
the amount of time is nontrivial to test.


This is actually fixed by 0002, as it makes InstrStop() update ->total,
rather than ->counter as before.


But I'd prefer not to break the intermediary state ;).

I guess we could squash both patches?

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


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?


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.


Greetings,

Andres Freund