On Wed, Sep 20, 2023 at 8:23 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Wed, 20 Sept 2023 at 15:00, Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.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