public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Subject: pg_test_timing: fix unit typo and widen diff type
Date: Thu, 2 Apr 2026 11:09:23 +0800
Message-ID: <[email protected]> (raw)
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/
Attachments:
[application/octet-stream] v1-0001-pg_test_timing-fix-unit-in-backward-clock-warning.patch (1.1K, 2-v1-0001-pg_test_timing-fix-unit-in-backward-clock-warning.patch)
download | inline diff:
From 0dc6d45322d60fb54c27a649f2b146242ed64f62 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Thu, 2 Apr 2026 10:21:40 +0800
Subject: [PATCH v1 1/2] pg_test_timing: fix unit in backward-clock warning
pg_test_timing measures timing differences in nanoseconds via
INSTR_TIME_GET_NANOSEC(), but the backward-clock warning incorrectly
reported the value as milliseconds. Fix the message to use "ns" so
that it matches the actual unit of diff.
Author: Chao Li <[email protected]>
Reviewed-by:
Discussion: https://postgr.es/m/
---
src/bin/pg_test_timing/pg_test_timing.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/bin/pg_test_timing/pg_test_timing.c b/src/bin/pg_test_timing/pg_test_timing.c
index aee41dbe3f9..cbdc5af09d9 100644
--- a/src/bin/pg_test_timing/pg_test_timing.c
+++ b/src/bin/pg_test_timing/pg_test_timing.c
@@ -191,7 +191,7 @@ test_timing(unsigned int duration)
if (unlikely(diff < 0))
{
fprintf(stderr, _("Detected clock going backwards in time.\n"));
- fprintf(stderr, _("Time warp: %d ms\n"), diff);
+ fprintf(stderr, _("Time warp: %d ns\n"), diff);
exit(1);
}
--
2.50.1 (Apple Git-155)
[application/octet-stream] v1-0002-pg_test_timing-use-int64-for-largest-observed-tim.patch (1.9K, 3-v1-0002-pg_test_timing-use-int64-for-largest-observed-tim.patch)
download | inline diff:
From e29ff7fef693e830b4929f9b7444cc943fd16516 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Thu, 2 Apr 2026 10:29:06 +0800
Subject: [PATCH v1 2/2] pg_test_timing: use int64 for largest observed time
delta
pg_test_timing obtains timing deltas with INSTR_TIME_GET_NANOSEC(),
which returns a nanosecond value that should be stored in int64. Adjust
diff and largest_diff to use int64 accordingly.
Also fix the corresponding printf format specifiers so these values are
printed correctly.
Author: Chao Li <[email protected]>
Reviewed-by:
Discussion: https://postgr.es/m/
---
src/bin/pg_test_timing/pg_test_timing.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_test_timing/pg_test_timing.c b/src/bin/pg_test_timing/pg_test_timing.c
index cbdc5af09d9..07b98a6388d 100644
--- a/src/bin/pg_test_timing/pg_test_timing.c
+++ b/src/bin/pg_test_timing/pg_test_timing.c
@@ -25,7 +25,7 @@ static long long int histogram[32];
static long long int direct_histogram[NUM_DIRECT];
/* separately record highest observed duration */
-static int32 largest_diff;
+static int64 largest_diff;
static long long int largest_diff_count;
@@ -176,8 +176,8 @@ test_timing(unsigned int duration)
while (INSTR_TIME_GT(end_time, cur))
{
- int32 diff,
- bits;
+ int64 diff;
+ int32 bits;
instr_time diff_time;
prev = cur;
@@ -191,7 +191,7 @@ test_timing(unsigned int duration)
if (unlikely(diff < 0))
{
fprintf(stderr, _("Detected clock going backwards in time.\n"));
- fprintf(stderr, _("Time warp: %d ns\n"), diff);
+ fprintf(stderr, _("Time warp: %lld ns\n"), diff);
exit(1);
}
@@ -319,7 +319,7 @@ output(uint64 loop_count)
double prct = (double) largest_diff_count * 100 / loop_count;
printf("...\n");
- printf("%*d %*.4f %*.4f %*lld\n",
+ printf("%*lld %*.4f %*.4f %*lld\n",
len1, largest_diff,
len2, prct,
len3, 100.0,
--
2.50.1 (Apple Git-155)
view thread (11+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: pg_test_timing: fix unit typo and widen diff type
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox