Skip to content

Commit d854720

Browse files
MasaoFujiiMasaoFujii
committed
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
1 parent 9fd8557 commit d854720

File tree

5 files changed

+82
-35
lines changed

5 files changed

+82
-35
lines changed

‎contrib/postgres_fdw/expected/postgres_fdw.out

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10480,3 +10480,22 @@ DROP TABLE result_tbl;
1048010480
DROP TABLE join_tbl;
1048110481
ALTER SERVER loopback OPTIONS (DROP async_capable);
1048210482
ALTER SERVER loopback2 OPTIONS (DROP async_capable);
10483+
-- ===================================================================
10484+
-- test invalid server and foreign table options
10485+
-- ===================================================================
10486+
-- Invalid fdw_startup_cost option
10487+
CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
10488+
OPTIONS(fdw_startup_cost '100$%$#$#');
10489+
ERROR: invalid value for floating point option "fdw_startup_cost": 100$%$#$#
10490+
-- Invalid fdw_tuple_cost option
10491+
CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
10492+
OPTIONS(fdw_tuple_cost '100$%$#$#');
10493+
ERROR: invalid value for floating point option "fdw_tuple_cost": 100$%$#$#
10494+
-- Invalid fetch_size option
10495+
CREATE FOREIGN TABLE inv_fsz (c1 int )
10496+
SERVER loopback OPTIONS (fetch_size '100$%$#$#');
10497+
ERROR: invalid value for integer option "fetch_size": 100$%$#$#
10498+
-- Invalid batch_size option
10499+
CREATE FOREIGN TABLE inv_bsz (c1 int )
10500+
SERVER loopback OPTIONS (batch_size '100$%$#$#');
10501+
ERROR: invalid value for integer option "batch_size": 100$%$#$#

‎contrib/postgres_fdw/option.c

Lines changed: 31 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include "commands/extension.h"
2121
#include "postgres_fdw.h"
2222
#include "utils/builtins.h"
23+
#include "utils/guc.h"
2324
#include "utils/varlena.h"
2425

2526
/*
@@ -119,41 +120,50 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
119120
strcmp(def->defname, "fdw_tuple_cost") == 0)
120121
{
121122
/* these must have a non-negative numeric value */
122-
double val;
123-
char *endp;
123+
char *value;
124+
double real_val;
125+
bool is_parsed;
124126

125-
val = strtod(defGetString(def), &endp);
126-
if (*endp || val < 0)
127+
value = defGetString(def);
128+
is_parsed = parse_real(value, &real_val, 0, NULL);
129+
130+
if (!is_parsed)
131+
ereport(ERROR,
132+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
133+
errmsg("invalid value for floating point option \"%s\": %s",
134+
def->defname, value)));
135+
136+
if (real_val < 0)
127137
ereport(ERROR,
128-
(errcode(ERRCODE_SYNTAX_ERROR),
129-
errmsg("%s requires a non-negative numeric value",
138+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
139+
errmsg("\"%s\" requires a non-negative floating point value",
130140
def->defname)));
131141
}
132142
else if (strcmp(def->defname, "extensions") == 0)
133143
{
134144
/* check list syntax, warn about uninstalled extensions */
135145
(void) ExtractExtensionList(defGetString(def), true);
136146
}
137-
else if (strcmp(def->defname, "fetch_size") == 0)
147+
else if (strcmp(def->defname, "fetch_size") == 0 ||
148+
strcmp(def->defname, "batch_size") == 0)
138149
{
139-
int fetch_size;
150+
char *value;
151+
int int_val;
152+
bool is_parsed;
153+
154+
value = defGetString(def);
155+
is_parsed = parse_int(value, &int_val, 0, NULL);
140156

141-
fetch_size = strtol(defGetString(def), NULL, 10);
142-
if (fetch_size <= 0)
157+
if (!is_parsed)
143158
ereport(ERROR,
144-
(errcode(ERRCODE_SYNTAX_ERROR),
145-
errmsg("%s requires a non-negative integer value",
146-
def->defname)));
147-
}
148-
else if (strcmp(def->defname, "batch_size") == 0)
149-
{
150-
int batch_size;
159+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
160+
errmsg("invalid value for integer option \"%s\": %s",
161+
def->defname, value)));
151162

152-
batch_size = strtol(defGetString(def), NULL, 10);
153-
if (batch_size <= 0)
163+
if (int_val <= 0)
154164
ereport(ERROR,
155-
(errcode(ERRCODE_SYNTAX_ERROR),
156-
errmsg("%s requires a non-negative integer value",
165+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
166+
errmsg("\"%s\" requires a non-negative integer value",
157167
def->defname)));
158168
}
159169
else if (strcmp(def->defname, "password_required") == 0)

‎contrib/postgres_fdw/postgres_fdw.c

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5024,7 +5024,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
50245024

50255025
if (strcmp(def->defname, "fetch_size") == 0)
50265026
{
5027-
fetch_size = strtol(defGetString(def), NULL, 10);
5027+
(void) parse_int(defGetString(def), &fetch_size, 0, NULL);
50285028
break;
50295029
}
50305030
}
@@ -5034,7 +5034,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
50345034

