rdelaage
/
postgresql
mirror of https://git.postgresql.org/git/postgresql.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix problems with ParamListInfo serialization mechanism. 

Commit d1b7c1ffe7 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.
This commit is contained in:
Robert Haas 2015-11-02 18:11:29 -05:00
parent bf25fb2f93
commit 1efc7e5382
8 changed files with 89 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
src/backend/commands/prepare.c
View File

@ -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)

1
src/backend/executor/functions.c
View File

@ -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

1
src/backend/executor/spi.c
View File

@ -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++)
{

54
src/backend/nodes/params.c
View File

@ -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++)
{

1
src/backend/tcop/postgres.c
View File

@ -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++)
{

16
src/backend/utils/adt/datum.c
View File

@ -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);

4
src/include/nodes/params.h
View File

@ -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;

40
src/pl/plpgsql/src/pl_exec.c
View File

@ -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)
/*
* 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)
{
/*
* 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
{
/*
* 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)