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.