timeout value overflow in wait for lsn

Hi ChangAo,

On Wed, Aug 19, 2026 at 9:41 PM cca5507 <cca5507@qq.com> wrote:

Hi,

The valid range of the timeout value in wait for lsn command is int64, which can overflow
later in WaitForLSN():

```
postgres=# wait for lsn '99/99999999' with (mode 'primary_flush', timeout '10000000000000000ms');
ERROR: timed out while waiting for target LSN 99/99999999 to be flushed; current primary_flush LSN 0/017CA348
Time: 0.584 ms
```

To fix it, change the valid range to int32, just like deadlock_timeout and many other GUCs.

Thoughts?

Good catch. I am not sure about the fix. The likely cause of overflow is:

#define TimestampTzPlusMilliseconds(tz, ms) \
((tz) + ((ms) * (int64) 1000))

values far greater than int32 could be a problem. But does this
warrant a truncation to int32? Yeah, from a pragmatic perspective,
these off-charts values are not expected in practice since the users
don't have and better not have this amount of patience for latency.
But truncating it alone like

+ if (unlikely(isnan(dval) || !FLOAT8_FITS_IN_INT32(dval)))

seems not adequate to me -- the interface supports int 64, it seems
not good to accept it first and then reject it loudly later. If this
change is desired, we might need to change the interface as well.
Another direction is to prevent the overflow while preserving the
current value by checking the timeout with

if (pg_mul_s64_overflow(timeout, USECS_PER_MSEC, &timeout_us) ||
pg_add_s64_overflow(now, timeout_us, &endtime) ||
!IS_VALID_TIMESTAMP(endtime))

but this seems unprecedented for a timeout value. It would be helpful
to hear Alexander's thoughts on this.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

On Wed, Aug 19, 2026 at 11:09 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:

Hi ChangAo,

On Wed, Aug 19, 2026 at 9:41 PM cca5507 <cca5507@qq.com> wrote:

Hi,

The valid range of the timeout value in wait for lsn command is int64, which can overflow
later in WaitForLSN():

```
postgres=# wait for lsn '99/99999999' with (mode 'primary_flush', timeout '10000000000000000ms');
ERROR: timed out while waiting for target LSN 99/99999999 to be flushed; current primary_flush LSN 0/017CA348
Time: 0.584 ms
```

To fix it, change the valid range to int32, just like deadlock_timeout and many other GUCs.

Thoughts?

Good catch. I am not sure about the fix. The likely cause of overflow is:

#define TimestampTzPlusMilliseconds(tz, ms) \
((tz) + ((ms) * (int64) 1000))

values far greater than int32 could be a problem. But does this
warrant a truncation to int32? Yeah, from a pragmatic perspective,
these off-charts values are not expected in practice since the users
don't have and better not have this amount of patience for latency.
But truncating it alone like

+ if (unlikely(isnan(dval) || !FLOAT8_FITS_IN_INT32(dval)))

seems not adequate to me -- the interface supports int 64, it seems
not good to accept it first and then reject it loudly later. If this
change is desired, we might need to change the interface as well.
Another direction is to prevent the overflow while preserving the
current value by checking the timeout with

if (pg_mul_s64_overflow(timeout, USECS_PER_MSEC, &timeout_us) ||
pg_add_s64_overflow(now, timeout_us, &endtime) ||
!IS_VALID_TIMESTAMP(endtime))

Sleeping with it for a night, the above writing seems to be somewhat
confusing. Here's a version improved by Sol:

Changing the check to FLOAT8_FITS_IN_INT32 would reject larger values.
If we choose that limit, the timeout variable and the WaitForLSN()
argument should also use int so that the interface matches the
accepted range.

Alternatively, we can preserve the int64 interface and check the
deadline calculation in WaitForLSN():

if (pg_mul_s64_overflow(timeout, USECS_PER_MSEC, &timeout_us) ||
pg_add_s64_overflow(now, timeout_us, &endtime) ||
!IS_VALID_TIMESTAMP(endtime))

-----------------

Another option is to reject values greater than or equal with
INT64_MAX/1000, which seems a bit hacky to me.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

Changing the check to FLOAT8_FITS_IN_INT32 would reject larger values.
If we choose that limit, the timeout variable and the WaitForLSN()
argument should also use int so that the interface matches the
accepted range.

