Hi Changao, Sawada-san, On Sat, Aug 29, 2026 at 6:02 PM cca5507 <[email protected]> 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.
