Re: Infinite Interval

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(at)gmail(dot)com> wrote:
>
> On Wed, Mar 1, 2023 at 3:03 PM Gregory Stark (as CFM) <stark(dot)cfm(at)gmail(dot)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