Horton

Thread

Commits

  1. Fix datatypes in comments in instr_time.h

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

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

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-08-06T08:54:23Z

    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

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

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-08-06T11:28:38Z

    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)

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-06T14:20:36Z

    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

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

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-08-06T14:49:32Z

    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)

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-08-06T14:57:28Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-06T15:16:20Z

    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

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

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-08-06T19:18:45Z

    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)