Alternatively, we can preserve the int64 interface and check the
deadline calculation in WaitForLSN():

    if (pg_mul_s64_overflow(timeout, USECS_PER_MSEC, &timeout_us) ||
        pg_add_s64_overflow(now, timeout_us, &endtime) ||
        !IS_VALID_TIMESTAMP(endtime))

-----------------

Another option is to reject values greater than or equal with
INT64_MAX/1000, which seems a bit hacky to me.

The max timeout value supported by WaitLatch() is INT_MAX, so I think
it's reasonable to limit the range to int32. And I think it's ok to use int64
as the argument in WaitForLSN() because convert int32 to int64 is always
safe.

--
Regards,
ChangAo Chen

On Thu, Aug 20, 2026 at 11:12 AM cca5507 <cca5507@qq.com> wrote:

Changing the check to FLOAT8_FITS_IN_INT32 would reject larger values.
If we choose that limit, the timeout variable and the WaitForLSN()
argument should also use int so that the interface matches the
accepted range.

Alternatively, we can preserve the int64 interface and check the
deadline calculation in WaitForLSN():

if (pg_mul_s64_overflow(timeout, USECS_PER_MSEC, &timeout_us) ||
pg_add_s64_overflow(now, timeout_us, &endtime) ||
!IS_VALID_TIMESTAMP(endtime))

-----------------

Another option is to reject values greater than or equal with
INT64_MAX/1000, which seems a bit hacky to me.

The max timeout value supported by WaitLatch() is INT_MAX, so I think
it's reasonable to limit the range to int32.

We have a loop in the wait infra, which means that the waiter could
fall asleep several times. Each time takes a INT_MAX maximumly, added
up toward a value larger than INT_MAX. That is why I was wondering
whether the bug itself warrants a truncation from 64 to 32. If there
are user needs like absurdly long timeouts, then we better satisfy
them and there're ways to do so. But in my poor imagination, waiting
greater than 25 days seems unlikely in reality. So I voted for the
limitation of the range.

And I think it's ok to use int64
as the argument in WaitForLSN() because convert int32 to int64 is always
safe.

Yeah, it is safe only if we handle the checking/rejection properly.
The wait for infra is also used by repack, which uses timeout as zero
for an indefinite wait and 100 milliseconds for a finite wait. So it
is not vulnerable to edge cases like this. But the infra could have
more potential callers in the future, we better not let them bother
the extra safety if we can deal with it easily. Sorry if this seems
nitpicking to you.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

We have a loop in the wait infra, which means that the waiter could
fall asleep several times. Each time takes a INT_MAX maximumly, added
up toward a value larger than INT_MAX. That is why I was wondering
whether the bug itself warrants a truncation from 64 to 32. If there
are user needs like absurdly long timeouts, then we better satisfy
them and there're ways to do so. But in my poor imagination, waiting
greater than 25 days seems unlikely in reality. So I voted for the
limitation of the range.

 And I think it's ok to use int64
 as the argument in WaitForLSN() because convert int32 to int64 is always
 safe.

Yeah, it is safe only if we handle the checking/rejection properly.
The wait for infra is also used by repack, which uses timeout as zero
for an indefinite wait and 100 milliseconds for a finite wait. So it
is not vulnerable to edge cases like this. But the infra could have
more potential callers in the future, we better not let them bother
the extra safety if we can deal with it easily. Sorry if this seems
nitpicking to you.

Make sense to me. The v2 patch forgets to update the type of the timeout
variable in repack worker, fixed in v3.

--
Regards,
ChangAo Chen

On Thu, Aug 20, 2026 at 2:17 PM cca5507 <cca5507@qq.com> wrote:

We have a loop in the wait infra, which means that the waiter could
fall asleep several times. Each time takes a INT_MAX maximumly, added
up toward a value larger than INT_MAX. That is why I was wondering
whether the bug itself warrants a truncation from 64 to 32. If there
are user needs like absurdly long timeouts, then we better satisfy
them and there're ways to do so. But in my poor imagination, waiting
greater than 25 days seems unlikely in reality. So I voted for the
limitation of the range.

And I think it's ok to use int64
as the argument in WaitForLSN() because convert int32 to int64 is always
safe.

Yeah, it is safe only if we handle the checking/rejection properly.
The wait for infra is also used by repack, which uses timeout as zero
for an indefinite wait and 100 milliseconds for a finite wait. So it
is not vulnerable to edge cases like this. But the infra could have
more potential callers in the future, we better not let them bother
the extra safety if we can deal with it easily. Sorry if this seems
nitpicking to you.

