postgres_fdw: Tighten up allowed values for batch_size, fetch_size options.

Previously the values such as '100$%$#$#', '9,223,372,' were 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 was because endptr was not used
while converting strings to integers using strtol.

This commit changes the logic so that it uses 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.

Back-patch to v14.

Author: Bharath Rupireddy
Reviewed-by: Ashutosh Bapat, Tom Lane, Kyotaro Horiguchi, Fujii Masao
Discussion: https://postgr.es/m/CALj2ACVMO6wY5Pc4oe1OCgUOAtdjHuFsBDw8R5uoYR86eWFQDA@mail.gmail.com

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 25112df916cd1ef144da456dedbe1e90ed319634..b510322c4ea8e723e586735c8fc58eb61b93a7ae 100644 (file)
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10480,3 +10480,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 672b55a808f4fdb7e2d9142f8247d8ac27f75249..4593cbc540e4ca73a2b50b948ceaf21ff8e78874 100644 (file)
--- 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 a5afac47ebda86bd42700ea661f844159d96a8d9..5cf10402a2023e1406f72a9f48a41072187009e3 100644 (file)
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -5024,7 +5024,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;
            }
        }
@@ -5034,7 +5034,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;
            }
        }
@@ -5801,14 +5801,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);
    }
@@ -5831,7 +5833,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);
    }
@@ -7341,7 +7343,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 95862e38ed9276d6420824cf580ec2be25832b9b..911f171d812e743be23fa49c0c5cef39a5cba27b 100644 (file)
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3339,3 +3339,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 d96c3d0f0cd3be00cea6311c1238c0d9896a765e..44acf7c586fcd06cb0df025ab29215e74aabdbaf 100644 (file)
--- 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></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></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.