Horton

Thread

Commits

  1. Detect overflow in timestamp[tz] subtraction.

  1. Silent overflow of interval type

    Nick Babadzhanian <pgnickb@gmail.com> — 2023-02-15T15:07:45Z

    Hello hackers,

I've been testing various edge-cases of timestamptz and related types
and noticed that despite being a 16-byte wide type, interval overflows
for some timestamptz (8-byte) subtractions (timestamp_mi).
A simple example of this would be:

select timestamptz'294276-12-31 23:59:59 UTC' - timestamptz'1582-10-15
00:00:00 UTC';

Yielding:
interval'-106599615 days -08:01:50.551616'

This makes sense from the implementation point of view, since both
timestamptz and Interval->TimeOffset are int64.

The patch attached simply throws an error when an overflow is
detected. However I'm not sure this is a reasonable approach for a
code path that could be very hot in some workloads. Another
consideration is that regardless of the values of the timestamps, the
absolute value of the difference can be stored in a uint64. However
that observation has little practical value.

That being said I'm willing to work on a fix that makes sense and
making it commit ready (or step aside if someone else wants to take
over) but I'd also understand if this is marked as "not worth fixing".

Regards,
Nick

  2. Re: Silent overflow of interval type

    Andrey Borodin <amborodin86@gmail.com> — 2023-02-15T20:24:24Z

    On Wed, Feb 15, 2023 at 7:08 AM Nikolai <pgnickb@gmail.com> wrote:
>
> The patch attached simply throws an error when an overflow is
> detected. However I'm not sure this is a reasonable approach for a
> code path that could be very hot in some workloads.

Given the extraordinary amount of overflow checks in the nearby code
of timestamp.c, I'd say that this case should not be an exception.
By chance did you look at all other nearby cases, is it the only place
with overflow? (I took a look too, but haven't found anything
suspicious)

Best regards, Andrey Borodin.

  3. Re: Silent overflow of interval type

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-16T00:12:45Z

    Andrey Borodin <amborodin86@gmail.com> writes:
> On Wed, Feb 15, 2023 at 7:08 AM Nikolai <pgnickb@gmail.com> wrote:
>> The patch attached simply throws an error when an overflow is
>> detected. However I'm not sure this is a reasonable approach for a
>> code path that could be very hot in some workloads.

> Given the extraordinary amount of overflow checks in the nearby code
> of timestamp.c, I'd say that this case should not be an exception.

Yeah, I don't think this would create a performance problem, at least not
if you're using a compiler that implements pg_sub_s64_overflow reasonably.
(And if you're not, and this bugs you, the answer is to get a better
compiler.)

> By chance did you look at all other nearby cases, is it the only place
> with overflow?

That was my immediate reaction as well.

			regards, tom lane

  4. Re: Silent overflow of interval type

    Nick Babadzhanian <pgnickb@gmail.com> — 2023-02-16T13:00:23Z

    On Thu, Feb 16, 2023 at 1:12 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah, I don't think this would create a performance problem, at least not
> if you're using a compiler that implements pg_sub_s64_overflow reasonably.
> (And if you're not, and this bugs you, the answer is to get a better

Please find attached the v2 of the said patch with the tests added. I
tested and it applies with all tests passing both on REL_14_STABLE,
REL_15_STABLE and master. I don't know how the decision on
backpatching is made and whether it makes sense here or not. If any
additional work is required, please let me know.

> By chance did you look at all other nearby cases, is it the only place
> with overflow?

Not really, no. The other place where it could overflow was in the
interval justification function and it was fixed about a year ago.
That wasn't backpatched afaict. See
https://postgr.es/m/CAAvxfHeNqsJ2xYFbPUf_8nNQUiJqkag04NW6aBQQ0dbZsxfWHA@mail.gmail.com

Regards,
Nick

  5. Re: Silent overflow of interval type

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-20T22:30:56Z

    Nick Babadzhanian <pgnickb@gmail.com> writes:
> Please find attached the v2 of the said patch with the tests added.

Pushed with light editing (for instance, I don't think interval.sql
is the place to test timestamp operators, even if the result is an
interval).

> I don't know how the decision on
> backpatching is made and whether it makes sense here or not.

We haven't got a really hard policy on that, but in this case
I elected not to, because it didn't seem worth the effort.
It seems fairly unlikely that people would hit this in production.
Also there's the precedent that related changes weren't backpatched.

			regards, tom lane