On 06/08/2024 17:20, Tom Lane wrote:
Heikki Linnakangas <hlinn...@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)
From 47a209a1840c9f66b584fb03a99baf56c5abe69f Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@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/zrhkv3maqfwns...@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
From a78416110d0d0746f668c7d133d40d7c7f982d49 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@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/zrhkv3maqfwns...@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
