From: David Geier <geidav.pg@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Ibrar Ahmed <ibrar.ahmad@gmail.com>
Cc: Maciek Sakrejda <m.sakrejda@gmail.com>
Cc: Lukas Fittl <lukas@fittl.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?
Date: Wed, 18 Jan 2023 14:02:48 +0100
Message-ID: <d07fca50-d6bd-f60c-c440-a480f8156c92@gmail.com> (raw)
In-Reply-To: <20230116173723.exeijemoytfo2yc2@awork3.anarazel.de>
References: <20200612232810.f46nbqkdhbutzqdg@alap3.anarazel.de>
<CAP53Pky+4MJfdC-R3HQFPRauy2N+5_7ExpEhCFUo8EU3bCPjEg@mail.gmail.com>
<20220701172639.ty4iu5almspueriu@alap3.anarazel.de>
<CAOtHd0BK98KE9_AjcX0ju-zihKAJwKAHabLpATauX3RgF5TwhA@mail.gmail.com>
<CALtqXTc1MjgjNU0hTh471wCvaxFaD2yby-TZ4a4DYpHO_tKLPA@mail.gmail.com>
<Y0Z75kh0WNcb5u3j@paquier.xyz>
<d7f94549-9179-b63c-878a-002a7cb586a6@gmail.com>
<d6e84dc8-75d1-4c3b-4b32-c4fcb7852275@gmail.com>
<3eaeaa3a-b78e-ef0d-7319-5d713bbc09a0@gmail.com>
<20230116173723.exeijemoytfo2yc2@awork3.anarazel.de>
On 1/16/23 18:37, Andres Freund wrote:
> Hi,
>
> On 2023-01-02 14:28:20 +0100, David Geier wrote:
>
> I'm doubtful this is worth the complexity it incurs. By the time we convert
> out of the instr_time format, the times shouldn't be small enough that the
> accuracy is affected much.
I don't feel strong about it and you have a point that we most likely
only convert ones we've accumulated a fair amount of cycles.
> Looking around, most of the existing uses of INSTR_TIME_GET_MICROSEC()
> actually accumulate themselves, and should instead keep things in the
> instr_time format and convert later. We'd win more accuracy / speed that way.
>
> I don't think the introduction of pg_time_usec_t was a great idea, but oh
> well.
Fully agreed. Why not replacing pg_time_usec_t with instr_time in a
separate patch? I haven't looked carefully enough if all occurrences
could sanely replaced but at least the ones that accumulate time seem
good starting points.
>> Additionally, I initialized a few variables of type instr_time which
>> otherwise resulted in warnings due to use of potentially uninitialized
>> variables.
> Unless we decide, as I suggested downthread, that we deprecate
> INSTR_TIME_SET_ZERO(), that's unfortunately not the right fix. I've a similar
> patch that adds all the necesarry INSTR_TIME_SET_ZERO() calls.
I don't feel strong about it, but like Tom tend towards keeping the
initialization macro.
Thanks that you have improved on the first patch and fixed these issues
in a better way.
>> What about renaming INSTR_TIME_GET_DOUBLE() to INSTR_TIME_GET_SECS() so that
>> it's consistent with the _MILLISEC() and _MICROSEC() variants?
>> The INSTR_TIME_GET_MICROSEC() returns a uint64 while the other variants
>> return double. This seems error prone. What about renaming the function or
>> also have the function return a double and cast where necessary at the call
>> site?
> I think those should be a separate discussion / patch.
OK. I'll propose follow-on patches ones we're done with the ones at hand.
I'll then rebase the RDTSC patches on your patch set.
--
David Geier
(ServiceNow)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: geidav.pg@gmail.com, andres@anarazel.de, michael@paquier.xyz, ibrar.ahmad@gmail.com, m.sakrejda@gmail.com, lukas@fittl.com
Subject: Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?
In-Reply-To: <d07fca50-d6bd-f60c-c440-a480f8156c92@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox