Thread
Commits
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
pg_test_timing: store timing deltas in int64
- 863c4b827d92 19 (unreleased) landed
-
pg_test_timing: fix unit in backward-clock warning
- fb5acf2d8f2e 14.23 landed
- d7241c156d41 15.18 landed
- 07e96aeff93f 16.14 landed
- 1fdf1c63744c 17.10 landed
- a8dbe5288b0e 18.4 landed
- 019cc9962bdb 19 (unreleased) landed
-
pg_test_timing: fix unit typo and widen diff type
Chao Li <li.evan.chao@gmail.com> — 2026-04-02T03:09:23Z
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/ -
Re: pg_test_timing: fix unit typo and widen diff type
Lukas Fittl <lukas@fittl.com> — 2026-04-02T04:17:35Z
Hi Chao, On Wed, Apr 1, 2026 at 8:10 PM Chao Li <li.evan.chao@gmail.com> wrote: > > 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. 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. > > 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. 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 -
Re: pg_test_timing: fix unit typo and widen diff type
Chao Li <li.evan.chao@gmail.com> — 2026-04-02T07:28:48Z
> On Apr 2, 2026, at 12:17, Lukas Fittl <lukas@fittl.com> wrote: > > Hi Chao, > > On Wed, Apr 1, 2026 at 8:10 PM Chao Li <li.evan.chao@gmail.com> wrote: >> >> 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. > > 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. > >> >> 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. > > 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. 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 > a positive time drift? 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. > > 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. > 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, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: pg_test_timing: fix unit typo and widen diff type
Chao Li <li.evan.chao@gmail.com> — 2026-04-08T04:06:07Z
> On Apr 2, 2026, at 15:28, Chao Li <li.evan.chao@gmail.com> wrote: > > > >> On Apr 2, 2026, at 12:17, Lukas Fittl <lukas@fittl.com> wrote: >> >> Hi Chao, >> >> On Wed, Apr 1, 2026 at 8:10 PM Chao Li <li.evan.chao@gmail.com> wrote: >>> >>> 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. >> >> 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. >> >>> >>> 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. >> >> 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. > > 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 >> a positive time drift? > > 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. > >> >> 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. >> > > 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, > -- > Chao Li (Evan) > HighGo Software Co., Ltd. > https://www.highgo.com/ > > > > > <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, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re:Re: pg_test_timing: fix unit typo and widen diff type
Xiaopeng Wang <wxp_728@163.com> — 2026-04-09T06:12:39Z
At 2026-04-08 12:06:07, "Chao Li" <li.evan.chao@gmail.com> wrote: > > >> On Apr 2, 2026, at 15:28, Chao Li <li.evan.chao@gmail.com> wrote: >> >> >> >>> On Apr 2, 2026, at 12:17, Lukas Fittl <lukas@fittl.com> wrote: >>> >>> Hi Chao, >>> >>> On Wed, Apr 1, 2026 at 8:10 PM Chao Li <li.evan.chao@gmail.com> wrote: >>>> >>>> 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. >>> >>> 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. >>> >>>> >>>> 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. >>> >>> 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. >> >> 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 >>> a positive time drift? >> >> 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. >> >>> >>> 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. >>> >> >> 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, >> -- >> Chao Li (Evan) >> HighGo Software Co., Ltd. >> https://www.highgo.com/ >> >> >> >> >> <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, >-- >Chao Li (Evan) >HighGo Software Co., Ltd. >https://www.highgo.com/ > > 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 -
Re: Re: pg_test_timing: fix unit typo and widen diff type
Fujii Masao <masao.fujii@gmail.com> — 2026-04-22T07:04:16Z
On Thu, Apr 9, 2026 at 3:13 PM wang.xiao.peng <wxp_728@163.com> wrote: > 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
-
Re: pg_test_timing: fix unit typo and widen diff type
Chao Li <li.evan.chao@gmail.com> — 2026-04-22T08:57:32Z
> On Apr 22, 2026, at 15:04, Fujii Masao <masao.fujii@gmail.com> wrote: > > On Thu, Apr 9, 2026 at 3:13 PM wang.xiao.peng <wxp_728@163.com> wrote: >> 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. 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, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
-
Re: pg_test_timing: fix unit typo and widen diff type
Fujii Masao <masao.fujii@gmail.com> — 2026-04-22T16:11:53Z
On Wed, Apr 22, 2026 at 5:58 PM Chao Li <li.evan.chao@gmail.com> wrote: > PFA v4: > > * 0001 unchanged from v3. > * 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
-
Re: pg_test_timing: fix unit typo and widen diff type
Chao Li <li.evan.chao@gmail.com> — 2026-04-23T02:02:31Z
> On Apr 23, 2026, at 00:11, Fujii Masao <masao.fujii@gmail.com> wrote: > > On Wed, Apr 22, 2026 at 5:58 PM Chao Li <li.evan.chao@gmail.com> wrote: >> PFA v4: >> >> * 0001 unchanged from v3. >> * 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? 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. > > Patch 0002 looks more like an improvement than a bug fix, so we should probably > 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, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
-
Re: pg_test_timing: fix unit typo and widen diff type
Fujii Masao <masao.fujii@gmail.com> — 2026-04-24T03:15:50Z
On Thu, Apr 23, 2026 at 11:03 AM Chao Li <li.evan.chao@gmail.com> 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”. 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 I think this is actually a v19-only bug. OK, I've pushed 0002 patch. Thanks! Regards, -- Fujii Masao
-
Re: pg_test_timing: fix unit typo and widen diff type
Chao Li <li.evan.chao@gmail.com> — 2026-04-24T03:30:10Z
> On Apr 24, 2026, at 11:15, Fujii Masao <masao.fujii@gmail.com> wrote: > > On Thu, Apr 23, 2026 at 11:03 AM Chao Li <li.evan.chao@gmail.com> 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”. > > 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 I think this is actually a v19-only bug. > > OK, I've pushed 0002 patch. Thanks! > > Regards, > > -- > Fujii Masao Hi Fujii-san, thank you very much for pushing. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/