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

On Wed, May 19, 2021 at 5:02 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> On 2021/05/19 14:34, Bharath Rupireddy wrote:
> > On Wed, May 19, 2021 at 8:28 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> >>>>> I agree with throwing an error for non-numeric junk though.
> >>>>> Allowing that on the grounds of backwards compatibility
> >>>>> seems like too much of a stretch.
> >>>>
> >>>> +1.
> >>>
> >>> +1.
> >>
> >> +1
> >
> > Thanks all for your inputs. PSA which uses parse_int for
> > batch_size/fech_size and parse_real for fdw_startup_cost and
> > fdw_tuple_cost.
>
> Thanks for updating the patch! It basically looks good to me.
>
> - val = strtod(defGetString(def), &endp);
> - if (*endp || val < 0)
> + is_parsed = parse_real(defGetString(def), &val, 0, NULL);
> + if (!is_parsed || val < 0)
> ereport(ERROR,
> (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("%s requires a non-negative numeric value",
>
> Isn't it better to check "!is_parsed" and "val < 0" separately like
> reloptions.c does? That is, we should throw different error messages
> for them?
>
> ERRCODE_SYNTAX_ERROR seems strange for this type of error?
> ERRCODE_INVALID_PARAMETER_VALUE is better and more proper?

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.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com