Horton
Menu

Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?

John Naylor <johncnaylorls@gmail.com>

View thread
From: John Naylor <johncnaylorls@gmail.com>
To: Lukas Fittl <lukas@fittl.com>
Cc: Andres Freund <andres@anarazel.de>, Jakub Wartak <jakub.wartak@enterprisedb.com>, Hannu Krosing <hannuk@google.com>, Robert Haas <robertmhaas@gmail.com>, Pavel Stehule <pavel.stehule@gmail.com>, vignesh C <vignesh21@gmail.com>, Michael Paquier <michael@paquier.xyz>, Ibrar Ahmed <ibrar.ahmad@gmail.com>, Maciek Sakrejda <m.sakrejda@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, David Geier <geidav.pg@gmail.com>
Date: 2026-03-16T00:49:17Z
Lists: pgsql-hackers 
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).

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

> > + level_type = (exx[2] >> 8) & 0xff;
> >
> > The comment says "ECX[15:8]". This looks like ECX[7:0] but maybe I'm
> > missing something.
>
> I think the code is right - bits 15 to 8 define the level type, and so
> we shift that right by 8 bits, and then apply 0xff to drop anything to
> the left of that.

Yes, I must have missed the shift, thanks.

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.


--
John Naylor
Amazon Web Services

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. pg_test_timing: Show additional TSC clock source debug info

  2. instrumentation: Avoid CPUID 0x15/0x16 for Hypervisor TSC frequency

  3. pg_test_timing: Also test RDTSC[P] timing, report time source, TSC frequency

  4. Allow retrieving x86 TSC frequency/flags from CPUID

  5. instrumentation: Standardize ticks to nanosecond conversion method

  6. instrumentation: Use Time-Stamp Counter on x86-64 to lower overhead

  7. Check for __cpuidex and __get_cpuid_count separately

  8. pg_test_timing: Reduce per-loop overhead

  9. Refactor handling of x86 CPUID instructions

  10. instrumentation: Drop INSTR_TIME_SET_CURRENT_LAZY macro

  11. Rename pg_crc32c_sse42_choose.c for general purpose

  12. Zero initialize uses of instr_time about to trigger compiler warnings

  13. instr_time: Represent time as an int64 on all platforms

  14. Add 250c8ee07ed to git-blame-ignore-revs