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
