public inbox for [email protected]
help / color / mirror / Atom feedRe: Infinite loop for generate_series with timestamp arguments
2+ messages / 1 participants
[nested] [flat]
* Re: Infinite loop for generate_series with timestamp arguments
@ 2025-03-03 19:10 Tom Lane <[email protected]>
2025-03-03 21:30 ` Re: Infinite loop for generate_series with timestamp arguments Tom Lane <[email protected]>
0 siblings, 1 reply; 2+ messages in thread
From: Tom Lane @ 2025-03-03 19:10 UTC (permalink / raw)
To: Jakob Teuber <[email protected]>; +Cc: [email protected]
Jakob Teuber <[email protected]> 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
^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: Infinite loop for generate_series with timestamp arguments
2025-03-03 19:10 Re: Infinite loop for generate_series with timestamp arguments Tom Lane <[email protected]>
@ 2025-03-03 21:30 ` Tom Lane <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Tom Lane @ 2025-03-03 21:30 UTC (permalink / raw)
To: Jakob Teuber <[email protected]>; +Cc: [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));
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2025-03-03 21:30 UTC | newest]
Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-03-03 19:10 Re: Infinite loop for generate_series with timestamp arguments Tom Lane <[email protected]>
2025-03-03 21:30 ` Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox