Hi, On 2023-01-23 18:23:17 +0100, David Geier wrote: > On 1/21/23 05:14, Andres Freund wrote: > > The elapsed time is already inherently unstable, so we shouldn't have any > > test > > output showing the time. > > > > But I doubt showing it in every explain is a good idea - we use instr_time > > in > > plenty of other places. Why show it in explain, but not in all those other > > places? > > Yeah. I thought it would only be an issue if we showed it unconditionally in > EXPLAIN ANALYZE. If we only show it with TIMING ON, we're likely fine with > pretty much all regression tests.
If we add it, it probably shouldn't depend on TIMING, but on SUMMARY. Regression test queries showing EXPLAIN ANALYZE output all do something like EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF) the SUMMARY OFF gets rid of the "top-level" "Planning Time" and "Execution Time", whereas the TIMING OFF gets rid of the per-node timing. Those are separate options because per-node timing is problematic performance-wise (right now), but whole-query timing rarely is. > But given the different opinions, I'll leave it out in the new patch set for > the moment being. Makes sense. Another, independent, thing worth thinking about: I think we might want to expose both rdtsc and rdtscp. For something like InstrStartNode()/InstrStopNode(), avoiding the "one-way barrier" of rdtscp is quite important to avoid changing the query performance. But for measuring whole-query time, we likely want to measure the actual time. It probably won't matter hugely for the whole query time - the out of order window of modern CPUs is large, but not *that* large - but I don't think we can generally assume that. I'm thinking of something like INSTR_TIME_SET_CURRENT() and INSTR_TIME_SET_CURRENT_FAST() or _NOBARRIER(). Greetings, Andres Freund