50355035
if (strcmp(def->defname, "fetch_size") == 0)
50365036
{
5037-
fetch_size = strtol(defGetString(def), NULL, 10);
5037+
(void) parse_int(defGetString(def), &fetch_size, 0, NULL);
50385038
break;
50395039
}
50405040
}
@@ -5801,14 +5801,16 @@ apply_server_options(PgFdwRelationInfo *fpinfo)
58015801
if (strcmp(def->defname, "use_remote_estimate") == 0)
58025802
fpinfo->use_remote_estimate = defGetBoolean(def);
58035803
else if (strcmp(def->defname, "fdw_startup_cost") == 0)
5804-
fpinfo->fdw_startup_cost = strtod(defGetString(def), NULL);
5804+
(void) parse_real(defGetString(def), &fpinfo->fdw_startup_cost, 0,
5805+
NULL);
58055806
else if (strcmp(def->defname, "fdw_tuple_cost") == 0)
5806-
fpinfo->fdw_tuple_cost = strtod(defGetString(def), NULL);
5807+
(void) parse_real(defGetString(def), &fpinfo->fdw_tuple_cost, 0,
5808+
NULL);
58075809
else if (strcmp(def->defname, "extensions") == 0)
58085810
fpinfo->shippable_extensions =
58095811
ExtractExtensionList(defGetString(def), false);
58105812
else if (strcmp(def->defname, "fetch_size") == 0)
5811-
fpinfo->fetch_size = strtol(defGetString(def), NULL, 10);
5813+
(void) parse_int(defGetString(def), &fpinfo->fetch_size, 0, NULL);
58125814
else if (strcmp(def->defname, "async_capable") == 0)
58135815
fpinfo->async_capable = defGetBoolean(def);
58145816
}
@@ -5831,7 +5833,7 @@ apply_table_options(PgFdwRelationInfo *fpinfo)
58315833
if (strcmp(def->defname, "use_remote_estimate") == 0)
58325834
fpinfo->use_remote_estimate = defGetBoolean(def);
58335835
else if (strcmp(def->defname, "fetch_size") == 0)
5834-
fpinfo->fetch_size = strtol(defGetString(def), NULL, 10);
5836+
(void) parse_int(defGetString(def), &fpinfo->fetch_size, 0, NULL);
58355837
else if (strcmp(def->defname, "async_capable") == 0)
58365838
fpinfo->async_capable = defGetBoolean(def);
58375839
}
@@ -7341,7 +7343,7 @@ get_batch_size_option(Relation rel)
73417343

73427344
if (strcmp(def->defname, "batch_size") == 0)
73437345
{
7344-
batch_size = strtol(defGetString(def), NULL, 10);
7346+
(void) parse_int(defGetString(def), &batch_size, 0, NULL);
73457347
break;
73467348
}
73477349
}

‎contrib/postgres_fdw/sql/postgres_fdw.sql

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3339,3 +3339,19 @@ DROP TABLE join_tbl;
33393339

33403340
ALTER SERVER loopback OPTIONS (DROP async_capable);
33413341
ALTER SERVER loopback2 OPTIONS (DROP async_capable);
3342+
3343+
-- ===================================================================
3344+
-- test invalid server and foreign table options
3345+
-- ===================================================================
3346+
-- Invalid fdw_startup_cost option
3347+
CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
3348+
OPTIONS(fdw_startup_cost '100$%$#$#');
3349+
-- Invalid fdw_tuple_cost option
3350+
CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
3351+
OPTIONS(fdw_tuple_cost '100$%$#$#');
3352+
-- Invalid fetch_size option
3353+
CREATE FOREIGN TABLE inv_fsz (c1 int )
3354+
SERVER loopback OPTIONS (fetch_size '100$%$#$#');
3355+
-- Invalid batch_size option
3356+
CREATE FOREIGN TABLE inv_bsz (c1 int )
3357+
SERVER loopback OPTIONS (batch_size '100$%$#$#');

‎doc/src/sgml/postgres-fdw.sgml

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -266,11 +266,11 @@ OPTIONS (ADD password_required 'false');
266266
<term><literal>fdw_startup_cost</literal> (<type>floating point</type>)</term>
267267
<listitem>
268268
<para>
269-
This option, which can be specified for a foreign server, is a numeric
270-
value that is added to the estimated startup cost of any foreign-table
271-
scan on that server. This represents the additional overhead of
272-
establishing a connection, parsing and planning the query on the
273-
remote side, etc.
269+
This option, which can be specified for a foreign server, is a floating
270+
point value that is added to the estimated startup cost of any
271+
foreign-table scan on that server. This represents the additional
272+
overhead of establishing a connection, parsing and planning the query on
273+
the remote side, etc.
274274
The default value is <literal>100</literal>.
275275
</para>
276276
</listitem>
@@ -280,8 +280,8 @@ OPTIONS (ADD password_required 'false');
280280
<term><literal>fdw_tuple_cost</literal> (<type>floating point</type>)</term>
281281
<listitem>
282282
<para>
283-
This option, which can be specified for a foreign server, is a numeric
284-
value that is used as extra cost per-tuple for foreign-table
283+
This option, which can be specified for a foreign server, is a floating
284+
point value that is used as extra cost per-tuple for foreign-table
285285
scans on that server. This represents the additional overhead of
286286
data transfer between servers. You might increase or decrease this
287287
number to reflect higher or lower network delay to the remote server.

0 commit comments

Comments
 (0)