Make sense to me. The v2 patch forgets to update the type of the timeout
variable in repack worker, fixed in v3.

Thanks. WFM.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

Does this one deserve a mention on the open items wiki [0]https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items?

[0]: https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items

--
nathan

Hi Nathan,

On Wed, Aug 26, 2026 at 3:51 AM Nathan Bossart <nathandbossart@gmail.com> wrote:

Does this one deserve a mention on the open items wiki [0]?

[0] https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items

This seems to be a simple issue for me. I am not sure what qualifies
one to be listed in that wiki. Apart from this, I don't have access to
edit that page. I sent an email to the RMT team but did not get a
reply yet.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

On Wed, Aug 19, 2026 at 11:18 PM cca5507 <cca5507@qq.com> wrote:

We have a loop in the wait infra, which means that the waiter could
fall asleep several times. Each time takes a INT_MAX maximumly, added
up toward a value larger than INT_MAX. That is why I was wondering
whether the bug itself warrants a truncation from 64 to 32. If there
are user needs like absurdly long timeouts, then we better satisfy
them and there're ways to do so. But in my poor imagination, waiting
greater than 25 days seems unlikely in reality. So I voted for the
limitation of the range.

And I think it's ok to use int64
as the argument in WaitForLSN() because convert int32 to int64 is always
safe.

Yeah, it is safe only if we handle the checking/rejection properly.
The wait for infra is also used by repack, which uses timeout as zero
for an indefinite wait and 100 milliseconds for a finite wait. So it
is not vulnerable to edge cases like this. But the infra could have
more potential callers in the future, we better not let them bother
the extra safety if we can deal with it easily. Sorry if this seems
nitpicking to you.

Make sense to me. The v2 patch forgets to update the type of the timeout
variable in repack worker, fixed in v3.

I found another issue around timeout value handling: if we specify a
timeout in [-0.5, 0.5], the WAIT FOR command waits forever. A negative
timeout in [-0.5, 0) should be rejected. ISTM a timeout in (0, 0.5] is
rounded down to 0, disabling the timeout essentially, which would
surprise users. I think we can either round up timeout in (0, 1] to 1,
or reject sub-millisecond values. I think we can fix both in the same
patch that fixes the overflow issue.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

I found another issue around timeout value handling: if we specify a
timeout in [-0.5, 0.5], the WAIT FOR command waits forever. A negative
timeout in [-0.5, 0) should be rejected. ISTM a timeout in (0, 0.5] is
rounded down to 0, disabling the timeout essentially, which would
surprise users. I think we can either round up timeout in (0, 1] to 1,
or reject sub-millisecond values. I think we can fix both in the same
patch that fixes the overflow issue.

Good catch! Fixed by moving the negative check before rint() and rounding
timeout in (0, 1) to 1.

Please see the v4 patch.

--
Regards,
ChangAo Chen

On Fri, Aug 28, 2026 at 2:11 AM cca5507 <cca5507@qq.com> wrote:

I found another issue around timeout value handling: if we specify a
timeout in [-0.5, 0.5], the WAIT FOR command waits forever. A negative
timeout in [-0.5, 0) should be rejected. ISTM a timeout in (0, 0.5] is
rounded down to 0, disabling the timeout essentially, which would
surprise users. I think we can either round up timeout in (0, 1] to 1,
or reject sub-millisecond values. I think we can fix both in the same
patch that fixes the overflow issue.

Good catch! Fixed by moving the negative check before rint() and rounding
timeout in (0, 1) to 1.

Please see the v4 patch.

Thank you for updating the patch! Here are review comments:

+           if (dval < 0.0)
+               ereport(ERROR,
+                       errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                       errmsg("timeout cannot be negative"));

Let's add parser_errposition() here.

Probably we can add the same to other ereport(ERROR) handling a
timeout option value.

---
             /*
              * 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.
 +            * range.  Round values in (0, 1) up to 1 to avoid treating them as
 +            * zero, which means waiting indefinitely.
              */
 -           dval = rint(dval);
 +           if (dval > 0.0 && dval < 1.0)
 +               dval = 1.0;
 +           else
 +               dval = rint(dval);

The first paragraph is for the else branch whereas the second
paragraph is for the if branch. I think we can write these comments
separately in each branch instead.

