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
