Hi Zsolt, Thanks for reviewing!
On Wed, Mar 11, 2026 at 12:52 PM Zsolt Parragi <[email protected]> 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
