Re: timeout value overflow in wait for lsn

On Sun, Aug 30, 2026 at 11:04 AM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
>
> Hi Changao, Sawada-san,
>
> On Sat, Aug 29, 2026 at 6:02 PM cca5507 <cca5507(at)qq(dot)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.