From: Tom Lane <tgl@sss.pgh.pa.us>
To: Jakob Teuber <jakob.teuber@tum.de>
cc: pgsql-general@lists.postgresql.org
Subject: Re: Infinite loop for generate_series with timestamp arguments
In-reply-to: <4063619.1741029046@sss.pgh.pa.us>
References: <05676a44-9408-4c15-9309-c0f39ecd511a@tum.de> <4063619.1741029046@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Mon, 03 Mar 2025 14:10:46 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <4080959.1741037382.0@sss.pgh.pa.us>
Date: Mon, 03 Mar 2025 16:30:21 -0500
Message-ID: <4081027.1741037421@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/4081027.1741037421%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4080959.1741037382.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="guard-against-interval-sign-changes-wip.patch";
	charset="us-ascii"
Content-ID: <4080959.1741037382.2@sss.pgh.pa.us>
Content-Description: guard-against-interval-sign-changes-wip.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/tim=
estamp.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 =3D 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 =3D finish;
 		fctx->step =3D *step;
 =

-		/* Determine sign of the interval */
-		fctx->step_sign =3D interval_sign(&fctx->step);
-
-		if (fctx->step_sign =3D=3D 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 =3D
+			DatumGetTimestamp(DirectFunctionCall2(timestamp_pl_interval,
+												  TimestampGetDatum(start),
+												  PointerGetDatum(&fctx->step)));
+		fctx->step_sign =3D timestamp_cmp_internal(nextval, start);
+
+		if (fctx->step_sign =3D=3D 0)
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("step size cannot be infinite")));
+					 errmsg("step size cannot equal zero")));
 =

 		funcctx->user_fctx =3D fctx;
 		MemoryContextSwitchTo(oldcontext);
@@ -6682,9 +6690,18 @@ generate_series_timestamp(PG_FUNCTION_ARGS)
 		timestamp_cmp_internal(result, fctx->finish) >=3D 0)
 	{
 		/* increment current in preparation for next iteration */
-		fctx->current =3D DatumGetTimestamp(DirectFunctionCall2(timestamp_pl_in=
terval,
-															  TimestampGetDatum(fctx->current),
-															  PointerGetDatum(&fctx->step)));
+		if (nextval)
+			fctx->current =3D nextval;	/* already calculated it */
+		else
+			fctx->current =3D
+				DatumGetTimestamp(DirectFunctionCall2(timestamp_pl_interval,
+													  TimestampGetDatum(result),
+													  PointerGetDatum(&fctx->step)));
+		/* check for directional instability */
+		if (fctx->step_sign !=3D 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));

------- =_aaaaaaaaaa0--