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

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?

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

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