Fix comments in instr_time.h and remove an unneeded cast to int64
Hi hackers,
While working on [1]/messages/by-id/19E276C9-2C2B-435A-B275-8FA22222AEB8@gmail.com, I came across what seems to be incorrect comments in
instr_time.h and an unneeded cast to int64.
Indeed, 03023a2664 represented time as an int64 on all platforms but forgot to
update the comment related to INSTR_TIME_GET_MICROSEC() and provided an incorrect
comment for INSTR_TIME_GET_NANOSEC().
Please find attached a tiny patch to correct those and, in passing, remove what
I think is an unneeded cast to int64.
[1]: /messages/by-id/19E276C9-2C2B-435A-B275-8FA22222AEB8@gmail.com
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Fix-comments-in-instr_time.h-and-remove-an-unneed.patchtext/x-diff; charset=us-asciiDownload
From 1d1f8089a9eba4e87af66d7c0f23f30d52dc042c Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Tue, 6 Aug 2024 08:04:43 +0000
Subject: [PATCH v1] Fix comments in instr_time.h and remove an unneeded cast
to int64
03023a2664 represented time as an int64 on all platforms but forgot to update
the comment related to INSTR_TIME_GET_MICROSEC() and provided an incorrect
comment for INSTR_TIME_GET_NANOSEC().
In passing removing an unneeded cast to int64.
---
src/include/portability/instr_time.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
100.0% src/include/portability/
diff --git a/src/include/portability/instr_time.h b/src/include/portability/instr_time.h
index a6fc1922f2..8f9ba2f151 100644
--- a/src/include/portability/instr_time.h
+++ b/src/include/portability/instr_time.h
@@ -32,9 +32,9 @@
*
* INSTR_TIME_GET_MILLISEC(t) convert t to double (in milliseconds)
*
- * INSTR_TIME_GET_MICROSEC(t) convert t to uint64 (in microseconds)
+ * INSTR_TIME_GET_MICROSEC(t) get t in microseconds
*
- * INSTR_TIME_GET_NANOSEC(t) convert t to uint64 (in nanoseconds)
+ * INSTR_TIME_GET_NANOSEC(t) get t in nanoseconds
*
* Note that INSTR_TIME_SUBTRACT and INSTR_TIME_ACCUM_DIFF convert
* absolute times to intervals. The INSTR_TIME_GET_xxx operations are
@@ -123,7 +123,7 @@ pg_clock_gettime_ns(void)
((t) = pg_clock_gettime_ns())
#define INSTR_TIME_GET_NANOSEC(t) \
- ((int64) (t).ticks)
+ ((t).ticks)
#else /* WIN32 */
--
2.34.1
On 06/08/2024 11:54, Bertrand Drouvot wrote:
Hi hackers,
While working on [1], I came across what seems to be incorrect comments in
instr_time.h and an unneeded cast to int64.Indeed, 03023a2664 represented time as an int64 on all platforms but forgot to
update the comment related to INSTR_TIME_GET_MICROSEC() and provided an incorrect
comment for INSTR_TIME_GET_NANOSEC().Please find attached a tiny patch to correct those and, in passing, remove what
I think is an unneeded cast to int64.
Applied, thanks!
--
Heikki Linnakangas
Neon (https://neon.tech)
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 06/08/2024 11:54, Bertrand Drouvot wrote:
Please find attached a tiny patch to correct those and, in passing, remove what
I think is an unneeded cast to int64.
Applied, thanks!
I think this comment change is a dis-improvement. It's removed the
documentation of the important fact that INSTR_TIME_GET_MICROSEC and
INSTR_TIME_GET_NANOSEC return a different data type from
INSTR_TIME_GET_MILLISEC (ie, integer versus float). Also, the
expectation is that users of these APIs do not know the actual data
type of instr_time, and instead we tell them what the output of those
macros is. This patch just blew a hole in that abstraction.
regards, tom lane
On 06/08/2024 17:20, Tom Lane wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 06/08/2024 11:54, Bertrand Drouvot wrote:
Please find attached a tiny patch to correct those and, in passing, remove what
I think is an unneeded cast to int64.Applied, thanks!
I think this comment change is a dis-improvement. It's removed the
documentation of the important fact that INSTR_TIME_GET_MICROSEC and
INSTR_TIME_GET_NANOSEC return a different data type from
INSTR_TIME_GET_MILLISEC (ie, integer versus float). Also, the
expectation is that users of these APIs do not know the actual data
type of instr_time, and instead we tell them what the output of those
macros is. This patch just blew a hole in that abstraction.
Hmm, ok I see. Then I propose:
1. Revert
2. Just fix the comment to say int64 instead of uint64.
--
Heikki Linnakangas
Neon (https://neon.tech)
Attachments:
0001-Revert-Fix-comments-in-instr_time.h-and-remove-an-un.patchtext/x-patch; charset=UTF-8; name=0001-Revert-Fix-comments-in-instr_time.h-and-remove-an-un.patchDownload
From 47a209a1840c9f66b584fb03a99baf56c5abe69f Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 6 Aug 2024 17:46:58 +0300
Subject: [PATCH 1/2] Revert "Fix comments in instr_time.h and remove an
unneeded cast to int64"
This reverts commit 3dcb09de7b. Tom Lane pointed out that it broke the
abstraction provided by the macros. The callers should not need to
know what the internal type is.
This commit is an exact revert, the next commit will fix the comments
on the macros that incorrectly claim that they return uint64.
Discussion: https://www.postgresql.org/message-id/ZrHkv3MAQfwNSmTG@ip-10-97-1-34.eu-west-3.compute.internal
---
src/include/portability/instr_time.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/include/portability/instr_time.h b/src/include/portability/instr_time.h
index 8f9ba2f151..a6fc1922f2 100644
--- a/src/include/portability/instr_time.h
+++ b/src/include/portability/instr_time.h
@@ -32,9 +32,9 @@
*
* INSTR_TIME_GET_MILLISEC(t) convert t to double (in milliseconds)
*
- * INSTR_TIME_GET_MICROSEC(t) get t in microseconds
+ * INSTR_TIME_GET_MICROSEC(t) convert t to uint64 (in microseconds)
*
- * INSTR_TIME_GET_NANOSEC(t) get t in nanoseconds
+ * INSTR_TIME_GET_NANOSEC(t) convert t to uint64 (in nanoseconds)
*
* Note that INSTR_TIME_SUBTRACT and INSTR_TIME_ACCUM_DIFF convert
* absolute times to intervals. The INSTR_TIME_GET_xxx operations are
@@ -123,7 +123,7 @@ pg_clock_gettime_ns(void)
((t) = pg_clock_gettime_ns())
#define INSTR_TIME_GET_NANOSEC(t) \
- ((t).ticks)
+ ((int64) (t).ticks)
#else /* WIN32 */
--
2.39.2
0002-Fix-datatypes-in-comments-in-instr_time.h.patchtext/x-patch; charset=UTF-8; name=0002-Fix-datatypes-in-comments-in-instr_time.h.patchDownload
From a78416110d0d0746f668c7d133d40d7c7f982d49 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 6 Aug 2024 17:47:21 +0300
Subject: [PATCH 2/2] Fix datatypes in comments in instr_time.h
The INSTR_TIME_GET_NANOSEC(t) and INSTR_TIME_GET_MICROSEC(t) macros
return a signed int64.
Discussion: https://www.postgresql.org/message-id/ZrHkv3MAQfwNSmTG@ip-10-97-1-34.eu-west-3.compute.internal
---
src/include/portability/instr_time.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/include/portability/instr_time.h b/src/include/portability/instr_time.h
index a6fc1922f2..e66ecf34cd 100644
--- a/src/include/portability/instr_time.h
+++ b/src/include/portability/instr_time.h
@@ -32,9 +32,9 @@
*
* INSTR_TIME_GET_MILLISEC(t) convert t to double (in milliseconds)
*
- * INSTR_TIME_GET_MICROSEC(t) convert t to uint64 (in microseconds)
+ * INSTR_TIME_GET_MICROSEC(t) convert t to int64 (in microseconds)
*
- * INSTR_TIME_GET_NANOSEC(t) convert t to uint64 (in nanoseconds)
+ * INSTR_TIME_GET_NANOSEC(t) convert t to int64 (in nanoseconds)
*
* Note that INSTR_TIME_SUBTRACT and INSTR_TIME_ACCUM_DIFF convert
* absolute times to intervals. The INSTR_TIME_GET_xxx operations are
--
2.39.2
Hi,
On Tue, Aug 06, 2024 at 05:49:32PM +0300, Heikki Linnakangas wrote:
On 06/08/2024 17:20, Tom Lane wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 06/08/2024 11:54, Bertrand Drouvot wrote:
Please find attached a tiny patch to correct those and, in passing, remove what
I think is an unneeded cast to int64.Applied, thanks!
I think this comment change is a dis-improvement. It's removed the
documentation of the important fact that INSTR_TIME_GET_MICROSEC and
INSTR_TIME_GET_NANOSEC return a different data type from
INSTR_TIME_GET_MILLISEC (ie, integer versus float). Also, the
expectation is that users of these APIs do not know the actual data
type of instr_time, and instead we tell them what the output of those
macros is. This patch just blew a hole in that abstraction.
Oh ok, did not think about it that way, thanks for the feedback!
Hmm, ok I see. Then I propose:
1. Revert
2. Just fix the comment to say int64 instead of uint64.
LGTM, thanks!
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Heikki Linnakangas <hlinnaka@iki.fi> writes:
Hmm, ok I see. Then I propose:
1. Revert
2. Just fix the comment to say int64 instead of uint64.
Yeah, it's probably reasonable to specify the output as int64
not uint64 (especially since it looks like that's what the
macros actually produce).
regards, tom lane
On 06/08/2024 18:16, Tom Lane wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
Hmm, ok I see. Then I propose:
1. Revert
2. Just fix the comment to say int64 instead of uint64.Yeah, it's probably reasonable to specify the output as int64
not uint64 (especially since it looks like that's what the
macros actually produce).
Committed
--
Heikki Linnakangas
Neon (https://neon.tech)