Horton

Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?

Andres Freund <andres@anarazel.de>

View thread
From: Andres Freund <andres@anarazel.de>
To: David Geier <geidav.pg@gmail.com>
Cc: Pavel Stehule <pavel.stehule@gmail.com>, Tomas Vondra <tomas.vondra@enterprisedb.com>, vignesh C <vignesh21@gmail.com>, Lukas Fittl <lukas@fittl.com>, Michael Paquier <michael@paquier.xyz>, Ibrar Ahmed <ibrar.ahmad@gmail.com>, Maciek Sakrejda <m.sakrejda@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2023-01-23T17:41:58Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. pg_test_timing: Also test RDTSC[P] timing, report time source, TSC frequency

  2. Allow retrieving x86 TSC frequency/flags from CPUID

  3. instrumentation: Standardize ticks to nanosecond conversion method

  4. instrumentation: Use Time-Stamp Counter on x86-64 to lower overhead

  5. Zero initialize uses of instr_time about to trigger compiler warnings

  6. instr_time: Represent time as an int64 on all platforms

  7. Add 250c8ee07ed to git-blame-ignore-revs

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