On Fri, Aug 28, 2026 at 2:11 AM cca5507 <[email protected]> wrote:
>
> > I found another issue around timeout value handling: if we specify a
> > timeout in [-0.5, 0.5], the WAIT FOR command waits forever. A negative
> > timeout in [-0.5, 0) should be rejected. ISTM a timeout in (0, 0.5] is
> > rounded down to 0, disabling the timeout essentially, which would
> > surprise users. I think we can either round up timeout in (0, 1] to 1,
> > or reject sub-millisecond values. I think we can fix both in the same
> > patch that fixes the overflow issue.
>
> Good catch! Fixed by moving the negative check before rint() and rounding
> timeout in (0, 1) to 1.
>
> Please see the v4 patch.
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.
---
/*
* 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.
---
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?
Also, I think it's better to mention the maximum value accepted as a
timeout value.
---
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.
---
The patch needs to run pgindent.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
