public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tom Lane <[email protected]>
To: Jakob Teuber <[email protected]>
Cc: [email protected]
Subject: Re: Infinite loop for generate_series with timestamp arguments
Date: Mon, 03 Mar 2025 16:30:21 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>

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



Attachments:

  [text/x-diff] guard-against-interval-sign-changes-wip.patch (2.5K, 2-guard-against-interval-sign-changes-wip.patch)
  download | inline diff:
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));

view thread (2+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Infinite loop for generate_series with timestamp arguments
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox