From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Joseph Koshakow <koshy44@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Andres Freund <andres@anarazel.de>
Subject: Re: Remove dependence on integer wrapping
Date: Sun, 9 Jun 2024 21:37:03 -0500
Message-ID: <ZmZmz9mnTYNT4aCd@nathan> (raw)
In-Reply-To: <618777.1717984674@sss.pgh.pa.us>
References: <CAAvxfHdBPOyEGS7s+xf4iaW0-cgiq25jpYdWBqQqvLtLe_t6tw@mail.gmail.com>
<ZmZbXK1CNMbkty0W@nathan>
<618777.1717984674@sss.pgh.pa.us>
On Sun, Jun 09, 2024 at 09:57:54PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> On Sun, Jun 09, 2024 at 04:59:22PM -0400, Joseph Koshakow wrote:
>>> The following comment was in the code for parsing timestamps:
>>> /* check for just-barely overflow (okay except time-of-day wraps) */
>>> /* caution: we want to allow 1999-12-31 24:00:00 */
>>>
>>> I wasn't able to fully understand it even after staring at it for
>>> a while. Is the comment suggesting that it is ok for the months field,
>>> for example, to wrap around? That doesn't sound right to me I tested
>>> the supplied timestamp, 1999-12-31 24:00:00, and it behaves the same
>>> before and after the patch.
>
>> I haven't stared at this for a while like you, but I am likewise confused
>> at first glance. This dates back to commit 84df54b, and it looks like this
>> comment was present in the first version of the patch in the thread [0]. I
>> CTRL+F'd for any discussion about this but couldn't immediately find
>> anything.
>
> I believe this is a copy-and-paste from 841b4a2d5, which added this:
>
> + *result = (date * INT64CONST(86400000000)) + time;
> + /* check for major overflow */
> + if ((*result - time) / INT64CONST(86400000000) != date)
> + return -1;
> + /* check for just-barely overflow (okay except time-of-day wraps) */
> + if ((*result < 0) ? (date >= 0) : (date < 0))
> + return -1;
>
> I think you could replace the whole thing by using overflow-aware
> multiplication and addition primitives in the result calculation.
> Lines 2-4 basically check for mult overflow and 5-7 for addition
> overflow.
Ah, I see. Joe's patch does that in one place. It's probably worth doing
that in the other places this "just-barefly overflow" comment appears IMHO.
I was still confused by the comment about 1999, but I tracked it down to
commit 542eeba [0]. IIUC it literally means that we need special handling
for that date because POSTGRES_EPOCH_JDATE is 2000-01-01.
[0] https://postgr.es/m/CABUevEx5zUO%3DKRUg06a9qnQ_e9EvTKscL6HxAM_L3xj71R7AQw%40mail.gmail.com
--
nathan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, tgl@sss.pgh.pa.us, koshy44@gmail.com, pgsql-hackers@lists.postgresql.org, andres@anarazel.de
Subject: Re: Remove dependence on integer wrapping
In-Reply-To: <ZmZmz9mnTYNT4aCd@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox