Thread: Fix comments in instr_time.h and remove an unneeded cast to int64
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. [1]: https://www.postgresql.org/message-id/19E276C9-2C2B-435A-B275-8FA22222AEB8%40gmail.com Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
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)
Attachment
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)