Re: timeout value overflow in wait for lsn

On Mon, Aug 31, 2026 at 9:47 AM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
>
> 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.

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.