Subject: Re: Infinite loop for generate_series with timestamp arguments

From: Tom Lane <[email-masked]>
Date: Mon, 03 Mar 2025 14:10:46 -0500

Jakob Teuber <[email-masked]> writes:
> I noticed, that the following query will send Postgres into an infinite 
> loop:
> (a)  select generate_series(timestamp '2025-01-30', timestamp 
> '2025-02-01', interval '1 month -29 days');
> One could certainly argue that “go and do an infinite loop” is plainly 
> the intended semantics of this query, but in other cases, Postgres tries 
> to guard against these types of looping generate_series statements:
> (b)  select generate_series(timestamp '2025-03-31', timestamp 
> '2025-04-01', interval '1 month -30 days');
>          → ERROR:  step size cannot equal zero

It does what it can, but there's no chance of being entirely perfect.
The core problem here is to figure out which direction the series
is intended to advance, so as to know whether the termination test
should be "less than" or "greater than" the end timestamp.  The
way generate_series_timestamp does that is to see whether
interval comparison thinks the interval value is less than or
greater than a zero interval --- and if it happens to be exactly
equal, you get the whine about zero step size.  But interval
comparison is far from bright:

 * Interval comparison is based on converting interval values to a linear
 * representation expressed in the units of the time field (microseconds,
 * in the case of integer timestamps) with days assumed to be always 24 hours
 * and months assumed to be always 30 days.

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?

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Mon, 03 Mar 2025 16:30:21 -0500

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