From: | Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> |
---|---|
To: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> |
Cc: | jian he <jian(dot)universality(at)gmail(dot)com>, Joseph Koshakow <koshy44(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "Gregory Stark (as CFM)" <stark(dot)cfm(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Infinite Interval |
Date: | 2023-09-21 13:37:04 |
Message-ID: | CAExHW5uE4VxdU02oh8xOUPL48=Xf1esKpQ1sVzfp58qt0GT9ng@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Sep 20, 2023 at 8:23 PM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
> On Wed, 20 Sept 2023 at 15:00, Ashutosh Bapat
> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> >
> > 0005 - Refactored Jian's code fixing window functions. Does not
> > contain the changes for serialization and deserialization. Jian,
> > please let me know if I have missed anything else.
> >
>
> That looks a lot neater. One thing I don't care for is this code
> pattern in finite_interval_pl():
>
> + result->month = span1->month + span2->month;
> + /* overflow check copied from int4pl */
> + if (SAMESIGN(span1->month, span2->month) &&
> + !SAMESIGN(result->month, span1->month))
> + ereport(ERROR,
>
> The problem is that this is a bug waiting to happen for anyone who
> uses this function with "result" pointing to the same Interval struct
> as "span1" or "span2". I understand that the current code avoids this
> by careful use of temporary Interval structs, but it's still a pretty
> ugly pattern. This can be avoided by using pg_add_s32/64_overflow(),
> which then allows the callers to be simplified, getting rid of the
> temporary Interval structs and memcpy()'s.
That's a good idea. Done.
>
> Also, in do_interval_discard(), this seems a bit risky:
>
> + neg_val.day = -newval->day;
> + neg_val.month = -newval->month;
> + neg_val.time = -newval->time;
>
> because it could in theory turn a finite large negative interval into
> an infinite one (-INT_MAX -> INT_MAX), leading to an assertion failure
> in finite_interval_pl(). Now maybe that's not possible for some other
> reasons, but I think we may as well do the same refactoring for
> interval_mi() as we're doing for interval_pl() -- i.e., introduce a
> finite_interval_mi() function, making the addition and subtraction
> code match, and removing the need for neg_val in
> do_interval_discard().
Your suspicion is correct. It did throw an error. Added tests for the
same. Introduced finite_interval_mi() which uses
pg_sub_s32/s64_overflow() functions.
I will send updated patches with my next reply.
--
Best Wishes,
Ashutosh Bapat
From | Date | Subject | |
---|---|---|---|
Next Message | Ashutosh Bapat | 2023-09-21 13:51:30 | Re: Infinite Interval |
Previous Message | Hayato Kuroda (Fujitsu) | 2023-09-21 13:24:45 | RE: [PoC] pg_upgrade: allow to upgrade publisher node |