Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc? - Mailing list pgsql-hackers

Hi,

Random things I noticed while looking through v19:

- src/common/instr_time.c has the wrong IDENTIFICATION


- instr_time.c should do

#ifndef FRONTEND
#include "postgres.h"
#else
#include "postgres_fe.h"
#endif

  instead of always including postgres.h


- It's not PG style to put #includes in the middle of a file as done in
  instrument.c

- tsc_use_by_default() may be documenting things that aren't the case anymore


- It may be paranoia, but it seems like tsc_calibrate() should perhaps save
  the old clock source and restore it at the end?


- A brief comment in the source (rather than just the thread), why
  TSC_CALIBRATION_SKIPS exists would be nice


- pg_initialize_timing() references an allow_tsc_calibration argument that
  doesn't exist anymore


- Wonder if it would be clearer if pg_initialize_timing() callled
  set_ticks_per_ns_system()?


- Should pg_initialize_timing() allow repeated initialization? Seems like that
  would normally be a bug?


- It's nice that pg_test_timing shows the frequency.  I was thinking it were
  able to show the result of the calibration, even if we were able to
  determine the frequency without calibration.  That should make it easier to
  figure out whether the calibration works well.


- Wonder if some of the code would look a bit cleaner if timing_tsc_enabled,
  timing_tsc_frequency_khz were defined regardless of PG_INSTR_TSC_CLOCK.


- is it ok that we are doing pg_cpuid_subleaf() without checking the result?

  It's not clear to me if a failed __get_cpuid_count() would clear out the old
  reg or leave it in place.


- I don't think the elog() in pg_initialize_timing_tsc() works in any common
  scenario, as log_min_messages hasn't been initialized yet, so that message
  won't ever be emitted.

  If it used a different log level, it'd be emitted without knowing the
  log_line_prefix etc, because that hasn't yet been initialized.


- How much do we care about weird results when dynamically changing
  timing_clocksource?

postgres[182055][1]=# EXPLAIN ANALYZE SELECT set_config('timing_clock_source', 'tsc', true), pg_sleep(1),
set_config('timing_clock_source','system', true), pg_sleep(1);
 
┌────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│                                                 QUERY PLAN                                                 │
├────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Result  (cost=0.00..0.01 rows=1 width=72) (actual time=-6540570569.396..-6540570569.395 rows=1.00 loops=1) │
│ Planning Time: 0.184 ms                                                                                    │
│ Execution Time: -6540570569.355 ms                                                                         │
└────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
(3 rows)

Time: 2002.350 ms (00:02.002)


- SGML:

  - "modern CPUs" seems a bit pejorative, at least if we don't merge the
    aarch64 stuff.

  - 'tsc' describes just x86-64, even if there is a patch to support aarch64.
    Perhaps it'd be enough to sprinkle a few "E.g. on x86-64, ..." around.


- I wonder if it'd be useful to have a INSTR_TIME_SET_CURRENT_SYSTEM() that'd
  always use the "system" method, but convert it to to ticks if appropriate.

  E.g. test_tsc_timing() could measure the elapsed time with system and
  display the difference in how long the test_timing() takes between tsc based
  source and system time.

  This might be more of a thing for later.


Think we're getting there...

Greetings,

Andres