On Sun, Mar 19, 2023 at 12:18 AM Joseph Koshakow <kosh...@gmail.com> wrote: > > The problem is that `secs = rint(secs)` rounds the seconds too early > and loses any fractional seconds. Do we have an overflow detecting > multiplication function for floats?
We have float8_mul() which checks for overflow. typedef double float8; > > > + 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. > > These checks appear in `make_interval`, `justify_X`, > `interval_um_internal`, `interval_pl`, `interval_mi`, `interval_mul`, > `interval_div`. For all of these it's possible that the interval > overflows/underflows the non-finite ranges, but does not > overflow/underflow the data type. For example > `SELECT INTERVAL '2147483646 months' + INTERVAL '1 month'` would error > on this check. Without this patch postgres@64807=#SELECT INTERVAL '2147483646 months' + INTERVAL '1 month'; ?column? ------------------------ 178956970 years 7 mons (1 row) That result looks correct postgres@64807=#select 178956970 * 12 + 7; ?column? ------------ 2147483647 (1 row) So some backward compatibility break. I don't think we can avoid the backward compatibility break without expanding interval structure and thus causing on-disk breakage. But we can reduce the chances of breaking, if we change INTERVAL_NOT_FINITE to check all the three fields, instead of just month. > > > > + 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? > > Done. Thanks. Can we add relevant contexts at similar other places? Also if we use all the three fields, we will need to add such checks in interval_justify_hours() > > I replaced these checks with the following: > > + else if (interval->time == PG_INT64_MIN || interval->day == PG_INT32_MIN || > interval->month == PG_INT32_MIN) > + ereport(ERROR, > + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), > + errmsg("interval out of range"))); > > I think this covers the same overflow check but is maybe a bit more > obvious. Unless, there's something I'm missing? Thanks. Your current version is closer to int4um(). Some more review comments in the following email. -- Best Wishes, Ashutosh Bapat