Re: Infinite Interval - Mailing list pgsql-hackers

On Mon, Mar 20, 2023 at 3:16 AM Joseph Koshakow <koshy44@gmail.com> wrote:
>
>
>
> On Sun, Mar 19, 2023 at 5:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> >    Did you actually write "if TIMESTAMP_IS_NOBEGIN(dt2)" and not
> >    "if (TIMESTAMP_IS_NOBEGIN(dt2))"?  If the former, I'm not surprised
> >    that pgindent gets confused.  The parentheses are required by the
> >    C standard.  Your code might accidentally work because the macro
> >    has parentheses internally, but call sites have no business
> >    knowing that.  For example, it would be completely legit to change
> >    TIMESTAMP_IS_NOBEGIN to be a plain function, and then this would be
> >    syntactically incorrect.
>
> Oh duh. I've been doing too much Rust development and did this without
> thinking. I've attached a patch with a fix.
>

Thanks for fixing this.

On this latest patch, I have one code comment

@@ -3047,7 +3180,30 @@ timestamptz_pl_interval_internal(TimestampTz timestamp,
    TimestampTz result;
    int         tz;

-   if (TIMESTAMP_NOT_FINITE(timestamp))
+   /*
+    * Adding two infinites with the same sign results in an infinite
+    * timestamp with the same sign. Adding two infintes with different signs
+    * results in an error.
+    */
+   if (INTERVAL_IS_NOBEGIN(span))
+   {
+       if TIMESTAMP_IS_NOEND(timestamp)
+           ereport(ERROR,
+                   (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                    errmsg("interval out of range")));
+       else
+           TIMESTAMP_NOBEGIN(result);
+   }
+   else if (INTERVAL_IS_NOEND(span))
+   {
+       if TIMESTAMP_IS_NOBEGIN(timestamp)
+           ereport(ERROR,
+                   (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                    errmsg("interval out of range")));
+       else
+           TIMESTAMP_NOEND(result);
+   }
+   else if (TIMESTAMP_NOT_FINITE(timestamp))

This code is duplicated in timestamp_pl_interval(). We could create a function
to encode the infinity handling rules and then call it in these two places. The
argument types are different, Timestamp and TimestampTz viz. which map to in64,
so shouldn't be a problem. But it will be slightly unreadable. Or use macros
but then it will be difficult to debug.

What do you think?

Next I will review the test changes and also make sure that every
operator that interval as one of its operands or the result has been
covered in the code. This is the following list

#select oprname, oprcode from pg_operator where oprleft =
'interval'::regtype or oprright = 'interval'::regtype or oprresult =
'interval'::regtype;
 oprname |         oprcode
---------+-------------------------
 +       | date_pl_interval
 -       | date_mi_interval
 +       | timestamptz_pl_interval
 -       | timestamptz_mi
 -       | timestamptz_mi_interval
 =       | interval_eq
 <>      | interval_ne
 <       | interval_lt
 <=      | interval_le
 >       | interval_gt
 >=      | interval_ge
 -       | interval_um
 +       | interval_pl
 -       | interval_mi
 -       | time_mi_time
 *       | interval_mul
 *       | mul_d_interval
 /       | interval_div
 +       | time_pl_interval
 -       | time_mi_interval
 +       | timetz_pl_interval
 -       | timetz_mi_interval
 +       | interval_pl_time
 +       | timestamp_pl_interval
 -       | timestamp_mi
 -       | timestamp_mi_interval
 +       | interval_pl_date
 +       | interval_pl_timetz
 +       | interval_pl_timestamp
 +       | interval_pl_timestamptz
(30 rows)

--
Best Wishes,
Ashutosh Bapat