On 2021/05/19 11:36, Kyotaro Horiguchi wrote:
At Tue, 18 May 2021 19:46:39 +0530, Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote in
On Tue, May 18, 2021 at 7:19 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
Fujii Masao <masao.fu...@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
