Hi,
I took at this patch today. I did some minor changes, mostly:
1) change the code limiting batch_size from
if (fmstate->p_nums > 0 &&
(batch_size * fmstate->p_nums > PQ_QUERY_PARAM_MAX_LIMIT))
{
batch_size = PQ_QUERY_PARAM_MAX_LIMIT / fmstate->p_nums;
}
to
if (fmstate && fmstate->p_nums > 0)
batch_size = Min(batch_size,
PQ_QUERY_PARAM_MAX_LIMIT / fmstate->p_nums);
which I think is somewhat clearer / more common patter.
2) I've reworded the docs a bit, splitting the single para into two. I
think this makes it clearer.
Attached is a patch doing this. Please check the commit message etc.
Barring objections I'll get it committed in a couple days.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
>From 222806c4eef0fb3a021da3c1d4cc9e1e41bd9ddf Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email protected]>
Date: Sun, 30 May 2021 20:58:38 +0200
Subject: [PATCH] Make sure postgres_fdw batching does use too many parameters
The FE/BE protocol identifies parameters with an Int16 index, which
limits the maximum number of parameters per query to 65535. With
batching added to postges_fdw this limit is much easier to hit, as
the whole batch is essentially a single query, making this error much
easier to hit.
The failures are a bit unpredictable, because it also depends on the
number of columns in the query. So instead of just failing, this patch
tweaks the batch_size to not exceed the maximum number of parameters.
Reported-by: Hou Zhijie <[email protected]>
Reviewed-by: Bharath Rupireddy <[email protected]>
Discussion: https://postgr.es/m/OS0PR01MB571603973C0AC2874AD6BF2594299%40OS0PR01MB5716.jpnprd01.prod.outlook.com
---
contrib/postgres_fdw/postgres_fdw.c | 11 +++++++++--
doc/src/sgml/postgres-fdw.sgml | 11 +++++++++++
src/interfaces/libpq/fe-exec.c | 21 ++++++++++++---------
src/interfaces/libpq/libpq-fe.h | 2 ++
4 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index c48a421e88..ac86b06b8f 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1979,7 +1979,7 @@ postgresGetForeignModifyBatchSize(ResultRelInfo *resultRelInfo)
Assert(fmstate == NULL || fmstate->aux_fmstate == NULL);
/*
- * In EXPLAIN without ANALYZE, ri_fdwstate is NULL, so we have to lookup
+ * In EXPLAIN without ANALYZE, ri_FdwState is NULL, so we have to lookup
* the option directly in server/table options. Otherwise just use the
* value we determined earlier.
*/
@@ -1994,7 +1994,14 @@ postgresGetForeignModifyBatchSize(ResultRelInfo *resultRelInfo)
resultRelInfo->ri_TrigDesc->trig_insert_after_row))
return 1;
- /* Otherwise use the batch size specified for server/table. */
+ /*
+ * Otherwise use the batch size specified for server/table. The number of
+ * parameters in a batch is limited to 64k (uint16), so make sure we don't
+ * exceed this limit by using the maximum batch_size possible.
+ */
+ if (fmstate && fmstate->p_nums > 0)
+ batch_size = Min(batch_size, PQ_QUERY_PARAM_MAX_LIMIT / fmstate->p_nums);
+
return batch_size;
}
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index fb87372bde..564651dfaa 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -372,6 +372,17 @@ OPTIONS (ADD password_required 'false');
overrides an option specified for the server.
The default is <literal>1</literal>.
</para>
+
+ <para>
+ Note the actual number of rows <filename>postgres_fdw</filename> inserts at
+ once depends on the number of columns and the provided
+ <literal>batch_size</literal> value. The batch is executed as a single
+ query, and the libpq protocol (which <filename>postgres_fdw</filename>
+ uses to connect to a remote server) limits the number of parameters in a
+ single query to 64k. When the number of columns * <literal>batch_size</literal>
+ exceeds the limit, the <literal>batch_size</literal> will be adjusted to
+ avoid an error.
+ </para>
</listitem>
</varlistentry>
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 03592bdce9..832d61c544 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -1403,10 +1403,11 @@ PQsendQueryParams(PGconn *conn,
libpq_gettext("command string is a null pointer\n"));
return 0;
}
- if (nParams < 0 || nParams > 65535)
+ if (nParams < 0 || nParams > PQ_QUERY_PARAM_MAX_LIMIT)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("number of parameters must be between 0 and 65535\n"));
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("number of parameters must be between 0 and %d\n"),
+ PQ_QUERY_PARAM_MAX_LIMIT);
return 0;
}
@@ -1451,10 +1452,11 @@ PQsendPrepare(PGconn *conn,
libpq_gettext("command string is a null pointer\n"));
return 0;
}
- if (nParams < 0 || nParams > 65535)
+ if (nParams < 0 || nParams > PQ_QUERY_PARAM_MAX_LIMIT)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("number of parameters must be between 0 and 65535\n"));
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("number of parameters must be between 0 and %d\n"),
+ PQ_QUERY_PARAM_MAX_LIMIT);
return 0;
}
@@ -1548,10 +1550,11 @@ PQsendQueryPrepared(PGconn *conn,
libpq_gettext("statement name is a null pointer\n"));
return 0;
}
- if (nParams < 0 || nParams > 65535)
+ if (nParams < 0 || nParams > PQ_QUERY_PARAM_MAX_LIMIT)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("number of parameters must be between 0 and 65535\n"));
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("number of parameters must be between 0 and %d\n"),
+ PQ_QUERY_PARAM_MAX_LIMIT);
return 0;
}
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 227adde5a5..113ab52ada 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -429,6 +429,8 @@ extern PGresult *PQexecPrepared(PGconn *conn,
int resultFormat);
/* Interface for multiple-result or asynchronous queries */
+#define PQ_QUERY_PARAM_MAX_LIMIT 65535
+
extern int PQsendQuery(PGconn *conn, const char *query);
extern int PQsendQueryParams(PGconn *conn,
const char *command,
--
2.31.1
