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

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?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION