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

On Tue, May 18, 2021 at 7:46 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Tue, May 18, 2021 at 7:19 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)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.

I'm sorry about saying that the unquoted integers are accepted with
batch_size, fetch_size, but actually the parser is throwing the syntax
error.

So, we can safely use parse_int for batch_size and fetch_size,
parse_real for fdw_tuple_cost and fdw_startup_cost without changing
any behaviour.

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