Hi,
On 2026-04-05 12:38:58 -0700, Lukas Fittl wrote:
> On Sun, Apr 5, 2026 at 11:13 AM Andres Freund <[email protected]> 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
