From 1efc7e538204646fec0f790b683012ed9bd8a99f Mon Sep 17 00:00:00 2001 From: Robert Haas 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 fb33d305af..0d4aa69d82 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 812a610d02..0919c046e5 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 300401e01f..13ddb8fc25 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 d093263589..0351774921 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 cb580dc0a7..1dc2eb0fcc 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 3d9e35442d..0d61950455 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 83bebde69d..2beae5f5ca 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 c73f20b725..0b82e65424 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) -- 2.40.0