Subject: pg_test_timing: fix unit typo and widen diff type

From: Chao Li <[email-masked]>
Date: Thu, 2 Apr 2026 11:09:23 +0800

Hi,

This morning, as part of my usual routine, I synced the master branch and read through the recent commits. While reading 82c0cb4e672, I noticed a mistake in an error message. The relevant code is like:
```
diff = INSTR_TIME_GET_NANOSEC(diff_time);

fprintf(stderr, _("Time warp: %d ms\n"), diff);
```

Here, “diff" is in nanoseconds, but the error message prints ms as the unit, which is incorrect.

To fix that, I think there are two possible options:

1. Use INSTR_TIME_GET_MILLISEC to get “diff"
2. Change “ms" to “ns" in the error message

After reading through the whole file, I think option 2 is the right fix. While doing that, I also noticed another issue.

“diff" is currently defined as int32. Although one might think that is enough for a time delta, I believe it should be int64 for two reasons:

* INSTR_TIME_GET_NANOSEC() explicitly returns int64:
```
#define INSTR_TIME_GET_NANOSEC(t) \
               ((int64) (t).ticks)
```

* The current code has a sanity check for backward clock drift:
```
        /* Did time go backwards? */
        if (unlikely(diff < 0))
        {
            fprintf(stderr, _("Detected clock going backwards in time.\n"));
            fprintf(stderr, _("Time warp: %d ms\n"), diff);
            exit(1);
        }
```
Clock jumping forward is also possible, and a forward jump of about 2.14 seconds would overflow int32 when expressed in nanoseconds, making the value appear negative. In that case, the code could report a “backwards” clock jump when the actual jump was forwards, which would be misleading.

To make the patch easier to process, I split it into two parts: 0001 fixes the unit in the error message, and 0002 changes the type of diff. If this gets accepted, I would be happy to squash them into one commit.

I should also note that although I noticed this while reading 82c0cb4e672, I do not think this was an oversight of that commit. More likely, because clock drift backwards is rare, this issue has simply gone unnoticed for many years.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

---

From: Lukas Fittl <[email-masked]>
Date: Wed, 1 Apr 2026 21:17:35 -0700

Hi Chao,

> Here, “diff" is in nanoseconds, but the error message prints ms as the unit, which is incorrect.

Good catch!

