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

At Tue, 18 May 2021 19:46:39 +0530, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in
> 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.
>
> 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.

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

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center