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.
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