From 1efc7e538204646fec0f790b683012ed9bd8a99f Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Mon, 2 Nov 2015 18:11:29 -0500
Subject: [PATCH] Fix problems with ParamListInfo serialization mechanism.
Commit d1b7c1ffe72e86932b5395f29e006c3f503bc53d introduced a mechanism
for serializing a ParamListInfo structure to be passed to a parallel
worker. However, this mechanism failed to handle external expanded
values, as pointed out by Noah Misch. Repair.
Moreover, plpgsql_param_fetch requires adjustment because the
serialization mechanism needs it to skip evaluating unused parameters
just as we would do when it is called from copyParamList, but params
== estate->paramLI in that case. To fix, make the bms_is_member test
in that function unconditional.
Finally, have setup_param_list set a new ParamListInfo field,
paramMask, to the parameters actually used in the expression, so that
we don't try to fetch those that are not needed when serializing a
parameter list. This isn't necessary for correctness, but it makes
the performance of the parallel executor code comparable to what we
do for cases involving cursors.
Design suggestions and extensive review by Noah Misch. Patch by me.
---
src/backend/commands/prepare.c | 1 +
src/backend/executor/functions.c | 1 +
src/backend/executor/spi.c | 1 +
src/backend/nodes/params.c | 54 +++++++++++++++++++++++++-------
src/backend/tcop/postgres.c | 1 +
src/backend/utils/adt/datum.c | 16 ++++++++++
src/include/nodes/params.h | 4 ++-
src/pl/plpgsql/src/pl_exec.c | 40 +++++++++++++----------
8 files changed, 89 insertions(+), 29 deletions(-)
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index fb33d305aff..0d4aa69d825 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -392,6 +392,7 @@ EvaluateParams(PreparedStatement *pstmt, List *params,
paramLI->parserSetup = NULL;
paramLI->parserSetupArg = NULL;
paramLI->numParams = num_params;
+ paramLI->paramMask = NULL;
i = 0;
foreach(l, exprstates)
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 812a610d029..0919c046e5e 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -910,6 +910,7 @@ postquel_sub_params(SQLFunctionCachePtr fcache,
paramLI->parserSetup = NULL;
paramLI->parserSetupArg = NULL;
paramLI->numParams = nargs;
+ paramLI->paramMask = NULL;
fcache->paramLI = paramLI;
}
else
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 300401e01f9..13ddb8fc250 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2330,6 +2330,7 @@ _SPI_convert_params(int nargs, Oid *argtypes,
paramLI->parserSetup = NULL;
paramLI->parserSetupArg = NULL;
paramLI->numParams = nargs;
+ paramLI->paramMask = NULL;
for (i = 0; i < nargs; i++)
{
diff --git a/src/backend/nodes/params.c b/src/backend/nodes/params.c
index d093263589c..03517749212 100644
--- a/src/backend/nodes/params.c
+++ b/src/backend/nodes/params.c
@@ -15,6 +15,7 @@
#include "postgres.h"
+#include "nodes/bitmapset.h"
#include "nodes/params.h"
#include "storage/shmem.h"
#include "utils/datum.h"
@@ -50,6 +51,7 @@ copyParamList(ParamListInfo from)
retval->parserSetup = NULL;
retval->parserSetupArg = NULL;
retval->numParams = from->numParams;
+ retval->paramMask = NULL;
for (i = 0; i < from->numParams; i++)
{
@@ -58,6 +60,17 @@ copyParamList(ParamListInfo from)
int16 typLen;
bool typByVal;
+ /* Ignore parameters we don't need, to save cycles and space. */
+ if (retval->paramMask != NULL &&
+ !bms_is_member(i, retval->paramMask))
+ {
+ nprm->value = (Datum) 0;
+ nprm->isnull = true;
+ nprm->pflags = 0;
+ nprm->ptype = InvalidOid;
+ continue;
+ }
+
/* give hook a chance in case parameter is dynamic */
if (!OidIsValid(oprm->ptype) && from->paramFetch != NULL)
(*from->paramFetch) (from, i + 1);
@@ -90,19 +103,28 @@ EstimateParamListSpace(ParamListInfo paramLI)
for (i = 0; i < paramLI->numParams; i++)
{
ParamExternData *prm = ¶mLI->params[i];
+ Oid typeOid;
int16 typLen;
bool typByVal;
- /* give hook a chance in case parameter is dynamic */
- if (!OidIsValid(prm->ptype) && paramLI->paramFetch != NULL)
- (*paramLI->paramFetch) (paramLI, i + 1);
+ /* Ignore parameters we don't need, to save cycles and space. */
+ if (paramLI->paramMask != NULL &&
+ !bms_is_member(i, paramLI->paramMask))
+ typeOid = InvalidOid;
+ else
+ {
+ /* give hook a chance in case parameter is dynamic */
+ if (!OidIsValid(prm->ptype) && paramLI->paramFetch != NULL)
+ (*paramLI->paramFetch) (paramLI, i + 1);
+ typeOid = prm->ptype;
+ }
sz = add_size(sz, sizeof(Oid)); /* space for type OID */
sz = add_size(sz, sizeof(uint16)); /* space for pflags */
/* space for datum/isnull */
- if (OidIsValid(prm->ptype))
- get_typlenbyval(prm->ptype, &typLen, &typByVal);
+ if (OidIsValid(typeOid))
+ get_typlenbyval(typeOid, &typLen, &typByVal);
else
{
/* If no type OID, assume by-value, like copyParamList does. */
@@ -150,15 +172,24 @@ SerializeParamList(ParamListInfo paramLI, char **start_address)
for (i = 0; i < nparams; i++)
{
ParamExternData *prm = ¶mLI->params[i];
+ Oid typeOid;
int16 typLen;
bool typByVal;
- /* give hook a chance in case parameter is dynamic */
- if (!OidIsValid(prm->ptype) && paramLI->paramFetch != NULL)
- (*paramLI->paramFetch) (paramLI, i + 1);
+ /* Ignore parameters we don't need, to save cycles and space. */
+ if (paramLI->paramMask != NULL &&
+ !bms_is_member(i, paramLI->paramMask))
+ typeOid = InvalidOid;
+ else
+ {
+ /* give hook a chance in case parameter is dynamic */
+ if (!OidIsValid(prm->ptype) && paramLI->paramFetch != NULL)
+ (*paramLI->paramFetch) (paramLI, i + 1);
+ typeOid = prm->ptype;
+ }
/* Write type OID. */
- memcpy(*start_address, &prm->ptype, sizeof(Oid));
+ memcpy(*start_address, &typeOid, sizeof(Oid));
*start_address += sizeof(Oid);
/* Write flags. */
@@ -166,8 +197,8 @@ SerializeParamList(ParamListInfo paramLI, char **start_address)
*start_address += sizeof(uint16);
/* Write datum/isnull. */
- if (OidIsValid(prm->ptype))
- get_typlenbyval(prm->ptype, &typLen, &typByVal);
+ if (OidIsValid(typeOid))
+ get_typlenbyval(typeOid, &typLen, &typByVal);
else
{
/* If no type OID, assume by-value, like copyParamList does. */
@@ -209,6 +240,7 @@ RestoreParamList(char **start_address)
paramLI->parserSetup = NULL;
paramLI->parserSetupArg = NULL;
paramLI->numParams = nparams;
+ paramLI->paramMask = NULL;
for (i = 0; i < nparams; i++)
{
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index cb580dc0a72..1dc2eb0fcca 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1629,6 +1629,7 @@ exec_bind_message(StringInfo input_message)
params->parserSetup = NULL;
params->parserSetupArg = NULL;
params->numParams = numParams;
+ params->paramMask = NULL;
for (paramno = 0; paramno < numParams; paramno++)
{
diff --git a/src/backend/utils/adt/datum.c b/src/backend/utils/adt/datum.c
index 3d9e35442da..0d619504552 100644
--- a/src/backend/utils/adt/datum.c
+++ b/src/backend/utils/adt/datum.c
@@ -264,6 +264,11 @@ datumEstimateSpace(Datum value, bool isnull, bool typByVal, int typLen)
/* no need to use add_size, can't overflow */
if (typByVal)
sz += sizeof(Datum);
+ else if (VARATT_IS_EXTERNAL_EXPANDED(value))
+ {
+ ExpandedObjectHeader *eoh = DatumGetEOHP(value);
+ sz += EOH_get_flat_size(eoh);
+ }
else
sz += datumGetSize(value, typByVal, typLen);
}
@@ -292,6 +297,7 @@ void
datumSerialize(Datum value, bool isnull, bool typByVal, int typLen,
char **start_address)
{
+ ExpandedObjectHeader *eoh = NULL;
int header;
/* Write header word. */
@@ -299,6 +305,11 @@ datumSerialize(Datum value, bool isnull, bool typByVal, int typLen,
header = -2;
else if (typByVal)
header = -1;
+ else if (VARATT_IS_EXTERNAL_EXPANDED(value))
+ {
+ eoh = DatumGetEOHP(value);
+ header = EOH_get_flat_size(eoh);
+ }
else
header = datumGetSize(value, typByVal, typLen);
memcpy(*start_address, &header, sizeof(int));
@@ -312,6 +323,11 @@ datumSerialize(Datum value, bool isnull, bool typByVal, int typLen,
memcpy(*start_address, &value, sizeof(Datum));
*start_address += sizeof(Datum);
}
+ else if (eoh)
+ {
+ EOH_flatten_into(eoh, (void *) *start_address, header);
+ *start_address += header;
+ }
else
{
memcpy(*start_address, DatumGetPointer(value), header);
diff --git a/src/include/nodes/params.h b/src/include/nodes/params.h
index 83bebde69d4..2beae5f5cab 100644
--- a/src/include/nodes/params.h
+++ b/src/include/nodes/params.h
@@ -14,7 +14,8 @@
#ifndef PARAMS_H
#define PARAMS_H
-/* To avoid including a pile of parser headers, reference ParseState thus: */
+/* Forward declarations, to avoid including other headers */
+struct Bitmapset;
struct ParseState;
@@ -71,6 +72,7 @@ typedef struct ParamListInfoData
ParserSetupHook parserSetup; /* parser setup hook */
void *parserSetupArg;
int numParams; /* number of ParamExternDatas following */
+ struct Bitmapset *paramMask; /* if non-NULL, can ignore omitted params */
ParamExternData params[FLEXIBLE_ARRAY_MEMBER];
} ParamListInfoData;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index c73f20b725a..0b82e654244 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3287,6 +3287,7 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
estate->paramLI->parserSetup = (ParserSetupHook) plpgsql_parser_setup;
estate->paramLI->parserSetupArg = NULL; /* filled during use */
estate->paramLI->numParams = estate->ndatums;
+ estate->paramLI->paramMask = NULL;
estate->params_dirty = false;
/* set up for use of appropriate simple-expression EState and cast hash */
@@ -5558,6 +5559,12 @@ setup_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
*/
paramLI->parserSetupArg = (void *) expr;
+ /*
+ * Allow parameters that aren't needed by this expression to be
+ * ignored.
+ */
+ paramLI->paramMask = expr->paramnos;
+
/*
* Also make sure this is set before parser hooks need it. There is
* no need to save and restore, since the value is always correct once
@@ -5592,6 +5599,9 @@ setup_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
* shared param list, where it could get passed to some less-trusted function.
*
* Caller should pfree the result after use, if it's not NULL.
+ *
+ * XXX. Could we use ParamListInfo's new paramMask to avoid creating unshared
+ * parameter lists?
*/
static ParamListInfo
setup_unshared_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
@@ -5623,6 +5633,7 @@ setup_unshared_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
paramLI->parserSetup = (ParserSetupHook) plpgsql_parser_setup;
paramLI->parserSetupArg = (void *) expr;
paramLI->numParams = estate->ndatums;
+ paramLI->paramMask = NULL;
/*
* Instantiate values for "safe" parameters of the expression. We
@@ -5696,25 +5707,20 @@ plpgsql_param_fetch(ParamListInfo params, int paramid)
/* now we can access the target datum */
datum = estate->datums[dno];
- /* need to behave slightly differently for shared and unshared arrays */
- if (params != estate->paramLI)
- {
- /*
- * We're being called, presumably from copyParamList(), for cursor
- * parameters. Since copyParamList() will try to materialize every
- * single parameter slot, it's important to do nothing when asked for
- * a datum that's not supposed to be used by this SQL expression.
- * Otherwise we risk failures in exec_eval_datum(), not to mention
- * possibly copying a lot more data than the cursor actually uses.
- */
- if (!bms_is_member(dno, expr->paramnos))
- return;
- }
- else
+ /*
+ * Since copyParamList() or SerializeParamList() will try to materialize
+ * every single parameter slot, it's important to do nothing when asked
+ * for a datum that's not supposed to be used by this SQL expression.
+ * Otherwise we risk failures in exec_eval_datum(), or copying a lot more
+ * data than necessary.
+ */
+ if (!bms_is_member(dno, expr->paramnos))
+ return;
+
+ if (params == estate->paramLI)
{
/*
- * Normal evaluation cases. We don't need to sanity-check dno, but we
- * do need to mark the shared params array dirty if we're about to
+ * We need to mark the shared params array dirty if we're about to
* evaluate a resettable datum.
*/
switch (datum->dtype)
--
GitLab