Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?

Hi Zsolt,

Thanks for reviewing!

On Wed, Mar 11, 2026 at 12:52 PM Zsolt Parragi
<zsolt.parragi@percona.com> wrote:
>
> + /*
> + * Once freq_khz / prev_freq_khz is small, check if it stays that way.
> + * If it does for long enough, we've got a winner frequency.
> + */
> + if (prev_freq_khz != 0 && fabs(freq_khz / prev_freq_khz) < 1.0001)
>
> Doesn't this accept any frequency decrease as stable? Shouldn't it
> instead use something like
>
> fabs(1 - freq_khz / prev_freq_khz) < 0.0001
>
> which works in both directions?

Yeah, good point - I think you're correct.

I'll wait for Andres take another look at the TSC calibration logic as
written in the patch to see if there are further suggestions on how to
adjust, but otherwise will make that change in the next revision.

> - total_time = duration > 0 ? duration * INT64CONST(1000000000) : 0;
> + INSTR_TIME_SET_NANOSEC(duration_time, duration > 0 ? duration * NS_PER_S : 0);
>
>   INSTR_TIME_SET_CURRENT(start_time);
> - cur = INSTR_TIME_GET_NANOSEC(start_time);
> + cur = start_time;
>
> - while (time_elapsed < total_time)
> + end_time = start_time;
> + INSTR_TIME_ADD(end_time, duration_time);
>
> Is this correct? Won't INSTR_TIME_ADD add nanoseconds (duration_time)
> to TSC ticks (end_time)? INSTR_TIME_GET_NANOSEC uses pg_ticks_to_ns,
> but INSTR_TIME_SET_NANOSEC simply stores the value without any
> conversion function.

Ah, good catch - we basically have to do the inverse of pg_ticks_to_ns
when PG_INSTR_TICKS_TO_NS is active and ticks_per_ns_scaled is
non-zero.

I think having INSTR_TIME_SET_NANOSEC around to be able to set a
"target time" that then gets compared against in a loop seems more
generally useful, e.g. it could allow using the instr_time
infrastructure for cases like the one reported at [0], so I'll go add
that in the next revision, unless I hear feedback the other way.

> Also, if the clock source is switched during statement we get totally
> incorrect results. I don't think this has any real consequences in the
> current use case, but maybe it's worth at least mentioning somewhere?

Yeah, that's basically by design, because we can't invalidate prior
instr_time values when the timing clock source changes. I don't think
this is a big problem in practice, since changing the timing source
would be a rare thing to do - I think its mainly useful to do so
interactively when confirming numbers in case the values look off with
TSC.

Maybe we should emit a warning from the assign function when called
from inside a transaction? (since that'd be a clear cut case where its
not a good idea to do the SET)

Alternatively documenting this explicitly sounds good. Were you
thinking of user-facing documentation, or more on the code side?

Thanks,
Lukas

[0]: https://www.postgresql.org/message-id/flat/CAJhEC07OK8J7tLUbyiccnuOXRE7UKxBNqD2-pLfeFXa%3DtBoWtw%40mail.gmail.com

-- 
Lukas Fittl