Re: timeout value overflow in wait for lsn

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.

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