Re: timeout value overflow in wait for lsn

On Tue, Sep 1, 2026 at 6:40 PM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
>
> On Wed, Sep 2, 2026 at 5:09 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Mon, Aug 31, 2026 at 6:17 PM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
> > >
> > > On Mon, Aug 31, 2026 at 10:07 PM cca5507 <cca5507(at)qq(dot)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.

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