Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?

From: Andres Freund <andres(at)anarazel(dot)de>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, David Geier <geidav(dot)pg(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Lukas Fittl <lukas(at)fittl(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com>, Maciek Sakrejda <m(dot)sakrejda(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?
Date: 2023-01-21 00:40:32
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On 2023-01-17 10:50:53 -0800, Andres Freund wrote:
> On 2023-01-17 12:26:57 -0500, Tom Lane wrote:
> > > 0001) Add INSTR_TIME_SET_ZERO() calls where otherwise 0002 causes gcc to
> > > warn
> > > Alternatively we can decide to deprecate INSTR_TIME_SET_ZERO() and
> > > just allow to assign 0.
> >
> > I think it's probably wise to keep the macro. If we ever rethink this
> > again, we'll be glad we kept it. Similarly, IS_ZERO is a good idea
> > even if it would work with just compare-to-zero.
> Perhaps an INSTR_TIME_ZERO() that could be assigned in variable definitions
> could give us the best of both worlds?

I tried that in the attached 0005. I found that it reads better if I also add
INSTR_TIME_CURRENT(). If we decide to go for this, I'd roll it into 0001
instead, but I wanted to get agreement on it first.


> > I'm almost tempted to suggest you define instr_time as a struct with a
> > uint64 field, just to help keep us honest about that.
> I can see that making sense. Unless somebody pipes up with opposition to that
> plan soon, I'll see how it goes.

Done in the attached. I think it looks good. Actually found a type confusion
buglet in 0004, so the type safety benefit is noticable.

It does require a new INSTR_TIME_IS_LT() for the loop exit condition in 0004,
but that seems fine.

Besides cosmetic stuff I also added back the cast to double in window's
INSTR_TIME_GET_NANOSEC() - I think there's an overflow danger without it.

We should make this faster by pre-computing
(double) NS_PER_S / GetTimerFrequency()
once, as that'd avoid doing the the slow division on every conversion. But
that's an old issue and thus better tackled separately.


Andres Freund

Attachment Content-Type Size
v8-0001-Zero-initialize-instr_time-uses-causing-compiler-.patch text/x-diff 4.4 KB
v8-0002-instr_time-Represent-time-as-an-int64-on-all-plat.patch text/x-diff 7.2 KB
v8-0003-instr_time-Add-INSTR_TIME_SET_SECONDS-INSTR_TIME_.patch text/x-diff 2.1 KB
v8-0004-wip-report-nanoseconds-in-pg_test_timing.patch text/x-diff 11.5 KB
v8-0005-wip-instr_time-Add-and-use-INSTR_TIME_ZERO-INSTR_.patch text/x-diff 12.2 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Matthias van de Meent 2023-01-21 00:51:28 Re: bug: copy progress reporting of backends which run multiple COPYs
Previous Message Jeff Davis 2023-01-21 00:34:44 Re: Rework of collation code, extensibility