---
The documentation says "The timeout 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 Section 19.1.1).",
which seems incorrect to me as we parse the timeout value using
parse_real(), clearly accepting real values. Or should we have used
parse_int() in the first place?

Also, I think it's better to mention the maximum value accepted as a
timeout value.

---
I think it's better to add regression tests for the timeout option.
049_wait_for_lsn.pl would be a good place to have them.

---
The patch needs to run pgindent.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Thank you for updating the patch! Here are review comments:

+           if (dval < 0.0)
+               ereport(ERROR,
+                       errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                       errmsg("timeout cannot be negative"));

Let's add parser_errposition() here.

Probably we can add the same to other ereport(ERROR) handling a
timeout option value.

Fixed.

---
             /*
              * 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.
 +            * range.  Round values in (0, 1) up to 1 to avoid treating them as
 +            * zero, which means waiting indefinitely.
              */
 -           dval = rint(dval);
 +           if (dval > 0.0 && dval < 1.0)
 +               dval = 1.0;
 +           else
 +               dval = rint(dval);

The first paragraph is for the else branch whereas the second
paragraph is for the if branch. I think we can write these comments
separately in each branch instead.

Fixed.

---
The documentation says "The timeout 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 Section 19.1.1).",
which seems incorrect to me as we parse the timeout value using
parse_real(), clearly accepting real values. Or should we have used
parse_int() in the first place?

I think it's ok to use parse_real() here because parse_int() also accepts
real values.

Also, I think it's better to mention the maximum value accepted as a
timeout value.

Fixed.

---
I think it's better to add regression tests for the timeout option.
049_wait_for_lsn.pl would be a good place to have them.

Fixed.

---
The patch needs to run pgindent.

Fixed.

Please see the v5 patch.

--
Regards,
ChangAo Chen

Hi Changao, Sawada-san,

On Sat, Aug 29, 2026 at 6:02 PM cca5507 <cca5507@qq.com> wrote:

Thank you for updating the patch! Here are review comments:

+           if (dval < 0.0)
+               ereport(ERROR,
+                       errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                       errmsg("timeout cannot be negative"));

Let's add parser_errposition() here.

Probably we can add the same to other ereport(ERROR) handling a
timeout option value.

Fixed.

---
/*
* 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.
+            * range.  Round values in (0, 1) up to 1 to avoid treating them as
+            * zero, which means waiting indefinitely.
*/
-           dval = rint(dval);
+           if (dval > 0.0 && dval < 1.0)
+               dval = 1.0;
+           else
+               dval = rint(dval);

The first paragraph is for the else branch whereas the second
paragraph is for the if branch. I think we can write these comments
separately in each branch instead.

Fixed.

---
The documentation says "The timeout 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 Section 19.1.1).",
which seems incorrect to me as we parse the timeout value using
parse_real(), clearly accepting real values. Or should we have used
parse_int() in the first place?

I think it's ok to use parse_real() here because parse_int() also accepts
real values.

I think there's still subtlety regarding the use of parse_real() or
parse_int() and how to handle fractional values. I'll reply later for
this.

Also, I think it's better to mention the maximum value accepted as a
timeout value.

Fixed.

Mentioning the max value in doc seems useful to me, despite no
precedents of timeout have done so even if they share the same
capping.

+          The valid range is from 0 to 2,147,483,647 milliseconds, inclusive.
+          A value of zero means waiting indefinitely.

2,147,483,647 milliseconds seems ok for agents to read but not very
interpretable to humans. I doubt that few people would actually type
it manually. The main use here seems to let users have a vague concept
of the max value, so it might be helpful to convert that value to
something that humans can read like xx days.

---
I think it's better to add regression tests for the timeout option.
049_wait_for_lsn.pl would be a good place to have them.

Fixed.

---
The patch needs to run pgindent.

Fixed.

Please see the v5 patch.

--
Regards,
ChangAo Chen

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

On Sun, Aug 30, 2026 at 11:04 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:

Hi Changao, Sawada-san,

On Sat, Aug 29, 2026 at 6:02 PM cca5507 <cca5507@qq.com> wrote:

Thank you for updating the patch! Here are review comments:

+           if (dval < 0.0)
+               ereport(ERROR,
+                       errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                       errmsg("timeout cannot be negative"));

Let's add parser_errposition() here.

