Re: postgres_fdw - should we tighten up batch_size, fetch_size options against non-numeric values?

On Wed, Jun 30, 2021 at 5:53 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> On 2021/05/20 1:01, Bharath Rupireddy wrote:
> > Thanks for the comments. I added separate messages, changed the error
> > code from ERRCODE_SYNTAX_ERROR to ERRCODE_INVALID_PARAMETER_VALUE and
> > also quoted the option name in the error message. PSA v3 patch.
>
> Thanks for updating the patch!
>
> +                                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +                                                errmsg("invalid numeric value for option \"%s\"",
> +                                                               def->defname)));
>
> In reloptions.c, when parse_real() fails to parse the input, the error message
> "invalid value for floating point option..." is output.
> For the sake of consistency, we should use the same error message here?

Actually, there's an existing error message errmsg("%s requires a
non-negative numeric value" that used "numeric value". If we were to
change errmsg("invalid numeric value for option \"%s\"", to
errmsg("invalid value for floating point option \"%s\"",, then we
might have to change the existing message. And also, the docs use
"numeric value" for fdw_startup_cost and fdw_tuple_cost. IMO, let's go
with errmsg("invalid value for numeric option \"%s\": %s",.

> -                                               (errcode(ERRCODE_SYNTAX_ERROR),
> -                                                errmsg("%s requires a non-negative integer value",
> +                                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +                                                errmsg("invalid integer value for option \"%s\"",
>
> IMO the error message should be "invalid value for integer option..." here
> because of the same reason I told above. Thought?

Changed.

PSA v4.

Regards,
Bharath Rupireddy.