public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tom Lane <[email protected]>
To: Joseph Koshakow <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Fix overflow in DecodeInterval
Date: Sat, 02 Apr 2022 13:20:56 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CAAvxfHff0JLYHwyBrtMx_=6wr=k2Xp+D+-X3vEhHjJYMj+mQcg@mail.gmail.com>
<[email protected]>
<CAAvxfHd-DZzRC9wROFbsZq=8zK+R-5G7DAoZ-oUK8n09cGA0eA@mail.gmail.com>
<CAAvxfHddHqA=dDKrbi+d+vo52j=sUwVt6_PYOsJGo+Eq91v4=g@mail.gmail.com>
<[email protected]>
<CAAvxfHdrQc8pVoSnTPBfTKzHQzmYTg1qheNxMLoWDcrWpTJapg@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAAvxfHd3gsOkGsbu5AVG+ayHiqsWzuZsi8WknL7v4C_A_4TwOQ@mail.gmail.com>
<CAAvxfHeNNjsJarnzVOycP__T2x4iwbbSWBDaDePcuBjz8--x6Q@mail.gmail.com>
<[email protected]>
<CAAvxfHfKYHw8n19s7eAcDo_B2T1SS22O1p0X4OE5_WBK230O+Q@mail.gmail.com>
<CAAvxfHduBhZdUXLFwLOSGW=0c17frLojcbCqNsZrw5n73pekZg@mail.gmail.com>
<CAAvxfHdVaHrriTWyvsLg3POX13rOyUGysur59sOT02qmmTq0Rg@mail.gmail.com>
<[email protected]>
<CAAvxfHdQGiX2xC78y3Y_LTdCiTOWtMXkaLai7s7-mG+=H7qNXA@mail.gmail.com>
<CAAvxfHd+8GAZSPxcuJa+Z3TG2! [email protected]>
<[email protected]>
I wrote:
> ... I almost feel that this is
> committable, but there is one thing that is bothering me. The
> part of DecodeInterval that does strange things with signs in the
> INTSTYLE_SQL_STANDARD case (starting about line 3400 in datetime.c
> before this patch, or line 3600 after) used to separately force the
> hour, minute, second, and microsecond fields to negative.
> Now it forces the merged tm_usec field to negative. It seems to
> me that this could give a different answer than before, if the
> h/m/s/us values had been of different signs before they got merged.
> However, I don't think that that situation is possible in SQL-spec-
> compliant input, so it may not be a problem. Again, a counterexample
> would be interesting.
As best I can tell, the case isn't reachable with spec-compliant input,
but it's easy to demonstrate an issue if you set intervalstyle to
sql_standard and then put in Postgres-format input. Historically,
you got
regression=# show intervalstyle;
IntervalStyle
---------------
postgres
(1 row)
regression=# select '-23 hours 45 min 12.34 sec'::interval;
interval
--------------
-22:14:47.66
(1 row)
(because by default the field signs are taken as independent)
regression=# set intervalstyle = sql_standard ;
SET
regression=# select '-23 hours 45 min 12.34 sec'::interval;
interval
--------------
-23:45:12.34
(1 row)
However, with this patch both cases produce "-22:14:47.66",
because we already merged the differently-signed fields and
DecodeInterval can't tease them apart again. Perhaps we could
get away with changing this nonstandard corner case, but I'm
pretty uncomfortable with that thought --- I don't think
random semantics changes are within the charter of this patch.
I think the patch can be salvaged, though. I like the concept
of converting all the sub-day fields to microseconds immediately,
because it avoids a host of issues, so I don't want to give that up.
What I'm going to look into is detecting the sign-adjustment-needed
case up front (which is easy enough, since it's looking at the
input data not the conversion results) and then forcing the
individual field values negative before we accumulate them into
the pg_itm_in struct.
Meanwhile, the fact that we didn't detect this issue immediately
shows that there's a gap in our regression tests. So the *first*
thing I'm gonna do is push a patch to add test cases like what
I showed above.
regards, tom lane
view thread (20+ messages) latest in thread
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]
Subject: Re: Fix overflow in DecodeInterval
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