Probably we can add the same to other ereport(ERROR) handling a
timeout option value.

Fixed.

---
/*
* 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.
+            * range.  Round values in (0, 1) up to 1 to avoid treating them as
+            * zero, which means waiting indefinitely.
*/
-           dval = rint(dval);
+           if (dval > 0.0 && dval < 1.0)
+               dval = 1.0;
+           else
+               dval = rint(dval);

The first paragraph is for the else branch whereas the second
paragraph is for the if branch. I think we can write these comments
separately in each branch instead.

Fixed.

---
The documentation says "The timeout 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 Section 19.1.1).",
which seems incorrect to me as we parse the timeout value using
parse_real(), clearly accepting real values. Or should we have used
parse_int() in the first place?

I think it's ok to use parse_real() here because parse_int() also accepts
real values.

I think there's still subtlety regarding the use of parse_real() or
parse_int() and how to handle fractional values. I'll reply later for
this.

--- The issue
Patch v5 does not fully prevent nonzero timeouts from becoming
indefinite waits. For example:
timeout '0.0001s'  → 0.0ms
timeout '-0.0001s' → -0.0ms

Both bypass the new positive/negative checks and become int zero. The
added 0.1ms tests miss this because that spelling retains microsecond
precision during conversion.

postgres=# \timing on
Timing is on.
postgres=# SET statement_timeout = '1s';
SET
Time: 4.071 ms
postgres=#
postgres=# WAIT FOR LSN 'FFFFFFFF/FFFFFFFE'
postgres-# WITH (timeout '0.0001s', no_throw);
ERROR: canceling statement due to statement timeout
Time: 1002.685 ms (00:01.003)
postgres=# WAIT FOR LSN 'FFFFFFFF/FFFFFFFE'
postgres-# WITH (timeout '-0.0001s', no_throw);
ERROR: canceling statement due to statement timeout
Time: 1001.470 ms (00:01.001)

The commands above are canceled for statement timeout rather than the
timeout of WAIT FOR.

--- The likely cause
The main cause for this seems to be parse_real() calls
convert_to_base_unit(), which follows GUC semantics and rounds
fractional values to the next smaller unit. For a value expressed in
seconds, conversion rounds to whole milliseconds before returning:

0.0001s → 0.1ms → 0.0ms

convert_to_base_unit:

for (int i = 0; *table[i].unit; i++)
{
if (base_unit == table[i].base_unit &&
strcmp(unitstr, table[i].unit) == 0)
{
double cvalue = value * table[i].multiplier;

/*
* If the user gave a fractional value such as "30.1GB", round it
* off to the nearest multiple of the next smaller unit, if there
* is one.
*/
if (*table[i + 1].unit &&
base_unit == table[i + 1].base_unit)
cvalue = rint(cvalue / table[i + 1].multiplier) *
table[i + 1].multiplier;

*base_value = cvalue;
return true;
}
}

For 0.0001s, substitute the table values:
value = 0.0001;
table[i].multiplier = 1000; /* seconds → milliseconds */
table[i + 1].multiplier = 1; /* next smaller unit is ms */

cvalue = value * 1000;
= 0.1;

cvalue = rint(cvalue / 1) * 1;
= rint(0.1);
= 0.0;

Thus information is already lost before v5 checks dval < 0.0 or dval >
0.0. Negative zero compares neither less nor greater than zero, so
additional conditions on the returned dval cannot recover the original
value.

This issue seems also to be true for parse_int since it also makes use
of convert_to_base_unit.

Moreover, even 0.1ms, which parse_real() returns as 0.1, is rounded by
parse_int() before the caller sees it:
parse_int("0.1ms", GUC_UNIT_MS)
0.1ms → 0.1ms # unit conversion preserves it
→ rint(0.1)
→ 0
The relevant code is:
if (!convert_to_base_unit(val,
endptr, (flags & GUC_UNIT),
&val))
...

/* Round to int, then check for overflow */
val = rint(val);

if (result)
*result = (int) val;

--- 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.

Also, I think it's better to mention the maximum value accepted as a
timeout value.

Fixed.

Mentioning the max value in doc seems useful to me, despite no
precedents of timeout have done so even if they share the same
capping.

+          The valid range is from 0 to 2,147,483,647 milliseconds, inclusive.
+          A value of zero means waiting indefinitely.

