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.

diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index fb33d305aff3d0be30d5b1c06df544624475217c..0d4aa69d8256c657e841f6b4eaf713ede153ebec 100644 (file)
--- 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 812a610d029bbb97e66fa2d28ec548a1cfb240a4..0919c046e5ed274f6a6005b520dc6663628d8a51 100644 (file)
--- 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 300401e01f98424fadf8aaf6608791f24a600c1b..13ddb8fc250e25f485cc59a9c8f2b99c41f38a3e 100644 (file)
--- 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 d093263589cece566b8409e441a46294b09dbac0..035177492127b983b16523226bd9701c09a60519 100644 (file)
--- 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 = &paramLI->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 = &paramLI->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 cb580dc0a7269d226bc2eb93793b748d208715a6..1dc2eb0fcca3c87d1f424ef71592c5cf1045f89c 100644 (file)
--- 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 3d9e35442dabd6d5a056ac360a32886d99d9f37e..0d619504552a113f1c09df91779d70f4c65166e5 100644 (file)
--- 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 83bebde69d48fb6ee109071af06b0397972c0765..2beae5f5cab5162b2c08504ae0286fcffb214a60 100644 (file)
--- 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 c73f20b725a0ed65d5fe0c8c2a83c2bba8254755..0b82e654244c635792bec4c62e64058cbcd7c608 100644 (file)
--- 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)