On Tue, Sep 1, 2026 at 6:40 PM Xuneng Zhou <[email protected]> wrote:
>
> On Wed, Sep 2, 2026 at 5:09 AM Masahiko Sawada <[email protected]> wrote:
> >
> > On Mon, Aug 31, 2026 at 6:17 PM Xuneng Zhou <[email protected]> wrote:
> > >
> > > On Mon, Aug 31, 2026 at 10:07 PM cca5507 <[email protected]> wrote:
> > > >
> > > > > --- The divergence
> > > > > Rounding like this does not make a lot of sense to me in the first
> > > > > place, especially when the value rounded down to is zero, which
> > > > > disables the timeout in lots of places. I don't know whether users
> > > > > have ever been surprised by this behavior and why it was designed like
> > > > > this. One reason for this seems to be that the value is small enough
> > > > > to do so.
> > > > >
> > > > > The general GUC doc says:
> > > > > fractional values are rounded to the nearest integer if the parameter
> > > > > is of integer type.
> > > > > If a fractional value is specified with a unit, it will be rounded to
> > > > > a multiple of the next smaller unit if there is one.
> > > > >
> > > > > However, the individual statement_timeout and lock_timeout
> > > > > descriptions do not clearly warn that a nonzero or negative spelling
> > > > > can round to zero and consequently disable the timeout. They only
> > > > > state that zero disables it.
> > > > >
> > > > > I am wondering whether the timeout in WAIT FOR needs to take a
> > > > > different path since it is not constrained with backward
> > > > > compatibility.
> > > >
> > > > How about just keeping it consistent with GUC: use parse_int() and
> > > > error out if timeout < 0. I didn't see users complaining about this
> > > > behavior.
> > >
> > > TBH, I am not a fan of this workaround unless we deal with the
> > > reported GUC issue later. The strange behaviors reported earlier seems
> > > relatively hard to hit since it requires relatively uncommon input and
> > > it is also not that easy to notice even if it is hit. But the
> > > operational consequence and the scope of affected GUCs seems
> > > non-trivial to me. That said, I don't have enough concentrated time to
> > > dig into the issue, figure out a proper fix and write it down for now,
> > > since I am on a vacation.
> >
> > I would prefer using parse_int() here.
> >
> > It does lead to some slightly surprising cases (for instance '-0.4ms'
> > converts to 0, which means waiting forever), including the case I
> > raised earlier in this thread. But existing integer GUC parameters
> > such as statement_timeout have been behaving that way, so I don't
> > think it would be a problem in practice. And if we fix the common
> > parsing infrastructure so that it doesn't produce such surprising
> > results, that would fix both places at once.
> >
> > Using parse_int() also addresses the reported overflow at its root,
> > since the value is then capped at INT_MAX milliseconds, which is all
> > that WaitLatch() accepts anyway.
> >
> > As for the currently proposed idea of adding our own checks and
> > rounding, while it would be flexible enough to deal with corner cases
> > that parse_int() and parse_real() don't handle today, I think the GUC
> > side would then have to be fixed as well, and PG19 is not the right
> > time to work on that.
>
> Agree. Maybe it is not a good time to do so for v19, which means that
> the timeout in wait for could be a new victim of that.
So I've reviewed the v6 patch, and here are some comments:
wait.c no longer calls rint() or isnan(), so we don't need to include math.h.
---
+ The valid range is <literal>0 .. INT_MAX</literal> milliseconds.
+ A value of zero means waiting indefinitely.
INT_MAX is a C identifier and it's better to avoid using it in that section.
Also, the current description "The timeout might be given as integer
number of milliseconds." seems not correct to me since pares_int()
falls back to strtod() when the value has a decimal point. ANd the
paragraph also doesn't mention the rounding. How about rewriting it to
something like:
<para>
When specified and <parameter>timeout</parameter> is
greater than zero,
the command waits until <parameter>lsn</parameter> is reached or
- the specified <parameter>timeout</parameter> has elapsed.
+ the specified <parameter>timeout</parameter> has elapsed. A value
+ of zero (the default) means the command waits indefinitely.
</para>
<para>
- The <parameter>timeout</parameter> might be given as
integer number of
- milliseconds. Also it might be given as string literal with
- integer number of milliseconds or a number with unit
- (see <xref linkend="config-setting-names-values"/>).
+ The <parameter>timeout</parameter> is an amount of time in
+ milliseconds. It may also be specified as a string containing the
+ numerical value followed by a time unit
+ (see <xref linkend="config-setting-names-values"/>). The maximum
+ value is <literal>2147483647 ms</literal>.
+ </para>
+ <para>
+ Fractional values are rounded to the nearest millisecond. Note
+ that a <parameter>timeout</parameter> of half a millisecond or
+ less therefore rounds down to zero, which means waiting
+ indefinitely.
</para>
I've attached the updated patch that incorporated the above points.
Please review it.
> I still think that it needs to be fixed in HEAD.
Agreed, and I'd rather we did that on its own thread.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
From 9de876a7ef09301bee4ddb4c22251fdd44794e62 Mon Sep 17 00:00:00 2001
From: ChangAo Chen <[email protected]>
Date: Thu, 10 Sep 2026 11:11:15 -0700
Subject: [PATCH v7] Fix timeout overflow in WAIT FOR LSN.
Commit 447aae13b03 accepted TIMEOUT values up to INT64_MAX
milliseconds, but computing the deadline multiplies by 1000, which
caused an int64 overflow. And the command reported a timeout at once
instead of waiting.
Parse the value as an int instead, which simplifies the code and is
consistent with other timeout values such as the statement_timeout GUC.
The specified values are now rounded to the nearest millisecond, so a
positive timeout below half a millisecond becomes zero and waits
indefinitely.
Backpatch to v19, where the TIMEOUT option was introduced.
Reported-by: ChangAo Chen <[email protected]>
Author: ChangAo Chen <[email protected]>
Reviewed-by: Xuneng Zhou <[email protected]>
Reviewed-by: Masahiko Sawada <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Backpatch-through: 19
---
doc/src/sgml/ref/wait_for.sgml | 18 ++++++++++----
src/backend/access/transam/xlogwait.c | 2 +-
src/backend/commands/repack_worker.c | 4 ++--
src/backend/commands/wait.c | 32 ++++++-------------------
src/include/access/xlogwait.h | 2 +-
src/test/recovery/t/049_wait_for_lsn.pl | 14 +++++++++++
6 files changed, 38 insertions(+), 34 deletions(-)
diff --git a/doc/src/sgml/ref/wait_for.sgml b/doc/src/sgml/ref/wait_for.sgml
index 04ca9400426..36b9ab61976 100644
--- a/doc/src/sgml/ref/wait_for.sgml
+++ b/doc/src/sgml/ref/wait_for.sgml
@@ -145,13 +145,21 @@ WAIT FOR LSN '<replaceable class="parameter">lsn</replaceable>'
<para>
When specified and <parameter>timeout</parameter> is greater than zero,
the command waits until <parameter>lsn</parameter> is reached or
- the specified <parameter>timeout</parameter> has elapsed.
+ the specified <parameter>timeout</parameter> has elapsed. A value
+ of zero (the default) means the command waits indefinitely.
</para>
<para>
- The <parameter>timeout</parameter> might be given as integer number of
- milliseconds. Also it might be given as string literal with
- integer number of milliseconds or a number with unit
- (see <xref linkend="config-setting-names-values"/>).
+ The <parameter>timeout</parameter> is an amount of time in
+ milliseconds. It may also be specified as a string containing the
+ numerical value followed by a time unit
+ (see <xref linkend="config-setting-names-values"/>). The maximum
+ value is <literal>2147483647 ms</literal>.
+ </para>
+ <para>
+ Fractional values are rounded to the nearest millisecond. Note
+ that a <parameter>timeout</parameter> of half a millisecond or
+ less therefore rounds down to zero, which means waiting
+ indefinitely.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/access/transam/xlogwait.c b/src/backend/access/transam/xlogwait.c
index eee90e7f626..2ea8c24a74f 100644
--- a/src/backend/access/transam/xlogwait.c
+++ b/src/backend/access/transam/xlogwait.c
@@ -437,7 +437,7 @@ WaitLSNTypeRequiresRecovery(WaitLSNType t)
* or replica got promoted before the target LSN reached.
*/
WaitLSNResult
-WaitForLSN(WaitLSNType lsnType, XLogRecPtr targetLSN, int64 timeout)
+WaitForLSN(WaitLSNType lsnType, XLogRecPtr targetLSN, int timeout)
{
XLogRecPtr currentLSN;
WaitLSNProcInfo *procInfo;
diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
index b4ba9cfc67b..bf2bc2dca13 100644
--- a/src/backend/commands/repack_worker.c
+++ b/src/backend/commands/repack_worker.c
@@ -453,7 +453,7 @@ decode_concurrent_changes(LogicalDecodingContext *ctx,
if (record == NULL)
{
- int64 timeout = 0;
+ int timeout = 0;
WaitLSNResult res;
/*
@@ -472,7 +472,7 @@ decode_concurrent_changes(LogicalDecodingContext *ctx,
* should already have been flushed to disk.
*/
if (!XLogRecPtrIsValid(lsn_upto))
- timeout = 100L;
+ timeout = 100;
res = WaitForLSN(WAIT_LSN_TYPE_PRIMARY_FLUSH,
ctx->reader->EndRecPtr + 1,
timeout);
diff --git a/src/backend/commands/wait.c b/src/backend/commands/wait.c
index 9ba4c75021e..bac38d17726 100644
--- a/src/backend/commands/wait.c
+++ b/src/backend/commands/wait.c
@@ -13,8 +13,6 @@
*/
#include "postgres.h"
-#include <math.h>
-
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogwait.h"
@@ -35,7 +33,7 @@ ExecWaitStmt(ParseState *pstate, WaitStmt *stmt, bool isTopLevel,
DestReceiver *dest)
{
XLogRecPtr lsn;
- int64 timeout = 0;
+ int timeout = 0;
WaitLSNResult waitLSNResult;
WaitLSNType lsnType = WAIT_LSN_TYPE_STANDBY_REPLAY; /* default */
bool throw = true;
@@ -92,7 +90,6 @@ ExecWaitStmt(ParseState *pstate, WaitStmt *stmt, bool isTopLevel,
{
char *timeout_str;
const char *hintmsg;
- double dval;
if (timeout_specified)
errorConflictingDefElem(defel, pstate);
@@ -100,33 +97,18 @@ ExecWaitStmt(ParseState *pstate, WaitStmt *stmt, bool isTopLevel,
timeout_str = defGetString(defel);
- if (!parse_real(timeout_str, &dval, GUC_UNIT_MS, &hintmsg))
- {
+ if (!parse_int(timeout_str, &timeout, GUC_UNIT_MS, &hintmsg))
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid timeout value: \"%s\"", timeout_str),
- hintmsg ? errhint("%s", _(hintmsg)) : 0);
- }
-
- /*
- * Get rid of any fractional part in the input. This is so we
- * don't fail on just-out-of-range values that would round into
- * range.
- */
- dval = rint(dval);
+ hintmsg ? errhint("%s", _(hintmsg)) : 0,
+ parser_errposition(pstate, defel->location));
- /* Range check */
- if (unlikely(isnan(dval) || !FLOAT8_FITS_IN_INT64(dval)))
- ereport(ERROR,
- errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
- errmsg("timeout value is out of range"));
-
- if (dval < 0)
+ if (timeout < 0)
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("timeout cannot be negative"));
-
- timeout = (int64) dval;
+ errmsg("timeout cannot be negative"),
+ parser_errposition(pstate, defel->location));
}
else if (strcmp(defel->defname, "no_throw") == 0)
{
diff --git a/src/include/access/xlogwait.h b/src/include/access/xlogwait.h
index 07157f220ea..2bf0263e9e2 100644
--- a/src/include/access/xlogwait.h
+++ b/src/include/access/xlogwait.h
@@ -104,6 +104,6 @@ extern XLogRecPtr GetCurrentLSNForWaitType(WaitLSNType lsnType);
extern void WaitLSNWakeup(WaitLSNType lsnType, XLogRecPtr currentLSN);
extern void WaitLSNCleanup(void);
extern WaitLSNResult WaitForLSN(WaitLSNType lsnType, XLogRecPtr targetLSN,
- int64 timeout);
+ int timeout);
#endif /* XLOG_WAIT_H */
diff --git a/src/test/recovery/t/049_wait_for_lsn.pl b/src/test/recovery/t/049_wait_for_lsn.pl
index cb7d4d461de..50c08dfac8f 100644
--- a/src/test/recovery/t/049_wait_for_lsn.pl
+++ b/src/test/recovery/t/049_wait_for_lsn.pl
@@ -343,6 +343,14 @@ $node_standby->psql(
stderr => \$stderr);
ok($stderr =~ /timeout cannot be negative/, "get error for negative timeout");
+# Test out of range timeout
+$node_standby->psql(
+ 'postgres',
+ "WAIT FOR LSN '${test_lsn}' WITH (timeout '2147483648ms');",
+ stderr => \$stderr);
+ok($stderr =~ /invalid timeout value: "2147483648ms"/,
+ "get error for out of range timeout");
+
# Test unknown parameter with WITH clause
$node_standby->psql(
'postgres',
@@ -407,6 +415,12 @@ $output = $node_standby->safe_psql(
ok($output eq "timeout",
"WAIT FOR WITH clause returns correct timeout status");
+# Test maximum timeout
+$output = $node_standby->safe_psql(
+ 'postgres', qq[
+ WAIT FOR LSN '${lsn2}' WITH (timeout '2147483647ms', no_throw);]);
+ok($output eq "success", "maximum timeout value is accepted");
+
# Test WITH clause error case - invalid option
$node_standby->psql(
'postgres',
--
2.55.0
