Fix comments in instr_time.h and remove an unneeded cast to int64

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)

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)