2,147,483,647 milliseconds seems ok for agents to read but not very
interpretable to humans. I doubt that few people would actually type
it manually. The main use here seems to let users have a vague concept
of the max value, so it might be helpful to convert that value to
something that humans can read like xx days.

---
I think it's better to add regression tests for the timeout option.
049_wait_for_lsn.pl would be a good place to have them.

Fixed.

---
The patch needs to run pgindent.

Fixed.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

On Mon, Aug 31, 2026 at 9:47 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:

On Sun, Aug 30, 2026 at 11:04 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:

Hi Changao, Sawada-san,

On Sat, Aug 29, 2026 at 6:02 PM cca5507 <cca5507@qq.com> wrote:

Thank you for updating the patch! Here are review comments:

+           if (dval < 0.0)
+               ereport(ERROR,
+                       errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                       errmsg("timeout cannot be negative"));

Let's add parser_errposition() here.

Probably we can add the same to other ereport(ERROR) handling a
timeout option value.

Fixed.

---
/*
* 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.
+            * range.  Round values in (0, 1) up to 1 to avoid treating them as
+            * zero, which means waiting indefinitely.
*/
-           dval = rint(dval);
+           if (dval > 0.0 && dval < 1.0)
+               dval = 1.0;
+           else
+               dval = rint(dval);

The first paragraph is for the else branch whereas the second
paragraph is for the if branch. I think we can write these comments
separately in each branch instead.

Fixed.

---
The documentation says "The timeout 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 Section 19.1.1).",
which seems incorrect to me as we parse the timeout value using
parse_real(), clearly accepting real values. Or should we have used
parse_int() in the first place?

I think it's ok to use parse_real() here because parse_int() also accepts
real values.

I think there's still subtlety regarding the use of parse_real() or
parse_int() and how to handle fractional values. I'll reply later for
this.

--- The issue
Patch v5 does not fully prevent nonzero timeouts from becoming
indefinite waits. For example:
timeout '0.0001s'  → 0.0ms
timeout '-0.0001s' → -0.0ms

Both bypass the new positive/negative checks and become int zero. The
added 0.1ms tests miss this because that spelling retains microsecond
precision during conversion.

postgres=# \timing on
Timing is on.
postgres=# SET statement_timeout = '1s';
SET
Time: 4.071 ms
postgres=#
postgres=# WAIT FOR LSN 'FFFFFFFF/FFFFFFFE'
postgres-# WITH (timeout '0.0001s', no_throw);
ERROR: canceling statement due to statement timeout
Time: 1002.685 ms (00:01.003)
postgres=# WAIT FOR LSN 'FFFFFFFF/FFFFFFFE'
postgres-# WITH (timeout '-0.0001s', no_throw);
ERROR: canceling statement due to statement timeout
Time: 1001.470 ms (00:01.001)

The commands above are canceled for statement timeout rather than the
timeout of WAIT FOR.

--- The likely cause
The main cause for this seems to be parse_real() calls
convert_to_base_unit(), which follows GUC semantics and rounds
fractional values to the next smaller unit. For a value expressed in
seconds, conversion rounds to whole milliseconds before returning:

0.0001s → 0.1ms → 0.0ms

convert_to_base_unit:

for (int i = 0; *table[i].unit; i++)
{
if (base_unit == table[i].base_unit &&
strcmp(unitstr, table[i].unit) == 0)
{
double cvalue = value * table[i].multiplier;

/*
* If the user gave a fractional value such as "30.1GB", round it
* off to the nearest multiple of the next smaller unit, if there
* is one.
*/
if (*table[i + 1].unit &&
base_unit == table[i + 1].base_unit)
cvalue = rint(cvalue / table[i + 1].multiplier) *
table[i + 1].multiplier;

*base_value = cvalue;
return true;
}
}

For 0.0001s, substitute the table values:
value = 0.0001;
table[i].multiplier = 1000; /* seconds → milliseconds */
table[i + 1].multiplier = 1; /* next smaller unit is ms */

cvalue = value * 1000;
= 0.1;

cvalue = rint(cvalue / 1) * 1;
= rint(0.1);
= 0.0;

Thus information is already lost before v5 checks dval < 0.0 or dval >
0.0. Negative zero compares neither less nor greater than zero, so
additional conditions on the returned dval cannot recover the original
value.

This issue seems also to be true for parse_int since it also makes use
of convert_to_base_unit.

