Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?

Hi John,

On Sun, Mar 15, 2026 at 5:49 PM John Naylor <johncnaylorls@gmail.com> wrote:
>
> On Tue, Mar 10, 2026 at 5:04 PM Lukas Fittl <lukas@fittl.com> wrote:
>
> > > Elsewhere in the patch series there are comments that refer to EBX etc
> > > while the code looks like exx[1]. It hasn't been a problem up to now
> > > since there were only a couple places that used cpuid. But this
> > > patchset adds quite a few more, so now seems like a good time to make
> > > it more readable with register name symbols.
> >
> > I'm not a fan of using macros for this, but what if we define
> > ourselves a struct like this:
> >
> > typedef struct CPUIDResult
> > {
> >     unsigned int eax;
> >     unsigned int ebx;
> >     unsigned int ecx;
> >     unsigned int edx;
> > } CPUIDResult;
> >
> > Then the code reads like:
> >
> > pg_cpuid(0x80000001, &r);
> > X86Features[PG_RDTSCP] = r.edx >> 27 & 1;
>
> One objection is that __cpuid() takes an array of 4 integers as an
> argument. I think it would technically happen to work to pass a
> pointer to this struct, but it seems the wrong thing to do. If you're
> not a fan of macros, the other way would be an enum of indices (and
> named with all caps).

How about we deal with this in the pg_cpuid function by having
__cpuid() write into a separate array and then assign that to
CPUIDResult? See the attached 0001 patch.

I think the struct fields are the clearest approach here, but if you
disagree let me know and I can adjust further.

> > > + if (exx[2] & (1 << 27))
> > > + {
> > > + uint32 xcr0_val = 0;
> > > +
> > >
> > > By moving the call to leaf 7 up, the results of leaf 1 just got blown
> > > away, so isn't OXSAVE support now broken? Maybe we actually want 2
> > > separate arrays? (Like "regs" and "ext_regs") That'll also avoid
> > > having to memset.
> >
> > Yeah, I think using two result variables make sense here.
> > Alternatively we could save the result of the OXSAVE check in a
> > boolean.
>
> Actually I like the idea of a boolean better, since that would be less
> churn, and maybe simpler to reason about. 0005 overwrites one of the
> variables in subsequent calls, so it could be confusing for future
> additions as to which one to use.

Yeah, makes sense - switched it to a boolean in the 0005 patch that
adds the TSC logic.

> v11-0001:
>
> +static inline bool
> +pg_cpuid_subleaf(int leaf, int subleaf, CPUIDResult *r)
> +{
> +#if defined(HAVE__GET_CPUID_COUNT)
> +  return __get_cpuid_count(leaf, subleaf, &r->eax, &r->ebx, &r->ecx,
> &r->edx) == 1;
> +#elif defined(HAVE__CPUIDEX)
> +  __cpuidex((int *) r, leaf, subleaf);
> +  return true;
> +#else
> +  memset(r, 0, sizeof(CPUIDResult));
> +  return false;
> +#endif
> +}
>
> This needs a comment to explain the return value.

Good point, added a comment.

I think that should address all open feedback on the 0001 patch.

Attached v12, which also has these additional changes:

v12/0003 (pg_test_timing: Reduce per-loop overhead)

- Fixed the ns to ticks translation issue, per Zsolt's feedback
- Reworked this to use a INSTR_TIME_ADD_NANOSEC macro (instead of
INSTR_TIME_SET_NANOSEC) - I think that's best for the intent of "set a
target time to compare against using a time interval", and avoids
callers passing in fixed timestamps that'd be more expensive to
convert

v12/0004 (Streamline ticks to nanosecond conversion across platforms)

- Moved pg_initialize_timing to InitProcessGlobals instead of main. I
had previously placed this in main because it might have helped a
different TSC calibration technique that could have benefited from a
distance between two function calls, but the calibration function
appears fast enough that we can just run it all at once.
- Add assert that pg_initialize_timing was called before INSTR* macros
are called, and add a missing pg_initialize_timing in pg_regress, as
well as correct where we call it for pgbench

v12/0005 (Main TSC patch)

- Clarify that TSC frequency can technically change between instr_time
set and read (in code, next to the relevant variables), per Zsolt's
feedback
- Add pg_cpuidex helper that can be used to call _cpuidex (and
intentionally not __get_cpuid_count) for getting VM Hypervisor
information

TSC calibration improvements:
- Pass TSC frequency to child processes for EXEC_BACKEND, this makes
TSC calibration usable on Windows, per off-list suggestion from Andres
- To support the EXEC_BACKEND change, refactor tsc_frequency_khz to
have a sentinel value of -1 indicating the TSC was not initialized, vs
0 indicating it was initialized but isn't usable, and drop
has_usable_tsc variable
- Fix the TSC calibration logic to handle frequency decrease, per
Zsolt's feedback
- Run TSC calibration only once GUCs are being set, since we don't
need it earlier and we can simplify the client program logic that way
(if you don't call pg_set_timing_clock_source you won't get TSC)
- Based on further testing, measure the frequency only every 100
iterations, and require at least 10 stable cycles (i.e. effective min
iterations is 1000, max is 1 million as before) -- this improves the
accuracy on my two test machines (AMD bare metal + Azure HyperV)

v12/0006 (pg_test_timing patch)

- Show TSC frequency in pg_test_timing output

Thanks,
Lukas

--
Lukas Fittl