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