Re: Infinite loop for generate_series with timestamp arguments

[Tom Lane <tgl@xxxxxxxxxxxxx>]

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));