Moreover, even 0.1ms, which parse_real() returns as 0.1, is rounded by
parse_int() before the caller sees it:
parse_int("0.1ms", GUC_UNIT_MS)
0.1ms → 0.1ms # unit conversion preserves it
→ rint(0.1)
→ 0
The relevant code is:
if (!convert_to_base_unit(val,
endptr, (flags & GUC_UNIT),
&val))
...

/* Round to int, then check for overflow */
val = rint(val);

if (result)
*result = (int) val;

--- 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.

Here's a further investigation report from Sol with instructions but
*without* much audit from me -- it may hallucinate in some places. It
seems that materials gathered so far deserve a separate treatment.

Also, I think it's better to mention the maximum value accepted as a
timeout value.

Fixed.

Mentioning the max value in doc seems useful to me, despite no
precedents of timeout have done so even if they share the same
capping.

+          The valid range is from 0 to 2,147,483,647 milliseconds, inclusive.
+          A value of zero means waiting indefinitely.

2,147,483,647 milliseconds seems ok for agents to read but not very
interpretable to humans. I doubt that few people would actually type
it manually. The main use here seems to let users have a vague concept
of the max value, so it might be helpful to convert that value to
something that humans can read like xx days.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

--- 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.

--
Regards,
ChangAo Chen

On Mon, Aug 31, 2026 at 10:07 PM cca5507 <cca5507@qq.com> 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. Attached file is a response from Sol
regarding the anomaly which somewhat makes sense to me. It would be
helpful to hear Sawada-san and Alexander's thoughts on it. Also CCed
the members of RMT.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

On Mon, Aug 31, 2026 at 6:17 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:

On Mon, Aug 31, 2026 at 10:07 PM cca5507 <cca5507@qq.com> 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.

Attached file is a response from Sol
regarding the anomaly which somewhat makes sense to me. It would be
helpful to hear Sawada-san and Alexander's thoughts on it. Also CCed
the members of RMT.

FYI I don't think attaching an LLM response as a PDF is a good way to
bring material to this list. A most part of it is not about the unit
conversion at all, but advice to you about how to revise your report
and how to frame the argument here. For instance one section is headed
"This changes how I would revise your report", and another closes by
noting "makes the proposal much harder to object to". That is odd
material to send to the people on this thread who are working on this
problem, and it leaves the readers to figure out which parts are
claims about PostgreSQL and which are not, and to verify the contents.
I'd rather see the parts you have verified yourself, stated in your
own words and in plain text.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Wed, Sep 2, 2026 at 5:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Aug 31, 2026 at 6:17 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:

On Mon, Aug 31, 2026 at 10:07 PM cca5507 <cca5507@qq.com> 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. I still think
that it needs to be fixed in HEAD. The current generation of LLM keeps
finding embarrassing bugs as we can see in the pre-release season.
This is one example of that. At first, it seems trivial to me.
However, it turns out to not be the case as I looked into it. Fixing
bugs is not fun and I don't like to do it all the time. But some bugs
left unreported and unfixed would certainly surprise and disappoint
users one day. I guess the number and the frequency of bugs reporting
to the mailing list could only be used as one aspect of criteria for
judging whether the bug needs to be fixed or not, given lots of users
are not accustomed to the working style of mailing lists and are not
able to write decent reports in English, which demotivate them to
report, or simply don't know the existence of the channel.

Attached file is a response from Sol
regarding the anomaly which somewhat makes sense to me. It would be
helpful to hear Sawada-san and Alexander's thoughts on it. Also CCed
the members of RMT.

FYI I don't think attaching an LLM response as a PDF is a good way to
bring material to this list. A most part of it is not about the unit
conversion at all, but advice to you about how to revise your report
and how to frame the argument here. For instance one section is headed
"This changes how I would revise your report", and another closes by
noting "makes the proposal much harder to object to". That is odd
material to send to the people on this thread who are working on this
problem, and it leaves the readers to figure out which parts are
claims about PostgreSQL and which are not, and to verify the contents.
I'd rather see the parts you have verified yourself, stated in your
own words and in plain text.

Also agree. I didn't have a laptop at hand and enough concentrated
time at that time. I could've done better on myself later but thought
I needed to report it early at a faster pace. The conversation
happened on my mobile phone. I don't like the tone and expression it
used, which I ask explicitly not to do so in Codex on the computer.
Sorry if this seems a disrespect of your time.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.