public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tom Lane <[email protected]>
To: Nathan Bossart <[email protected]>
Cc: Joseph Koshakow <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Andres Freund <[email protected]>
Subject: Re: Remove dependence on integer wrapping
Date: Sun, 09 Jun 2024 21:57:54 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <ZmZbXK1CNMbkty0W@nathan>
References: <CAAvxfHdBPOyEGS7s+xf4iaW0-cgiq25jpYdWBqQqvLtLe_t6tw@mail.gmail.com>
	<ZmZbXK1CNMbkty0W@nathan>

Nathan Bossart <[email protected]> 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.

BTW, while I approve of trying to get rid of our need for -fwrapv,
I'm quite scared of actually doing it.  Whatever cases you may have
discovered by running the regression tests will be at best the
tip of the iceberg.  Is there any chance of using static analysis
to find all the places of concern?

			regards, tom lane

view thread (4+ messages)

reply

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: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Remove dependence on integer wrapping
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox