Re: Unexpected interval comparison

Hello,

At Tue, 21 Mar 2017 07:52:25 -0700, Adrian Klaver <adrian.klaver@aklaver.com> wrote in
<375c9e5a-960f-942c-913f-55632a1f0a90@aklaver.com>
> On 03/21/2017 07:42 AM, Tom Lane wrote:
> > Frazer McLean <frazer@frazermclean.co.uk> writes:
> >> I came across an unexpected comparison (tested on PostgreSQL 9.4 and
> >> 9.6) for intervals with a large difference in magnitude.
> >
> >>   '1 year'::interval > '32618665 years'::interval;
> >
> >> Is this a bug?
> >
> > It looks like the problem is overflow of the result of
> > interval_cmp_value,
> > because it's trying to compute
> >
> > =# select '32618665'::int8 * 30 * 86400 * 1000000;
> > ERROR:  bigint out of range
> >
> > It's not immediately obvious how to avoid that while preserving the
> > same
> > comparison semantics :-(

This is an apparent bug of interval comparison. During comparison
interval is converted into int64 in milliseconds but it overflows
in the case.

Detecting the overflow during the conversion can fix it and
preserving the semantics (except value range). The current code
tells a lie anyway for the cases but I'm not sure limting the
range of value is acceptable or not.

| =# select '106751990 days 24:59:59'::interval;
|         interval
| -------------------------
|  106751990 days 24:59:59
| =# select '106751990 days 24:59:59'::interval > '1 year'::interval;
| ERROR:  interval out of range during comparison

If this is not acceptable, some refactoring would be required.


> Not sure if it helps but this works:
>
> test=# select extract(epoch from '1 year'::interval) > extract(epoch
> from '32618665 years'::interval);
>  ?column?
> ----------
>  f

It calculates in seconds. So it is useful if subseconds are not
significant. But extract also silently overflows during
converting the same interval to usecs. This seems to need the
same amendment.

> =# select extract(usec from '32618665 years'::interval);
>  date_part
> -----------
>          0

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 4be1999..f77cfcc 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -2293,10 +2293,23 @@ static inline TimeOffset
 interval_cmp_value(const Interval *interval)
 {
     TimeOffset    span;
+    TimeOffset timedayfraction;
+    TimeOffset timedays;

-    span = interval->time;
-    span += interval->month * INT64CONST(30) * USECS_PER_DAY;
-    span += interval->day * INT64CONST(24) * USECS_PER_HOUR;
+    timedays = ((int64)interval->time) / USECS_PER_DAY;
+    timedayfraction = interval->time - timedays * USECS_PER_DAY;
+
+    /* calculate span in days. this cannot overflow */
+    span = timedays;
+    span += interval->month * INT64CONST(30);
+    span += interval->day;
+
+    /* overflow check */
+    if (span > INT64CONST(0x7fffffffffffffff) / USECS_PER_DAY - 1)
+        ereport(ERROR,
+              (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+               errmsg ("interval out of range during comparison")));
+    span = span * USECS_PER_DAY + timedayfraction;

     return span;
 }
@@ -2304,6 +2317,7 @@ interval_cmp_value(const Interval *interval)
 static int
 interval_cmp_internal(Interval *interval1, Interval *interval2)
 {
+
     TimeOffset    span1 = interval_cmp_value(interval1);
     TimeOffset    span2 = interval_cmp_value(interval2);