It looks like the use of nanoseconds for "diff" got introduced last
year in 0b096e379e6f9bd49 (as you note later in the email, today's
commit didn't actually change that part), CCing Tom and Hannu as
authors of that earlier change.

That said, its a bit odd that we were using INSTR_TIME_GET_MICROSEC
there before that earlier commit, but called it "ms" (i.e.
milliseconds) in the error message.

> Clock jumping forward is also possible, and a forward jump of about 2.14 seconds would overflow int32 when expressed in nanoseconds, making the value appear negative. In that case, the code could r…

I agree it doesn't seem sound to use an int32 for storing the result
of INSTR_TIME_GET_NANOSEC. It looks like we may also need to adjust
the call to pg_leftmost_one_pos32 though if we actually accept that
large a "diff" value, as in your patch.

Maybe we should error out if the diff is larger than an int32, noting
a positive time drift?

Independently of that, its worth noting we could easily emit the diff
in a larger unit (micro or milliseconds) for easier interpretation, by
just calling INSTR_TIME_GET_MICROSEC / INSTR_TIME_GET_MILLISEC on the
"diff_time" again.

Thanks,
Lukas

--
Lukas Fittl

---

From: Chao Li <[email-masked]>
Date: Thu, 2 Apr 2026 15:28:48 +0800

> On Apr 2, 2026, at 12:17, Lukas Fittl <[email-masked]> wrote:
> 
> Hi Chao,
> 

You are right. Changed to pg_leftmost_one_pos64 in v2.

> Maybe we should error out if the diff is larger than an int32, noting

I agree we should warn/fail upon clock forwards drift. But I doubt int32 is too big (~2.14 seconds), I consider 1 second could be a too big threshold. Let’s wait for more voices on this.

> just calling INSTR_TIME_GET_MICROSEC / INSTR_TIME_GET_MILLISEC on the

Given the error should rarely happen, I personally feel that might not be super helpful. Also, if the drift is just beyond the threshold, bumping to microsecond or millisecond might print just 0.

PFA v2 - updated 0002 for pg_leftmost_one_pos64.

Best regards,

---

From: Chao Li <[email-masked]>
Date: Wed, 8 Apr 2026 12:06:07 +0800

> On Apr 2, 2026, at 15:28, Chao Li <[email-masked]> wrote:
> 
> 
> 
> <v2-0001-pg_test_timing-fix-unit-in-backward-clock-warning.patch><v2-0002-pg_test_timing-use-int64-for-largest-observed-tim.patch>

PFA v3: Fixed a CI failure.

Best regards,

---

From: "wang.xiao.peng" <[email-masked]>
Date: Thu, 9 Apr 2026 14:12:39 +0800 (CST)

>PFA v3: Fixed a CI failure.
Hi Chao, Lukas,
Just finished reviewing the v3 patches - looks good to me. The pg_leftmost_one_pos64 fix is spot on, and using INT64_FORMAT for the error message is the right approach.
Nice catch on the unit mismatch and the int32 overflow issue. Both patches are ready to go from my perspective.
Cheers,
Xiaopeng Wang

---

From: Fujii Masao <[email-masked]>
Date: Wed, 22 Apr 2026 16:04:16 +0900

> Just finished reviewing the v3 patches - looks good to me. The pg_leftmost_one_pos64 fix is spot on, and using INT64_FORMAT for the error message is the right approach.

Since pg_leftmost_one_pos64() can return up to 63, should the size of
histogram[] be changed from 32 to 64? If we want to display the full
histogram[] in the output, max_bit in output() would also need to be set to 63.
Alternatively, it may be fine to keep max_bit = 31 and show only
the first 32 histogram entries.

Patch 0001 looks good to me.

Regards,

-- 
Fujii Masao

---

From: Chao Li <[email-masked]>
Date: Wed, 22 Apr 2026 16:57:32 +0800

> On Apr 22, 2026, at 15:04, Fujii Masao <[email-masked]> wrote:
> 

I revisited this patch and think it makes sense to extend histogram[] to 64 entries.

In practice, the higher buckets will almost always remain zero, and output() already omits trailing histogram entries with zero counts, so this change is unlikely to affect the actual runtime output in normal cases.

Accordingly, I also adjusted the minimum width of the first column, since the maximum int64 value has 19 digits, and updated the printf format accordingly.

> Patch 0001 looks good to me.

Cool.

PFA v4:

* 0001 unchanged from v3.
* 0002 changed size of histogram[] 64.

Best regards,

---

From: Fujii Masao <[email-masked]>
Date: Thu, 23 Apr 2026 01:11:53 +0900

> * 0002 changed size of histogram[] 64.

Thanks for updating the patch!

Since patch 0001 is a bug fix, it should be backpatched to the supported stable
branches. However, in v19 the unit should be changed like s/ms/ns, while in v18
and earlier s/ms/us, since those older branches report the diff in microseconds.
Right?

Patch 0002 looks more like an improvement than a bug fix, so we should probably
wait for the next CommitFest before committing it. Thoughts?

Regards,

-- 
Fujii Masao

---

From: Chao Li <[email-masked]>
Date: Thu, 23 Apr 2026 10:02:31 +0800

> On Apr 23, 2026, at 00:11, Fujii Masao <[email-masked]> wrote:
> 

I just checked old branches. Looks like 0001 can be back-patched down to v10. Yes, pre-19, all branches use microsecond, so the back-patch should change “ms” to “us”.

I have helped create back-patch diff files as attached:

* nocfbot.v10-v13.pg_test_timing-backpatch.diff for v10-v13
* nocfbot.v14-v15.pg_test_timing-backpatch.diff for v14-v15
* nocfbot.v16-v18.pg_test_timing-backpatch.diff for v16-v18

For v16 to v18, we can make a tiny improvement by replacing “1e9” with a constant macro NS_PER_S. This change has been included in the diff.

I have built and tested on all branches.

> wait for the next CommitFest before committing it. Thoughts?

I see 0002 a bit differently. In v19, the unit changed from microseconds to nanoseconds, which introduced a potential overflow: nanoseconds require int64, but the local variable remained int32. So I think this is actually a v19-only bug.

PFA v5: 0001 replaced “1e9” with NS_PER_S; 0002 unchanged.

Best regards,

---

From: Fujii Masao <[email-masked]>
Date: Fri, 24 Apr 2026 12:15:50 +0900

> I just checked old branches. Looks like 0001 can be back-patched down to v10. Yes, pre-19, all branches use microsecond, so the back-patch should change “ms” to “us”.

Thanks! So I've pushed this unit change to master and backpatched it to
all supported branches.

> For v16 to v18, we can make a tiny improvement by replacing “1e9” with a constant macro NS_PER_S. This change has been included in the diff.

This change looks good to me. However, we should generally keep changes to
old stable branches to the minimum required, so I applied this change only
to master with 0002 patch.

> I see 0002 a bit differently. In v19, the unit changed from microseconds to nanoseconds, which introduced a potential overflow: nanoseconds require int64, but the local variable remained int32. So…

OK, I've pushed 0002 patch. Thanks!

Regards,

-- 
Fujii Masao

---

From: Chao Li <[email-masked]>
Date: Fri, 24 Apr 2026 11:30:10 +0800

> On Apr 24, 2026, at 11:15, Fujii Masao <[email-masked]> wrote:
> 

Hi Fujii-san, thank you very much for pushing.

Best regards,