| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | pg_test_timing: fix unit typo and widen diff type |
| Date: | 2026-04-02 03:09:23 |
| Message-ID: | F780CEEB-A237-4302-9F55-60E9D8B6533D@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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/
| Attachment | Content-Type | Size |
|---|---|---|
| v1-0001-pg_test_timing-fix-unit-in-backward-clock-warning.patch | application/octet-stream | 1.1 KB |
| v1-0002-pg_test_timing-use-int64-for-largest-observed-tim.patch | application/octet-stream | 1.9 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Chao Li | 2026-04-02 03:11:04 | Re: Redundant/mis-use of _(x) gettext macro? |
| Previous Message | Peter Smith | 2026-04-02 02:55:55 | Re: Redundant/mis-use of _(x) gettext macro? |