From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Robert Haas <robertmhaas@gmail.com>, David Geier <geidav.pg@gmail.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>
Subject: Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?
In-reply-to: <20230117164758.gx4uuzhk5grw7zea@awork3.anarazel.de>
References: <Y0Z75kh0WNcb5u3j@paquier.xyz>
 <d7f94549-9179-b63c-878a-002a7cb586a6@gmail.com>
 <d6e84dc8-75d1-4c3b-4b32-c4fcb7852275@gmail.com>
 <3eaeaa3a-b78e-ef0d-7319-5d713bbc09a0@gmail.com>
 <CAP53PkwtWtY-hkSwV7E6g_n657RnFcK0asSp0foSk5Qz_CCJXQ@mail.gmail.com>
 <cc72a411-aa07-834c-85c0-489a5924f8bc@gmail.com>
 <CALDaNm080KHmRHo8OPcAEj+vNzXejwHgmddji5hkAdCwNsuqKA@mail.gmail.com>
 <b201ac3c-1bff-2414-be8e-fc287f78be1a@gmail.com>
 <20230113195547.k4nlrmawpijqwlsa@awork3.anarazel.de>
 <CA+TgmoYOvh=k-H9m21Lh-SWbn7TNurm3JoOVxW+kOO=Gn1_8Xw@mail.gmail.com>
 <20230117164758.gx4uuzhk5grw7zea@awork3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Tue, 17 Jan 2023 08:47:58 -0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3044620.1673976417.1@sss.pgh.pa.us>
Date: Tue, 17 Jan 2023 12:26:57 -0500
Message-ID: <3044621.1673976417@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3044621.1673976417%40sss.pgh.pa.us>
Precedence: bulk

Andres Freund <andres@anarazel.de> writes:
> Here's an updated version of the move to representing instr_time as
> nanoseconds. It's now split into a few patches:

I took a quick look through this.

> 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.  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.

> 0003) Add INSTR_TIME_SET_SECOND()
>       This is used in 0004. Just allows setting an instr_time to a time in
>       seconds, allowing for a cheaper loop exit condition in 0004.

Code and comments are inconsistent about whether it's SET_SECOND or
SET_SECONDS.  I think I prefer the latter, but don't care that much.

> 0004) report nanoseconds in pg_test_timing

Didn't examine 0004 in any detail, but the others look good to go
other than these nits.

			regards, tom lane