Re: timeout value overflow in wait for lsn

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

> Also, I think it's better to mention the maximum value accepted as a
> timeout value.

Fixed.

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