postgres_fdw - should we tighten up batch_size, fetch_size options against non-numeric values?
Hi,
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.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v1-0001-Tighten-up-batch_size-fetch_size-options-against-.patchapplication/octet-stream; name=v1-0001-Tighten-up-batch_size-fetch_size-options-against-.patchDownload
From fec5a1577afe3a138ef54ad18920aa29386de0cb Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Mon, 17 May 2021 15:26:40 +0530
Subject: [PATCH v1] Tighten up batch_size, fetch_size options against
non-numeric values
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. This patch fixes this with passing endptr to
strtol.
---
contrib/postgres_fdw/option.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 672b55a808..d4044406a7 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -137,9 +137,10 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
else if (strcmp(def->defname, "fetch_size") == 0)
{
int fetch_size;
+ char *endp;
- fetch_size = strtol(defGetString(def), NULL, 10);
- if (fetch_size <= 0)
+ fetch_size = strtol(defGetString(def), &endp, 10);
+ if (*endp || fetch_size <= 0)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s requires a non-negative integer value",
@@ -148,9 +149,10 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
else if (strcmp(def->defname, "batch_size") == 0)
{
int batch_size;
+ char *endp;
- batch_size = strtol(defGetString(def), NULL, 10);
- if (batch_size <= 0)
+ batch_size = strtol(defGetString(def), &endp, 10);
+ if (*endp || batch_size <= 0)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s requires a non-negative integer value",
--
2.25.1
On Mon, May 17, 2021 at 3:29 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
Hi,
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 like a definite improvement. I wonder if we should modify
defGetInt variants to convert strings into integers, so that there's
consistent error message for such errors. We could define defGetUInt
so that we could mention non-negative in the error message. Whether or
not we do that, this looks good.
--
Best Wishes,
Ashutosh Bapat
On Mon, May 17, 2021 at 6:17 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
On Mon, May 17, 2021 at 3:29 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:Hi,
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 like a definite improvement. I wonder if we should modify
defGetInt variants to convert strings into integers, so that there's
consistent error message for such errors. We could define defGetUInt
so that we could mention non-negative in the error message.
If we do that, then the options that are only accepting unquoted
integers (i.e. 123, 456 etc.) and throwing errors for the quoted
integers ('123', '456' etc.) will then start to accept the quoted
integers. Other hackers might not agree to this change.
Another way is to have new API like
defGetInt32_2/defGetInt64_2/defGetNumeric_2 (or some other better
names) which basically accept both quoted and unquoted strings, see
[1]: int32 defGetInt32_2(DefElem *def) { if (def->arg == NULL) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("%s requires an integer value", def->defname)));
option needs to be parsed in both quoted and unquoted form. Or we can
also have these functions as [2]int32 defGetInt32_2(DefElem *def) { char *sval; int32 val; for only parsing quoted options. I
prefer [2]int32 defGetInt32_2(DefElem *def) { char *sval; int32 val; so that these API can be used without any code duplication.
Thoughts?
Whether or not we do that, this looks good.
I'm also okay if we can just fix the fetch_size and back_size options
for now as it's done in the patch attached with the first mail. Note
that I have not added any test case as this change is a trivial thing.
If required, I can add one.
[1]: int32 defGetInt32_2(DefElem *def) { if (def->arg == NULL) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("%s requires an integer value", def->defname)));
int32
defGetInt32_2(DefElem *def)
{
if (def->arg == NULL)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s requires an integer value",
def->defname)));
switch (nodeTag(def->arg))
{
case T_Integer:
return (int32) intVal(def->arg);
default:
{
char *sval;
int32 val;
sval = defGetString(def);
val = strtol(sval, &endp, 10);
if (*endp == '\0')
return val;
}
}
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s requires an integer value",
def->defname)));
return 0;
}
[2]: int32 defGetInt32_2(DefElem *def) { char *sval; int32 val;
int32
defGetInt32_2(DefElem *def)
{
char *sval;
int32 val;
sval = defGetString(def);
val = strtol(sval, &endp, 10);
if (*endp)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s requires an integer value",
def->defname)));
return val;
}
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On Mon, May 17, 2021 at 7:50 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
If we do that, then the options that are only accepting unquoted
integers (i.e. 123, 456 etc.) and throwing errors for the quoted
integers ('123', '456' etc.) will then start to accept the quoted
integers. Other hackers might not agree to this change.
I guess the options which weren't accepting quoted strings, will catch
these errors at the time of parsing itself. Even if that's not true, I
would see that as an improvement. Anyway, I won't stretch this
further.
Another way is to have new API like
defGetInt32_2/defGetInt64_2/defGetNumeric_2 (or some other better
names) which basically accept both quoted and unquoted strings, see
[1] for a rough sketch of the function. These API can be useful if an
option needs to be parsed in both quoted and unquoted form. Or we can
also have these functions as [2] for only parsing quoted options. I
prefer [2] so that these API can be used without any code duplication.
Thoughts?
I am not sure whether we want to maintain two copies. In that case
your first patch is fine.
Note
that I have not added any test case as this change is a trivial thing.
If required, I can add one.
That will help to make sure that we preserve the behaviour even
through code changes.
--
Best Wishes,
Ashutosh Bapat
On 2021/05/17 18:58, Bharath Rupireddy wrote:
Hi,
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.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
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 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.
regards, tom lane
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, egregression=# 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
On Tue, May 18, 2021 at 7:46 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
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, egregression=# 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
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, egregression=# 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
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, egregression=# 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
On Wed, May 19, 2021 at 8:28 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
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
Thanks all for your inputs. PSA which uses parse_int for
batch_size/fech_size and parse_real for fdw_startup_cost and
fdw_tuple_cost.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v2-0001-Tighten-up-batch_size-fetch_size-options-against-.patchapplication/octet-stream; name=v2-0001-Tighten-up-batch_size-fetch_size-options-against-.patchDownload
From 5ec9e489e1068a67256ca777a1e49d146c2e2c6c Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Wed, 19 May 2021 10:49:24 +0530
Subject: [PATCH v2] Tighten up batch_size, fetch_size options against
non-numeric values
It looks like the values such as '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. This is because endptr is not used while converting strings
to integers using strtol.
Use parse_int function instead of strtol as it serves the purpose by
returning false in case if it is unable to convert the string to
integer. Note that this function also rounds off the values such as
'100.456' to 100 and '100.567' or '100.678' to 101.
While on this, use parse_real for fdw_startup_cost and fdw_tuple_cost
options.
Since parse_int and parse_real are being used for reloptions and GUCs,
it is more appropriate to use in postgres_fdw rather than using
strtol and strtod directly.
---
.../postgres_fdw/expected/postgres_fdw.out | 19 +++++++++++++
contrib/postgres_fdw/option.c | 28 +++++++------------
contrib/postgres_fdw/postgres_fdw.c | 16 ++++++-----
contrib/postgres_fdw/sql/postgres_fdw.sql | 16 +++++++++++
4 files changed, 54 insertions(+), 25 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 7df30010f2..df4fdb5bb5 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10231,3 +10231,22 @@ DROP TABLE result_tbl;
DROP TABLE join_tbl;
ALTER SERVER loopback OPTIONS (DROP async_capable);
ALTER SERVER loopback2 OPTIONS (DROP async_capable);
+-- ===================================================================
+-- test invalid server and foreign table options
+-- ===================================================================
+-- Invalid fdw_startup_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_startup_cost '100$%$#$#');
+ERROR: fdw_startup_cost requires a non-negative numeric value
+-- Invalid fdw_tuple_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_tuple_cost '100$%$#$#');
+ERROR: fdw_tuple_cost requires a non-negative numeric value
+-- Invalid fetch_size option
+CREATE FOREIGN TABLE inv_fsz (c1 int )
+ SERVER loopback OPTIONS (fetch_size '100$%$#$#');
+ERROR: fetch_size requires a non-negative integer value
+-- Invalid batch_size option
+CREATE FOREIGN TABLE inv_bsz (c1 int )
+ SERVER loopback OPTIONS (batch_size '100$%$#$#');
+ERROR: batch_size requires a non-negative integer value
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 672b55a808..3dfcf2fb78 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -20,6 +20,7 @@
#include "commands/extension.h"
#include "postgres_fdw.h"
#include "utils/builtins.h"
+#include "utils/guc.h"
#include "utils/varlena.h"
/*
@@ -120,10 +121,10 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
{
/* these must have a non-negative numeric value */
double val;
- char *endp;
+ bool is_parsed;
- val = strtod(defGetString(def), &endp);
- if (*endp || val < 0)
+ is_parsed = parse_real(defGetString(def), &val, 0, NULL);
+ if (!is_parsed || val < 0)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s requires a non-negative numeric value",
@@ -134,23 +135,14 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
/* check list syntax, warn about uninstalled extensions */
(void) ExtractExtensionList(defGetString(def), true);
}
- else if (strcmp(def->defname, "fetch_size") == 0)
+ else if (strcmp(def->defname, "fetch_size") == 0 ||
+ strcmp(def->defname, "batch_size") == 0)
{
- int fetch_size;
+ int val;
+ bool is_parsed;
- fetch_size = strtol(defGetString(def), NULL, 10);
- if (fetch_size <= 0)
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("%s requires a non-negative integer value",
- def->defname)));
- }
- else if (strcmp(def->defname, "batch_size") == 0)
- {
- int batch_size;
-
- batch_size = strtol(defGetString(def), NULL, 10);
- if (batch_size <= 0)
+ is_parsed = parse_int(defGetString(def), &val, 0, NULL);
+ if (!is_parsed || val <= 0)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s requires a non-negative integer value",
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index c48a421e88..479043c189 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -4974,7 +4974,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
if (strcmp(def->defname, "fetch_size") == 0)
{
- fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fetch_size, 0, NULL);
break;
}
}
@@ -4984,7 +4984,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
if (strcmp(def->defname, "fetch_size") == 0)
{
- fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fetch_size, 0, NULL);
break;
}
}
@@ -5751,14 +5751,16 @@ apply_server_options(PgFdwRelationInfo *fpinfo)
if (strcmp(def->defname, "use_remote_estimate") == 0)
fpinfo->use_remote_estimate = defGetBoolean(def);
else if (strcmp(def->defname, "fdw_startup_cost") == 0)
- fpinfo->fdw_startup_cost = strtod(defGetString(def), NULL);
+ (void) parse_real(defGetString(def), &fpinfo->fdw_startup_cost, 0,
+ NULL);
else if (strcmp(def->defname, "fdw_tuple_cost") == 0)
- fpinfo->fdw_tuple_cost = strtod(defGetString(def), NULL);
+ (void) parse_real(defGetString(def), &fpinfo->fdw_tuple_cost, 0,
+ NULL);
else if (strcmp(def->defname, "extensions") == 0)
fpinfo->shippable_extensions =
ExtractExtensionList(defGetString(def), false);
else if (strcmp(def->defname, "fetch_size") == 0)
- fpinfo->fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fpinfo->fetch_size, 0, NULL);
else if (strcmp(def->defname, "async_capable") == 0)
fpinfo->async_capable = defGetBoolean(def);
}
@@ -5781,7 +5783,7 @@ apply_table_options(PgFdwRelationInfo *fpinfo)
if (strcmp(def->defname, "use_remote_estimate") == 0)
fpinfo->use_remote_estimate = defGetBoolean(def);
else if (strcmp(def->defname, "fetch_size") == 0)
- fpinfo->fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fpinfo->fetch_size, 0, NULL);
else if (strcmp(def->defname, "async_capable") == 0)
fpinfo->async_capable = defGetBoolean(def);
}
@@ -7289,7 +7291,7 @@ get_batch_size_option(Relation rel)
if (strcmp(def->defname, "batch_size") == 0)
{
- batch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &batch_size, 0, NULL);
break;
}
}
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 78379bdea5..61c15669d5 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3262,3 +3262,19 @@ DROP TABLE join_tbl;
ALTER SERVER loopback OPTIONS (DROP async_capable);
ALTER SERVER loopback2 OPTIONS (DROP async_capable);
+
+-- ===================================================================
+-- test invalid server and foreign table options
+-- ===================================================================
+-- Invalid fdw_startup_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_startup_cost '100$%$#$#');
+-- Invalid fdw_tuple_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_tuple_cost '100$%$#$#');
+-- Invalid fetch_size option
+CREATE FOREIGN TABLE inv_fsz (c1 int )
+ SERVER loopback OPTIONS (fetch_size '100$%$#$#');
+-- Invalid batch_size option
+CREATE FOREIGN TABLE inv_bsz (c1 int )
+ SERVER loopback OPTIONS (batch_size '100$%$#$#');
--
2.25.1
On 2021/05/19 14:34, Bharath Rupireddy wrote:
On Wed, May 19, 2021 at 8:28 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
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
Thanks all for your inputs. PSA which uses parse_int for
batch_size/fech_size and parse_real for fdw_startup_cost and
fdw_tuple_cost.
Thanks for updating the patch! It basically looks good to me.
- val = strtod(defGetString(def), &endp);
- if (*endp || val < 0)
+ is_parsed = parse_real(defGetString(def), &val, 0, NULL);
+ if (!is_parsed || val < 0)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s requires a non-negative numeric value",Isn't it better to check "!is_parsed" and "val < 0" separately like
reloptions.c does? That is, we should throw different error messages
for them?
ERRCODE_SYNTAX_ERROR seems strange for this type of error?
ERRCODE_INVALID_PARAMETER_VALUE is better and more proper?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Wed, May 19, 2021 at 5:02 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/05/19 14:34, Bharath Rupireddy wrote:
On Wed, May 19, 2021 at 8:28 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
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
Thanks all for your inputs. PSA which uses parse_int for
batch_size/fech_size and parse_real for fdw_startup_cost and
fdw_tuple_cost.Thanks for updating the patch! It basically looks good to me.
- val = strtod(defGetString(def), &endp); - if (*endp || val < 0) + is_parsed = parse_real(defGetString(def), &val, 0, NULL); + if (!is_parsed || val < 0) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("%s requires a non-negative numeric value",Isn't it better to check "!is_parsed" and "val < 0" separately like
reloptions.c does? That is, we should throw different error messages
for them?ERRCODE_SYNTAX_ERROR seems strange for this type of error?
ERRCODE_INVALID_PARAMETER_VALUE is better and more proper?
Thanks for the comments. I added separate messages, changed the error
code from ERRCODE_SYNTAX_ERROR to ERRCODE_INVALID_PARAMETER_VALUE and
also quoted the option name in the error message. PSA v3 patch.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v3-0001-Tighten-up-batch_size-fetch_size-options-against-.patchapplication/octet-stream; name=v3-0001-Tighten-up-batch_size-fetch_size-options-against-.patchDownload
From 4e475d1d55966b8057b9808a3439b2ca16fa787e Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Wed, 19 May 2021 21:29:16 +0530
Subject: [PATCH v3] Tighten up batch_size, fetch_size options against
non-numeric values
It looks like the values such as '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. This is because endptr is not used while converting strings
to integers using strtol.
Use parse_int function instead of strtol as it serves the purpose by
returning false in case if it is unable to convert the string to
integer. Note that this function also rounds off the values such as
'100.456' to 100 and '100.567' or '100.678' to 101.
While on this, use parse_real for fdw_startup_cost and fdw_tuple_cost
options.
Since parse_int and parse_real are being used for reloptions and GUCs,
it is more appropriate to use in postgres_fdw rather than using
strtol and strtod directly.
---
.../postgres_fdw/expected/postgres_fdw.out | 19 ++++++++
contrib/postgres_fdw/option.c | 44 +++++++++++--------
contrib/postgres_fdw/postgres_fdw.c | 16 ++++---
contrib/postgres_fdw/sql/postgres_fdw.sql | 16 +++++++
4 files changed, 69 insertions(+), 26 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 7df30010f2..ed2b590361 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10231,3 +10231,22 @@ DROP TABLE result_tbl;
DROP TABLE join_tbl;
ALTER SERVER loopback OPTIONS (DROP async_capable);
ALTER SERVER loopback2 OPTIONS (DROP async_capable);
+-- ===================================================================
+-- test invalid server and foreign table options
+-- ===================================================================
+-- Invalid fdw_startup_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_startup_cost '100$%$#$#');
+ERROR: invalid numeric value for option "fdw_startup_cost"
+-- Invalid fdw_tuple_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_tuple_cost '100$%$#$#');
+ERROR: invalid numeric value for option "fdw_tuple_cost"
+-- Invalid fetch_size option
+CREATE FOREIGN TABLE inv_fsz (c1 int )
+ SERVER loopback OPTIONS (fetch_size '100$%$#$#');
+ERROR: invalid integer value for option "fetch_size"
+-- Invalid batch_size option
+CREATE FOREIGN TABLE inv_bsz (c1 int )
+ SERVER loopback OPTIONS (batch_size '100$%$#$#');
+ERROR: invalid integer value for option "batch_size"
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 672b55a808..d39113ca94 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -20,6 +20,7 @@
#include "commands/extension.h"
#include "postgres_fdw.h"
#include "utils/builtins.h"
+#include "utils/guc.h"
#include "utils/varlena.h"
/*
@@ -120,13 +121,20 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
{
/* these must have a non-negative numeric value */
double val;
- char *endp;
+ bool is_parsed;
- val = strtod(defGetString(def), &endp);
- if (*endp || val < 0)
+ is_parsed = parse_real(defGetString(def), &val, 0, NULL);
+
+ if (!is_parsed)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid numeric value for option \"%s\"",
+ def->defname)));
+
+ if (val < 0)
ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("%s requires a non-negative numeric value",
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" requires a non-negative numeric value",
def->defname)));
}
else if (strcmp(def->defname, "extensions") == 0)
@@ -134,26 +142,24 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
/* check list syntax, warn about uninstalled extensions */
(void) ExtractExtensionList(defGetString(def), true);
}
- else if (strcmp(def->defname, "fetch_size") == 0)
+ else if (strcmp(def->defname, "fetch_size") == 0 ||
+ strcmp(def->defname, "batch_size") == 0)
{
- int fetch_size;
+ int val;
+ bool is_parsed;
- fetch_size = strtol(defGetString(def), NULL, 10);
- if (fetch_size <= 0)
+ is_parsed = parse_int(defGetString(def), &val, 0, NULL);
+
+ if (!is_parsed)
ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("%s requires a non-negative integer value",
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid integer value for option \"%s\"",
def->defname)));
- }
- else if (strcmp(def->defname, "batch_size") == 0)
- {
- int batch_size;
- batch_size = strtol(defGetString(def), NULL, 10);
- if (batch_size <= 0)
+ if (val <= 0)
ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("%s requires a non-negative integer value",
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" requires a non-negative integer value",
def->defname)));
}
else if (strcmp(def->defname, "password_required") == 0)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index c48a421e88..479043c189 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -4974,7 +4974,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
if (strcmp(def->defname, "fetch_size") == 0)
{
- fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fetch_size, 0, NULL);
break;
}
}
@@ -4984,7 +4984,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
if (strcmp(def->defname, "fetch_size") == 0)
{
- fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fetch_size, 0, NULL);
break;
}
}
@@ -5751,14 +5751,16 @@ apply_server_options(PgFdwRelationInfo *fpinfo)
if (strcmp(def->defname, "use_remote_estimate") == 0)
fpinfo->use_remote_estimate = defGetBoolean(def);
else if (strcmp(def->defname, "fdw_startup_cost") == 0)
- fpinfo->fdw_startup_cost = strtod(defGetString(def), NULL);
+ (void) parse_real(defGetString(def), &fpinfo->fdw_startup_cost, 0,
+ NULL);
else if (strcmp(def->defname, "fdw_tuple_cost") == 0)
- fpinfo->fdw_tuple_cost = strtod(defGetString(def), NULL);
+ (void) parse_real(defGetString(def), &fpinfo->fdw_tuple_cost, 0,
+ NULL);
else if (strcmp(def->defname, "extensions") == 0)
fpinfo->shippable_extensions =
ExtractExtensionList(defGetString(def), false);
else if (strcmp(def->defname, "fetch_size") == 0)
- fpinfo->fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fpinfo->fetch_size, 0, NULL);
else if (strcmp(def->defname, "async_capable") == 0)
fpinfo->async_capable = defGetBoolean(def);
}
@@ -5781,7 +5783,7 @@ apply_table_options(PgFdwRelationInfo *fpinfo)
if (strcmp(def->defname, "use_remote_estimate") == 0)
fpinfo->use_remote_estimate = defGetBoolean(def);
else if (strcmp(def->defname, "fetch_size") == 0)
- fpinfo->fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fpinfo->fetch_size, 0, NULL);
else if (strcmp(def->defname, "async_capable") == 0)
fpinfo->async_capable = defGetBoolean(def);
}
@@ -7289,7 +7291,7 @@ get_batch_size_option(Relation rel)
if (strcmp(def->defname, "batch_size") == 0)
{
- batch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &batch_size, 0, NULL);
break;
}
}
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 78379bdea5..61c15669d5 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3262,3 +3262,19 @@ DROP TABLE join_tbl;
ALTER SERVER loopback OPTIONS (DROP async_capable);
ALTER SERVER loopback2 OPTIONS (DROP async_capable);
+
+-- ===================================================================
+-- test invalid server and foreign table options
+-- ===================================================================
+-- Invalid fdw_startup_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_startup_cost '100$%$#$#');
+-- Invalid fdw_tuple_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_tuple_cost '100$%$#$#');
+-- Invalid fetch_size option
+CREATE FOREIGN TABLE inv_fsz (c1 int )
+ SERVER loopback OPTIONS (fetch_size '100$%$#$#');
+-- Invalid batch_size option
+CREATE FOREIGN TABLE inv_bsz (c1 int )
+ SERVER loopback OPTIONS (batch_size '100$%$#$#');
--
2.25.1
On 2021/05/20 1:01, Bharath Rupireddy wrote:
Thanks for the comments. I added separate messages, changed the error
code from ERRCODE_SYNTAX_ERROR to ERRCODE_INVALID_PARAMETER_VALUE and
also quoted the option name in the error message. PSA v3 patch.
Thanks for updating the patch!
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid numeric value for option \"%s\"",
+ def->defname)));In reloptions.c, when parse_real() fails to parse the input, the error message
"invalid value for floating point option..." is output.
For the sake of consistency, we should use the same error message here?
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("%s requires a non-negative integer value",
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid integer value for option \"%s\"",IMO the error message should be "invalid value for integer option..." here
because of the same reason I told above. Thought?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Wed, Jun 30, 2021 at 5:53 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/05/20 1:01, Bharath Rupireddy wrote:
Thanks for the comments. I added separate messages, changed the error
code from ERRCODE_SYNTAX_ERROR to ERRCODE_INVALID_PARAMETER_VALUE and
also quoted the option name in the error message. PSA v3 patch.Thanks for updating the patch!
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("invalid numeric value for option \"%s\"", + def->defname)));In reloptions.c, when parse_real() fails to parse the input, the error message
"invalid value for floating point option..." is output.
For the sake of consistency, we should use the same error message here?
Actually, there's an existing error message errmsg("%s requires a
non-negative numeric value" that used "numeric value". If we were to
change errmsg("invalid numeric value for option \"%s\"", to
errmsg("invalid value for floating point option \"%s\"",, then we
might have to change the existing message. And also, the docs use
"numeric value" for fdw_startup_cost and fdw_tuple_cost. IMO, let's go
with errmsg("invalid value for numeric option \"%s\": %s",.
- (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("%s requires a non-negative integer value", + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("invalid integer value for option \"%s\"",IMO the error message should be "invalid value for integer option..." here
because of the same reason I told above. Thought?
Changed.
PSA v4.
Regards,
Bharath Rupireddy.
Attachments:
v4-0001-Tighten-up-batch_size-fetch_size-options-against-.patchapplication/octet-stream; name=v4-0001-Tighten-up-batch_size-fetch_size-options-against-.patchDownload
From 39d251a1e126f9a3fcea7e89aa34c5576a4e88e9 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Wed, 30 Jun 2021 14:30:20 +0000
Subject: [PATCH v4] Tighten up batch_size, fetch_size options against
non-numeric values
It looks like the values such as '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. This is because endptr is not used while converting strings
to integers using strtol.
Use parse_int function instead of strtol as it serves the purpose by
returning false in case if it is unable to convert the string to
integer. Note that this function also rounds off the values such as
'100.456' to 100 and '100.567' or '100.678' to 101.
While on this, use parse_real for fdw_startup_cost and fdw_tuple_cost
options.
Since parse_int and parse_real are being used for reloptions and GUCs,
it is more appropriate to use in postgres_fdw rather than using
strtol and strtod directly.
---
.../postgres_fdw/expected/postgres_fdw.out | 19 +++++++
contrib/postgres_fdw/option.c | 52 +++++++++++--------
contrib/postgres_fdw/postgres_fdw.c | 16 +++---
contrib/postgres_fdw/sql/postgres_fdw.sql | 16 ++++++
4 files changed, 75 insertions(+), 28 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 31b5de91ad..966be24fdc 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10478,3 +10478,22 @@ DROP TABLE result_tbl;
DROP TABLE join_tbl;
ALTER SERVER loopback OPTIONS (DROP async_capable);
ALTER SERVER loopback2 OPTIONS (DROP async_capable);
+-- ===================================================================
+-- test invalid server and foreign table options
+-- ===================================================================
+-- Invalid fdw_startup_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_startup_cost '100$%$#$#');
+ERROR: invalid value for numeric option "fdw_startup_cost": 100$%$#$#
+-- Invalid fdw_tuple_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_tuple_cost '100$%$#$#');
+ERROR: invalid value for numeric option "fdw_tuple_cost": 100$%$#$#
+-- Invalid fetch_size option
+CREATE FOREIGN TABLE inv_fsz (c1 int )
+ SERVER loopback OPTIONS (fetch_size '100$%$#$#');
+ERROR: invalid value for integer option "fetch_size": 100$%$#$#
+-- Invalid batch_size option
+CREATE FOREIGN TABLE inv_bsz (c1 int )
+ SERVER loopback OPTIONS (batch_size '100$%$#$#');
+ERROR: invalid value for integer option "batch_size": 100$%$#$#
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 672b55a808..a56818dcf7 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -20,6 +20,7 @@
#include "commands/extension.h"
#include "postgres_fdw.h"
#include "utils/builtins.h"
+#include "utils/guc.h"
#include "utils/varlena.h"
/*
@@ -119,14 +120,23 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
strcmp(def->defname, "fdw_tuple_cost") == 0)
{
/* these must have a non-negative numeric value */
- double val;
- char *endp;
+ char *value;
+ double real_val;
+ bool is_parsed;
- val = strtod(defGetString(def), &endp);
- if (*endp || val < 0)
+ value = defGetString(def);
+ is_parsed = parse_real(value, &real_val, 0, NULL);
+
+ if (!is_parsed)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid value for numeric option \"%s\": %s",
+ def->defname, value)));
+
+ if (real_val < 0)
ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("%s requires a non-negative numeric value",
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" requires a non-negative numeric value",
def->defname)));
}
else if (strcmp(def->defname, "extensions") == 0)
@@ -134,26 +144,26 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
/* check list syntax, warn about uninstalled extensions */
(void) ExtractExtensionList(defGetString(def), true);
}
- else if (strcmp(def->defname, "fetch_size") == 0)
+ else if (strcmp(def->defname, "fetch_size") == 0 ||
+ strcmp(def->defname, "batch_size") == 0)
{
- int fetch_size;
+ char *value;
+ int int_val;
+ bool is_parsed;
+
+ value = defGetString(def);
+ is_parsed = parse_int(value, &int_val, 0, NULL);
- fetch_size = strtol(defGetString(def), NULL, 10);
- if (fetch_size <= 0)
+ if (!is_parsed)
ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("%s requires a non-negative integer value",
- def->defname)));
- }
- else if (strcmp(def->defname, "batch_size") == 0)
- {
- int batch_size;
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid value for integer option \"%s\": %s",
+ def->defname, value)));
- batch_size = strtol(defGetString(def), NULL, 10);
- if (batch_size <= 0)
+ if (int_val <= 0)
ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("%s requires a non-negative integer value",
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" requires a non-negative integer value",
def->defname)));
}
else if (strcmp(def->defname, "password_required") == 0)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index fafbab6b02..ad0fba317c 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -5032,7 +5032,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
if (strcmp(def->defname, "fetch_size") == 0)
{
- fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fetch_size, 0, NULL);
break;
}
}
@@ -5042,7 +5042,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
if (strcmp(def->defname, "fetch_size") == 0)
{
- fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fetch_size, 0, NULL);
break;
}
}
@@ -5809,14 +5809,16 @@ apply_server_options(PgFdwRelationInfo *fpinfo)
if (strcmp(def->defname, "use_remote_estimate") == 0)
fpinfo->use_remote_estimate = defGetBoolean(def);
else if (strcmp(def->defname, "fdw_startup_cost") == 0)
- fpinfo->fdw_startup_cost = strtod(defGetString(def), NULL);
+ (void) parse_real(defGetString(def), &fpinfo->fdw_startup_cost, 0,
+ NULL);
else if (strcmp(def->defname, "fdw_tuple_cost") == 0)
- fpinfo->fdw_tuple_cost = strtod(defGetString(def), NULL);
+ (void) parse_real(defGetString(def), &fpinfo->fdw_tuple_cost, 0,
+ NULL);
else if (strcmp(def->defname, "extensions") == 0)
fpinfo->shippable_extensions =
ExtractExtensionList(defGetString(def), false);
else if (strcmp(def->defname, "fetch_size") == 0)
- fpinfo->fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fpinfo->fetch_size, 0, NULL);
else if (strcmp(def->defname, "async_capable") == 0)
fpinfo->async_capable = defGetBoolean(def);
}
@@ -5839,7 +5841,7 @@ apply_table_options(PgFdwRelationInfo *fpinfo)
if (strcmp(def->defname, "use_remote_estimate") == 0)
fpinfo->use_remote_estimate = defGetBoolean(def);
else if (strcmp(def->defname, "fetch_size") == 0)
- fpinfo->fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fpinfo->fetch_size, 0, NULL);
else if (strcmp(def->defname, "async_capable") == 0)
fpinfo->async_capable = defGetBoolean(def);
}
@@ -7347,7 +7349,7 @@ get_batch_size_option(Relation rel)
if (strcmp(def->defname, "batch_size") == 0)
{
- batch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &batch_size, 0, NULL);
break;
}
}
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 286dd99573..3b6bfc5218 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3337,3 +3337,19 @@ DROP TABLE join_tbl;
ALTER SERVER loopback OPTIONS (DROP async_capable);
ALTER SERVER loopback2 OPTIONS (DROP async_capable);
+
+-- ===================================================================
+-- test invalid server and foreign table options
+-- ===================================================================
+-- Invalid fdw_startup_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_startup_cost '100$%$#$#');
+-- Invalid fdw_tuple_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_tuple_cost '100$%$#$#');
+-- Invalid fetch_size option
+CREATE FOREIGN TABLE inv_fsz (c1 int )
+ SERVER loopback OPTIONS (fetch_size '100$%$#$#');
+-- Invalid batch_size option
+CREATE FOREIGN TABLE inv_bsz (c1 int )
+ SERVER loopback OPTIONS (batch_size '100$%$#$#');
--
2.25.1
On 2021/06/30 23:31, Bharath Rupireddy wrote:
On Wed, Jun 30, 2021 at 5:53 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/05/20 1:01, Bharath Rupireddy wrote:
Thanks for the comments. I added separate messages, changed the error
code from ERRCODE_SYNTAX_ERROR to ERRCODE_INVALID_PARAMETER_VALUE and
also quoted the option name in the error message. PSA v3 patch.Thanks for updating the patch!
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("invalid numeric value for option \"%s\"", + def->defname)));In reloptions.c, when parse_real() fails to parse the input, the error message
"invalid value for floating point option..." is output.
For the sake of consistency, we should use the same error message here?Actually, there's an existing error message errmsg("%s requires a
non-negative numeric value" that used "numeric value". If we were to
change errmsg("invalid numeric value for option \"%s\"", to
errmsg("invalid value for floating point option \"%s\"",, then we
might have to change the existing message. And also, the docs use
"numeric value" for fdw_startup_cost and fdw_tuple_cost.
The recent commit 61d599ede7 documented that the type of those options is
floating point. But the docs still use "is a numeric value" in the descriptions
of them. Probably it should be replaced with "is a floating point value" there.
If we do this, isn't it better to use "floating point" even in the error message?
IMO, let's go
with errmsg("invalid value for numeric option \"%s\": %s",.- (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("%s requires a non-negative integer value", + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("invalid integer value for option \"%s\"",IMO the error message should be "invalid value for integer option..." here
because of the same reason I told above. Thought?Changed.
PSA v4.
Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Thu, Jul 1, 2021 at 8:23 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
The recent commit 61d599ede7 documented that the type of those options is
floating point. But the docs still use "is a numeric value" in the descriptions
of them. Probably it should be replaced with "is a floating point value" there.
If we do this, isn't it better to use "floating point" even in the error message?
Agreed. PSA v5 patch.
Regards,
Bharath Rupireddy.
Attachments:
v5-0001-Tighten-up-batch_size-fetch_size-options-against-.patchapplication/octet-stream; name=v5-0001-Tighten-up-batch_size-fetch_size-options-against-.patchDownload
From 83cb59dd4957112fda9bcff9d2b5846a6b4e7f32 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Thu, 1 Jul 2021 03:55:21 +0000
Subject: [PATCH v5] Tighten up batch_size, fetch_size options against
non-numeric values
It looks like the values such as '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. This is because endptr is not used while converting strings
to integers using strtol.
Use parse_int function instead of strtol as it serves the purpose by
returning false in case if it is unable to convert the string to
integer. Note that this function also rounds off the values such as
'100.456' to 100 and '100.567' or '100.678' to 101.
While on this, use parse_real for fdw_startup_cost and fdw_tuple_cost
options.
Since parse_int and parse_real are being used for reloptions and GUCs,
it is more appropriate to use in postgres_fdw rather than using
strtol and strtod directly.
---
.../postgres_fdw/expected/postgres_fdw.out | 19 +++++++
contrib/postgres_fdw/option.c | 52 +++++++++++--------
contrib/postgres_fdw/postgres_fdw.c | 16 +++---
contrib/postgres_fdw/sql/postgres_fdw.sql | 16 ++++++
doc/src/sgml/postgres-fdw.sgml | 14 ++---
5 files changed, 82 insertions(+), 35 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 31b5de91ad..d8173d4cc4 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10478,3 +10478,22 @@ DROP TABLE result_tbl;
DROP TABLE join_tbl;
ALTER SERVER loopback OPTIONS (DROP async_capable);
ALTER SERVER loopback2 OPTIONS (DROP async_capable);
+-- ===================================================================
+-- test invalid server and foreign table options
+-- ===================================================================
+-- Invalid fdw_startup_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_startup_cost '100$%$#$#');
+ERROR: invalid value for floating point option "fdw_startup_cost": 100$%$#$#
+-- Invalid fdw_tuple_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_tuple_cost '100$%$#$#');
+ERROR: invalid value for floating point option "fdw_tuple_cost": 100$%$#$#
+-- Invalid fetch_size option
+CREATE FOREIGN TABLE inv_fsz (c1 int )
+ SERVER loopback OPTIONS (fetch_size '100$%$#$#');
+ERROR: invalid value for integer option "fetch_size": 100$%$#$#
+-- Invalid batch_size option
+CREATE FOREIGN TABLE inv_bsz (c1 int )
+ SERVER loopback OPTIONS (batch_size '100$%$#$#');
+ERROR: invalid value for integer option "batch_size": 100$%$#$#
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 672b55a808..a7abbae5a1 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -20,6 +20,7 @@
#include "commands/extension.h"
#include "postgres_fdw.h"
#include "utils/builtins.h"
+#include "utils/guc.h"
#include "utils/varlena.h"
/*
@@ -119,14 +120,23 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
strcmp(def->defname, "fdw_tuple_cost") == 0)
{
/* these must have a non-negative numeric value */
- double val;
- char *endp;
+ char *value;
+ double real_val;
+ bool is_parsed;
- val = strtod(defGetString(def), &endp);
- if (*endp || val < 0)
+ value = defGetString(def);
+ is_parsed = parse_real(value, &real_val, 0, NULL);
+
+ if (!is_parsed)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid value for floating point option \"%s\": %s",
+ def->defname, value)));
+
+ if (real_val < 0)
ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("%s requires a non-negative numeric value",
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" requires a non-negative floating point value",
def->defname)));
}
else if (strcmp(def->defname, "extensions") == 0)
@@ -134,26 +144,26 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
/* check list syntax, warn about uninstalled extensions */
(void) ExtractExtensionList(defGetString(def), true);
}
- else if (strcmp(def->defname, "fetch_size") == 0)
+ else if (strcmp(def->defname, "fetch_size") == 0 ||
+ strcmp(def->defname, "batch_size") == 0)
{
- int fetch_size;
+ char *value;
+ int int_val;
+ bool is_parsed;
+
+ value = defGetString(def);
+ is_parsed = parse_int(value, &int_val, 0, NULL);
- fetch_size = strtol(defGetString(def), NULL, 10);
- if (fetch_size <= 0)
+ if (!is_parsed)
ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("%s requires a non-negative integer value",
- def->defname)));
- }
- else if (strcmp(def->defname, "batch_size") == 0)
- {
- int batch_size;
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid value for integer option \"%s\": %s",
+ def->defname, value)));
- batch_size = strtol(defGetString(def), NULL, 10);
- if (batch_size <= 0)
+ if (int_val <= 0)
ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("%s requires a non-negative integer value",
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" requires a non-negative integer value",
def->defname)));
}
else if (strcmp(def->defname, "password_required") == 0)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index fafbab6b02..ad0fba317c 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -5032,7 +5032,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
if (strcmp(def->defname, "fetch_size") == 0)
{
- fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fetch_size, 0, NULL);
break;
}
}
@@ -5042,7 +5042,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
if (strcmp(def->defname, "fetch_size") == 0)
{
- fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fetch_size, 0, NULL);
break;
}
}
@@ -5809,14 +5809,16 @@ apply_server_options(PgFdwRelationInfo *fpinfo)
if (strcmp(def->defname, "use_remote_estimate") == 0)
fpinfo->use_remote_estimate = defGetBoolean(def);
else if (strcmp(def->defname, "fdw_startup_cost") == 0)
- fpinfo->fdw_startup_cost = strtod(defGetString(def), NULL);
+ (void) parse_real(defGetString(def), &fpinfo->fdw_startup_cost, 0,
+ NULL);
else if (strcmp(def->defname, "fdw_tuple_cost") == 0)
- fpinfo->fdw_tuple_cost = strtod(defGetString(def), NULL);
+ (void) parse_real(defGetString(def), &fpinfo->fdw_tuple_cost, 0,
+ NULL);
else if (strcmp(def->defname, "extensions") == 0)
fpinfo->shippable_extensions =
ExtractExtensionList(defGetString(def), false);
else if (strcmp(def->defname, "fetch_size") == 0)
- fpinfo->fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fpinfo->fetch_size, 0, NULL);
else if (strcmp(def->defname, "async_capable") == 0)
fpinfo->async_capable = defGetBoolean(def);
}
@@ -5839,7 +5841,7 @@ apply_table_options(PgFdwRelationInfo *fpinfo)
if (strcmp(def->defname, "use_remote_estimate") == 0)
fpinfo->use_remote_estimate = defGetBoolean(def);
else if (strcmp(def->defname, "fetch_size") == 0)
- fpinfo->fetch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &fpinfo->fetch_size, 0, NULL);
else if (strcmp(def->defname, "async_capable") == 0)
fpinfo->async_capable = defGetBoolean(def);
}
@@ -7347,7 +7349,7 @@ get_batch_size_option(Relation rel)
if (strcmp(def->defname, "batch_size") == 0)
{
- batch_size = strtol(defGetString(def), NULL, 10);
+ (void) parse_int(defGetString(def), &batch_size, 0, NULL);
break;
}
}
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 286dd99573..3b6bfc5218 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3337,3 +3337,19 @@ DROP TABLE join_tbl;
ALTER SERVER loopback OPTIONS (DROP async_capable);
ALTER SERVER loopback2 OPTIONS (DROP async_capable);
+
+-- ===================================================================
+-- test invalid server and foreign table options
+-- ===================================================================
+-- Invalid fdw_startup_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_startup_cost '100$%$#$#');
+-- Invalid fdw_tuple_cost option
+CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS(fdw_tuple_cost '100$%$#$#');
+-- Invalid fetch_size option
+CREATE FOREIGN TABLE inv_fsz (c1 int )
+ SERVER loopback OPTIONS (fetch_size '100$%$#$#');
+-- Invalid batch_size option
+CREATE FOREIGN TABLE inv_bsz (c1 int )
+ SERVER loopback OPTIONS (batch_size '100$%$#$#');
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index d7d2baafc9..c9fce77599 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -266,11 +266,11 @@ OPTIONS (ADD password_required 'false');
<term><literal>fdw_startup_cost</literal> (<type>floating point</type>)</term>
<listitem>
<para>
- This option, which can be specified for a foreign server, is a numeric
- value that is added to the estimated startup cost of any foreign-table
- scan on that server. This represents the additional overhead of
- establishing a connection, parsing and planning the query on the
- remote side, etc.
+ This option, which can be specified for a foreign server, is a floating
+ point value that is added to the estimated startup cost of any
+ foreign-table scan on that server. This represents the additional
+ overhead of establishing a connection, parsing and planning the query on
+ the remote side, etc.
The default value is <literal>100</literal>.
</para>
</listitem>
@@ -280,8 +280,8 @@ OPTIONS (ADD password_required 'false');
<term><literal>fdw_tuple_cost</literal> (<type>floating point</type>)</term>
<listitem>
<para>
- This option, which can be specified for a foreign server, is a numeric
- value that is used as extra cost per-tuple for foreign-table
+ This option, which can be specified for a foreign server, is a floating
+ point value that is used as extra cost per-tuple for foreign-table
scans on that server. This represents the additional overhead of
data transfer between servers. You might increase or decrease this
number to reflect higher or lower network delay to the remote server.
--
2.25.1
On 2021/07/01 13:16, Bharath Rupireddy wrote:
On Thu, Jul 1, 2021 at 8:23 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
The recent commit 61d599ede7 documented that the type of those options is
floating point. But the docs still use "is a numeric value" in the descriptions
of them. Probably it should be replaced with "is a floating point value" there.
If we do this, isn't it better to use "floating point" even in the error message?Agreed. PSA v5 patch.
Thanks for updating the patch! LGTM.
Barring any objection, I will commit this patch.
One question is; should we back-patch this? This is not a bug fix,
so I'm not sure if it's worth back-patching that to already-released versions.
But it may be better to do that to v14.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Thu, Jul 1, 2021 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/07/01 13:16, Bharath Rupireddy wrote:
On Thu, Jul 1, 2021 at 8:23 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
The recent commit 61d599ede7 documented that the type of those options is
floating point. But the docs still use "is a numeric value" in the descriptions
of them. Probably it should be replaced with "is a floating point value" there.
If we do this, isn't it better to use "floating point" even in the error message?Agreed. PSA v5 patch.
Thanks for updating the patch! LGTM.
Barring any objection, I will commit this patch.
Thanks.
One question is; should we back-patch this? This is not a bug fix,
so I'm not sure if it's worth back-patching that to already-released versions.
But it may be better to do that to v14.
IMO, it's a good-to-have fix in v14. But, -1 for backpatching to v13
and lower branches.
Regards,
Bharath Rupireddy.
On 2021/07/01 21:41, Bharath Rupireddy wrote:
On Thu, Jul 1, 2021 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/07/01 13:16, Bharath Rupireddy wrote:
On Thu, Jul 1, 2021 at 8:23 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
The recent commit 61d599ede7 documented that the type of those options is
floating point. But the docs still use "is a numeric value" in the descriptions
of them. Probably it should be replaced with "is a floating point value" there.
If we do this, isn't it better to use "floating point" even in the error message?Agreed. PSA v5 patch.
Thanks for updating the patch! LGTM.
Barring any objection, I will commit this patch.Thanks.
One question is; should we back-patch this? This is not a bug fix,
so I'm not sure if it's worth back-patching that to already-released versions.
But it may be better to do that to v14.IMO, it's a good-to-have fix in v14. But, -1 for backpatching to v13
and lower branches.
Agreed. So I pushed the patch to master and v14. Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION