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. 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(). Regards, Dean
