Feedback-ID: id4a34324:Fastmail
Date: Sun, 5 Apr 2026 17:02:28 -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: <pgvy7lsoa5jldwgyay2g757xivzbmgyan547wealc7cwtduvra@dysyw6dtqdgs>
References: 
 <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>
 <mtjyijvuv7xavvcdy3rosg43ycy3t5sluioxkef46se25ajtxp@e3e2xvesqhzu>
 <CAP53Pky4zHCueAyPkWE-cH6SqE_m+Kppmy=zE5K36X3HvCFZLw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAP53Pky4zHCueAyPkWE-cH6SqE_m+Kppmy=zE5K36X3HvCFZLw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/pgvy7lsoa5jldwgyay2g757xivzbmgyan547wealc7cwtduvra%40dysyw6dtqdgs>
Precedence: bulk

Hi,

On 2026-04-05 12:38:58 -0700, Lukas Fittl wrote:
> 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.

I made 'firings' an an int64, rather than int. Could have made it unsigned,
but ExplainPropertyInteger accepts an int64...

Because the patch did change those lines anyway, I replaced
palloc0(sizeof(Instrumentation)) with palloc_object(), and
palloc0(n * sizeof(TriggerInstrumentation)) with palloc_array().

It also seemed silly to have an if around the assingments of need_*:

	if (instrument_options & (INSTRUMENT_BUFFERS | INSTRUMENT_TIMER | INSTRUMENT_WAL))
	{
		instr->need_bufusage = (instrument_options & INSTRUMENT_BUFFERS) != 0;
		instr->need_walusage = (instrument_options & INSTRUMENT_WAL) != 0;
		instr->need_timer = (instrument_options & INSTRUMENT_TIMER) != 0;
		instr->async_mode = async_mode;

but that gets cleared up in 0002 anyway.

But it did lead me to notice a pre-existing bug: We only set async_mode in the
   if (INSTRUMENT_BUFFERS | INSTRUMENT_TIMER | INSTRUMENT_WAL)
branch.


It looks like that doesn't matter today, because all async_mode is used for is
		/*
		 * In async mode, if the plan node hadn't emitted any tuples before,
		 * this might be the first tuple
		 */
		if (instr->async_mode && save_tuplecount < 1.0)
			instr->firsttuple = instr->counter;
and without INSTRUMENT_TIMER instr->counter would be zero anyway.

But I guess it's worth noting that in the commit message for 0002?


I felt a bit silly leaving the instr->need_* stuff in InstrAlloc(), when there
is InstrInit() directly afterwards that does the same thing, but then that
leads to removing the redundant memset etc, so I left it for 0002.


With that I pushed 0001.

Greetings,

Andres Freund