Re: Infinite Interval - Mailing list pgsql-hackers

Hi Joseph,

Thanks for working on the patch. Sorry for taking so long to review
this patch. But here's it finally, review of code changes

 static pg_tz *FetchDynamicTimeZone(TimeZoneAbbrevTable *tbl, const datetkn *tp,
-                                   DateTimeErrorExtra *extra);
+                                   DateTimeErrorExtra * extra);

There are a lot of these diffs. PG code doesn't leave an extra space between
variable name and *.


     /* Handle the integer part */
-    if (!int64_multiply_add(val, scale, &itm_in->tm_usec))
+    if (pg_mul_add_s64_overflow(val, scale, &itm_in->tm_usec))

I think this is a good change, since we are moving the function to int.h where
it belongs. We could separate these kind of changes into another patch for easy
review.

+
+    result->day = days;
+    if (pg_mul_add_s32_overflow(weeks, 7, &result->day))
+        ereport(ERROR,
+                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                 errmsg("interval out of range")));

I think such changes are also good, but probably a separate patch for ease of
review.

     secs = rint(secs * USECS_PER_SEC);
-    result->time = hours mj* ((int64) SECS_PER_HOUR * USECS_PER_SEC) +
-        mins * ((int64) SECS_PER_MINUTE * USECS_PER_SEC) +
-        (int64) secs;
+
+    result->time = secs;
+    if (pg_mul_add_s64_overflow(mins, ((int64) SECS_PER_MINUTE *
USECS_PER_SEC), &result->time))
+        ereport(ERROR,
+                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                 errmsg("interval out of range")));
+    if (pg_mul_add_s64_overflow(hours, ((int64) SECS_PER_HOUR *
USECS_PER_SEC), &result->time))
+        ereport(ERROR,
+                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                 errmsg("interval out of range")));

    shouldn't this be
    secs = rint(secs);
    result->time = 0;
    pg_mul_add_s64_overflow(secs, USECS_PER_SEC, &result->time) to catch
    overflow error early?

+        if TIMESTAMP_IS_NOBEGIN
+            (dt2)

Better be written as if (TIMESTAMP_IS_NOBEGIN(dt2))? There are more corrections
like this.

+    if (INTERVAL_NOT_FINITE(result))
+        ereport(ERROR,
+                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                 errmsg("interval out of range")));

Probably, I added these kind of checks. But I don't remember if those are
defensive checks or whether it's really possible that the arithmetic above
these lines can yield an non-finite interval.


+    else
+    {
+        result->time = -interval->time;
+        result->day = -interval->day;
+        result->month = -interval->month;
+
+        if (INTERVAL_NOT_FINITE(result))
+            ereport(ERROR,
+                    (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                     errmsg("interval out of range")));

If this error ever gets to the user, it could be confusing. Can we elaborate by
adding context e.g. errcontext("while negating an interval") or some such?

-
-    result->time = -interval->time;
-    /* overflow check copied from int4um */
-    if (interval->time != 0 && SAMESIGN(result->time, interval->time))
-        ereport(ERROR,
-                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
-                 errmsg("interval out of range")));
-    result->day = -interval->day;
-    if (interval->day != 0 && SAMESIGN(result->day, interval->day))
-        ereport(ERROR,
-                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
-                 errmsg("interval out of range")));
-    result->month = -interval->month;
-    if (interval->month != 0 && SAMESIGN(result->month, interval->month))
-        ereport(ERROR,
-                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
-                 errmsg("interval out of range")));
+    interval_um_internal(interval, result);

Shouldn't we incorporate these checks in interval_um_internal()? I don't think
INTERVAL_NOT_FINITE() covers all of those.

+    /*
+     * Subtracting two infinite intervals with different signs results in an
+     * infinite interval with the same sign as the left operand. Subtracting
+     * two infinte intervals with the same sign results in an error.
+     */

I think we need someone to validate these assumptions and similar assumptions
in interval_pl(). Googling gives confusing results in some cases. I have not
looked for IEEE standard around this specificallly.

+    if (INTERVAL_NOT_FINITE(interval))
+    {
+        double        r = NonFiniteIntervalPart(type, val, lowunits,
+                                              INTERVAL_IS_NOBEGIN(interval),
+                                              false);
+
+        if (r)

I see that this code is very similar to the corresponding code in timestamp and
timestamptz, so it's bound to be correct. But I always thought float equality
is unreliable. if (r) is equivalent to if (r == 0.0) so it will not work as
intended. But may be (float) 0.0 is a special value for which equality holds
true.

+static inline bool
+pg_mul_add_s64_overflow(int64 val, int64 multiplier, int64 *sum)

I think this needs a prologue similar to int64_multiply_add(), that the patch
removes. Similarly for pg_mul_add_s32_overflow().

On Thu, Mar 2, 2023 at 3:51 AM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> On Wed, Mar 1, 2023 at 3:03 PM Gregory Stark (as CFM) <stark.cfm@gmail.com> wrote:
> >
> >    It looks like this patch needs a (perhaps trivial) rebase.
>
> Attached is a rebased patch.
>
> >    It sounds like all the design questions are resolved so perhaps this
> >    can be set to Ready for Committer once it's rebased?
>
> There hasn't really been a review of this patch yet. It's just been
> mostly me talking to myself in this thread, and a couple of
> contributions from jian.
>
> - Joe Koshakow



--
Best Wishes,
Ashutosh Bapat