On Thu, Oct 15, 2015 at 8:23 PM, Noah Misch <n...@leadboat.com> wrote:
> Agreed. More specifically, I had in mind for copyParamList() to check the
> mask while e.g. ExecEvalParamExtern() would either check nothing or merely
> assert that any mask included the requested parameter. It would be tricky to
> verify that as safe, so ...
>
>> Would it work to define this as "if non-NULL,
>> params lacking a 1-bit may be safely ignored"? Or some other tweak
>> that basically says that you don't need to care about this, but you
>> can if you want to.
>
> ... this is a better specification.
Here's an attempt to implement that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
commit 1ffc4a3a1bac686b46d47dfa40bd0eb3cb8b0be4
Author: Robert Haas <rhaas@postgresql.org>
Date: Thu Oct 22 23:56:51 2015 -0400
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. 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,
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.
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index fb33d30..0d4aa69 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 812a610..0919c04 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 300401e..13ddb8f 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 d093263..9f5fd3a 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 = bms_copy(from->paramMask);
for (i = 0; i < from->numParams; i++)
{
@@ -58,6 +60,20 @@ copyParamList(ParamListInfo from)
int16 typLen;
bool typByVal;
+ /*
+ * Ignore parameters we don't need, to save cycles and space, and
+ * in case the fetch hook might fail.
+ */
+ 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 +106,31 @@ 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, and
+ * in case the fetch hook might fail.
+ */
+ 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 +178,27 @@ 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, and
+ * in case the fetch hook might fail.
+ */
+ 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 +206,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 +249,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 d30fe35..f11a715 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 3d9e354..0d61950 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 83bebde..2beae5f 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 c73f20b..dc8e746 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 */
@@ -5559,6 +5560,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
* set. (Should be set already, but let's be sure.)
@@ -5623,6 +5630,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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
