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

On 2021/05/19 11:36, Kyotaro Horiguchi wrote:
> At Tue, 18 May 2021 19:46:39 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
>> On Tue, May 18, 2021 at 7:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>
>>> Fujii Masao <masao.fujii@oss.nttdata.com> writes:
>>>> On 2021/05/17 18:58, Bharath Rupireddy wrote:
>>>>> It looks like the values such as '123.456', '789.123' '100$%$#$#',
>>>>> '9,223,372,' are accepted and treated as valid integers for
>>>>> postgres_fdw options batch_size and fetch_size. Whereas this is not
>>>>> the case with fdw_startup_cost and fdw_tuple_cost options for which an
>>>>> error is thrown. Attaching a patch to fix that.
>>>
>>>> This looks an improvement. But one issue is that the restore of
>>>> dump file taken by pg_dump from v13 may fail for v14 with this patch
>>>> if it contains invalid setting of fetch_size, e.g., "fetch_size '123.456'".
>>>> OTOH, since batch_size was added in v14, it has no such issue.
>>>
>>> Maybe better to just silently round to integer?  I think that's
>>> what we generally do with integer GUCs these days, eg
>>>
>>> regression=# set work_mem = 102.9;
>>> SET
>>> regression=# show work_mem;
>>>   work_mem
>>> ----------
>>>   103kB
>>> (1 row)
>>
>> I think we can use parse_int to parse the fetch_size and batch_size as
>> integers, which also rounds off decimals to integers and returns false
>> for non-numeric junk. But, it accepts both quoted and unquoted
>> integers, something like batch_size 100 and batch_size '100', which I
>> feel is okay because the reloptions also accept both.
>>
>> While on this, we can also use parse_real for fdw_startup_cost and
>> fdw_tuple_cost options but with that they will accept both quoted and
>> unquoted real values.
>>
>> Thoughts?
> 
> They are more or less a kind of reloptions. So I think it is
> reasonable to treat the option same way with RELOPT_TYPE_INT.  That
> is, it would be better to use our standard functions rather than
> random codes using bare libc functions for input from users. The same
> can be said for parameters with real numbers, regardless of the
> "quoted" discussion.

Sounds reasonable. Since parse_int() is used to parse RELOPT_TYPE_INT value
in reloptions.c, your idea seems to be almost the same as Bharath's one.
That is, use parse_int() and parse_real() to parse integer and real options
values, respectively.

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

Regards,

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