Re: Infinite loop for generate_series with timestamp arguments - Mailing list pgsql-general

I wrote:
> We could perhaps do better by doing the initial addition of the
> interval and seeing if that produces a value greater than, less
> than, or equal to the start timestamp.  But I'm afraid that
> doesn't move the goalposts very far, because as this example
> shows, we might get different results in different months.

> Another idea is to check, after doing each addition, to make
> sure that the timestamp actually advanced in the expected
> direction.  But should we error out if not, or just stop?

Here's a very POC-y patch using both of these ideas (and
choosing to error out if the interval changes sign).
If we go this way, generate_series_timestamptz would need
similar changes, and some regression test cases would be
appropriate.  I'm not sure if the documentation needs
adjustment; it doesn't talk about the difficulty of
identifying the sign of an interval.

            regards, tom lane

diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 9682f9dbdca..202bbd1edcd 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -6622,6 +6622,7 @@ generate_series_timestamp(PG_FUNCTION_ARGS)
     FuncCallContext *funcctx;
     generate_series_timestamp_fctx *fctx;
     Timestamp    result;
+    Timestamp    nextval = 0;

     /* stuff done only on the first call of the function */
     if (SRF_IS_FIRSTCALL())
@@ -6651,18 +6652,25 @@ generate_series_timestamp(PG_FUNCTION_ARGS)
         fctx->finish = finish;
         fctx->step = *step;

-        /* Determine sign of the interval */
-        fctx->step_sign = interval_sign(&fctx->step);
-
-        if (fctx->step_sign == 0)
+        if (INTERVAL_NOT_FINITE((&fctx->step)))
             ereport(ERROR,
                     (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                     errmsg("step size cannot equal zero")));
+                     errmsg("step size cannot be infinite")));

-        if (INTERVAL_NOT_FINITE((&fctx->step)))
+        /*
+         * Compute the next series value so that we can identify the step
+         * direction.  This seems more reliable than trusting interval_sign().
+         */
+        nextval =
+            DatumGetTimestamp(DirectFunctionCall2(timestamp_pl_interval,
+                                                  TimestampGetDatum(start),
+                                                  PointerGetDatum(&fctx->step)));
+        fctx->step_sign = timestamp_cmp_internal(nextval, start);
+
+        if (fctx->step_sign == 0)
             ereport(ERROR,
                     (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                     errmsg("step size cannot be infinite")));
+                     errmsg("step size cannot equal zero")));

         funcctx->user_fctx = fctx;
         MemoryContextSwitchTo(oldcontext);
@@ -6682,9 +6690,18 @@ generate_series_timestamp(PG_FUNCTION_ARGS)
         timestamp_cmp_internal(result, fctx->finish) >= 0)
     {
         /* increment current in preparation for next iteration */
-        fctx->current = DatumGetTimestamp(DirectFunctionCall2(timestamp_pl_interval,
-                                                              TimestampGetDatum(fctx->current),
-                                                              PointerGetDatum(&fctx->step)));
+        if (nextval)
+            fctx->current = nextval;    /* already calculated it */
+        else
+            fctx->current =
+                DatumGetTimestamp(DirectFunctionCall2(timestamp_pl_interval,
+                                                      TimestampGetDatum(result),
+                                                      PointerGetDatum(&fctx->step)));
+        /* check for directional instability */
+        if (fctx->step_sign != timestamp_cmp_internal(fctx->current, result))
+            ereport(ERROR,
+                    (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                     errmsg("step size changed sign")));

         /* do when there is more left to send */
         SRF_RETURN_NEXT(funcctx, TimestampGetDatum(result));