Thread: Fix overflow in justify_interval related functions
I mentioned this issue briefly in another thread, but the justify_interval, justify_days, and justify_hours functions all have the potential to overflow. The attached patch fixes this issue. Cheers, Joe Koshakow
Attachment
On Sun, Feb 13, 2022 at 01:28:38PM -0500, Joseph Koshakow wrote: > +SELECT justify_hours(interval '2147483647 days 24 hrs'); > +ERROR: interval out of range The docs [0] claim that the maximum value for interval is 178 million years, but this test case is only ~6 million. Should we instead rework the logic to avoid overflow for this case? [0] https://www.postgresql.org/docs/devel/datatype-datetime.html -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
> On Sun, Feb 13, 2022 at 01:28:38PM -0500, Joseph Koshakow wrote:
>> +SELECT justify_hours(interval '2147483647 days 24 hrs');
>> +ERROR: interval out of range
> The docs [0] claim that the maximum value for interval is 178 million
> years, but this test case is only ~6 million. Should we instead rework the
> logic to avoid overflow for this case?
I think the docs are misleading you on this point. The maximum
value of the months part of an interval is 2^31 months or
about 178Myr, but what we're dealing with here is days, which
likewise caps at 2^31 days. justify_hours is not chartered
to transpose up to months, so it can't avoid that limit.
regards, tom lane
On Mon, Feb 14, 2022 at 01:55:56PM -0500, Tom Lane wrote: > Nathan Bossart <nathandbossart@gmail.com> writes: >> On Sun, Feb 13, 2022 at 01:28:38PM -0500, Joseph Koshakow wrote: >>> +SELECT justify_hours(interval '2147483647 days 24 hrs'); >>> +ERROR: interval out of range > >> The docs [0] claim that the maximum value for interval is 178 million >> years, but this test case is only ~6 million. Should we instead rework the >> logic to avoid overflow for this case? > > I think the docs are misleading you on this point. The maximum > value of the months part of an interval is 2^31 months or > about 178Myr, but what we're dealing with here is days, which > likewise caps at 2^31 days. justify_hours is not chartered > to transpose up to months, so it can't avoid that limit. Makes sense. So we could likely avoid it for justify_interval, but the others are at the mercy of the interval implementation. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Mon, Feb 14, 2022 at 2:15 PM Nathan Bossart <nathandbossart@gmail.com> wrote: > Makes sense. So we could likely avoid it for justify_interval, but the > others are at the mercy of the interval implementation. I'm not entirely sure what you mean by "it", but for both justify_interval and justify_days this commit throws an error if we try to set the months field above 2^31. > +SELECT justify_days(interval '2147483647 months 30 days'); > +ERROR: interval out of range > +SELECT justify_interval(interval '2147483647 months 30 days'); > +ERROR: interval out of range That's what these are testing. - Joe Koshakow
On Mon, Feb 14, 2022 at 04:57:07PM -0500, Joseph Koshakow wrote: > On Mon, Feb 14, 2022 at 2:15 PM Nathan Bossart <nathandbossart@gmail.com> wrote: >> Makes sense. So we could likely avoid it for justify_interval, but the >> others are at the mercy of the interval implementation. > > I'm not entirely sure what you mean by "it", but for both > justify_interval and justify_days this commit throws an error if we > try to set the months field above 2^31. > >> +SELECT justify_days(interval '2147483647 months 30 days'); >> +ERROR: interval out of range >> +SELECT justify_interval(interval '2147483647 months 30 days'); >> +ERROR: interval out of range > > That's what these are testing. I think it's possible to avoid overflow in justify_interval() in some cases by pre-justifying the days. I've attached a patch to demonstrate. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Mon, Feb 14, 2022 at 7:59 PM Nathan Bossart <nathandbossart@gmail.com> wrote: > I think it's possible to avoid overflow in justify_interval() in some cases > by pre-justifying the days. I've attached a patch to demonstrate. > > -- > Nathan Bossart > Amazon Web Services: https://aws.amazon.com Good catch, I didn't think about that. Though if you are pre-justifying the days, then I don't think it's possible for the second addition to days to overflow. The maximum amount the days field could be after the first justification is 29. I think we can simplify it further to the attached patch. - Joe
Attachment
On Mon, Feb 14, 2022 at 08:35:43PM -0500, Joseph Koshakow wrote:
> Good catch, I didn't think about that. Though if you are pre-justifying
> the days, then I don't think it's possible for the second addition to
> days to overflow. The maximum amount the days field could be after
> the first justification is 29. I think we can simplify it further to the
> attached patch.
LGTM. I've marked this one as ready-for-committer. It's a little weird
that justify_hours() and justify_days() can overflow in cases where there
is still a valid interval representation, but as Tom noted, those functions
have specific charters to follow.
> + ereport(ERROR,
> + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
> + errmsg("interval out of range")));
nitpick: I think there is ordinarily an extra space before errmsg() so that
it lines up with errcode().
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Mon, Feb 14, 2022 at 11:23 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
> It's a little weird
> that justify_hours() and justify_days() can overflow in cases where there
> is still a valid interval representation, but as Tom noted, those functions
> have specific charters to follow.
Yes it is a bit weird, but this follows the same behavior as adding
Intervals. The following query overflows:
postgres=# SELECT interval '2147483647 days' + interval '1 day';
ERROR: interval out of range
Even though the following query does not:
postgres=# SELECT justify_days(interval '2147483647 days') + interval '1 day';
?column?
-----------------------------
5965232 years 4 mons 8 days
(1 row)
The reason is, as Tom mentioned, that Interval's months, days, and
time (microseconds) are stored separately. They are only combined
during certain scenarios such as testing for equality, ordering,
justify_* methods, etc. I think the idea behind it is that not every month
has 30 days and not every day has 24 hrs, though I'm not sure
.
> > + ereport(ERROR,
> > + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
> > + errmsg("interval out of range")));
>
> nitpick: I think there is ordinarily an extra space before errmsg() so that
> it lines up with errcode().
I've attached a patch to add the space. Thanks so much for your review
and comments!
- Joe Koshakow
Attachment
Just checking because I'm not very familiar with the process, are there any outstanding items that I need to do for this patch? - Joe Koshakow
On Fri, Feb 25, 2022 at 10:30:57AM -0500, Joseph Koshakow wrote: > Just checking because I'm not very familiar with the process, > are there any outstanding items that I need to do for this patch? Unless someone has additional feedback, I don't think so. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Joseph Koshakow <koshy44@gmail.com> writes:
> [ v4-0001-Check-for-overflow-in-justify_interval-functions.patch ]
Pushed. I added a comment explaining why the one addition in
interval_justify_interval doesn't require an overflow check.
regards, tom lane