On Sat, Oct 24, 2015 at 6:31 PM, Noah Misch <n...@leadboat.com> wrote:
> On Sat, Oct 24, 2015 at 07:49:07AM -0400, Robert Haas wrote:
>> On Fri, Oct 23, 2015 at 9:38 PM, Noah Misch <n...@leadboat.com> wrote:
>> > Since that specification permits ParamListInfo consumers to ignore
>> > paramMask,
>> > the plpgsql_param_fetch() change from copy-paramlistinfo-fixes.patch is
>> > still
>> > formally required.
>>
>> So why am I not just doing that, then? Seems a lot more surgical.
>
> do $$
> declare
> param_unused text := repeat('a', 100 * 1024 * 1024);
> param_used oid := 403;
> begin
> perform count(*) from pg_am where oid = param_used;
> end
> $$;
>
> I expect that if you were to inspect the EstimateParamListSpace() return
> values when executing that, you would find that it serializes the irrelevant
> 100 MiB datum. No possible logic in plpgsql_param_fetch() could stop that
> from happening, because copyParamList() and SerializeParamList() call the
> paramFetch hook only for dynamic parameters. Cursors faced the same problem,
> which is the raison d'être for setup_unshared_param_list().
Well, OK. That's not strictly a correctness issue, but here's an
updated patch along the lines you suggested.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From 50895be5cdbb0fda41535be23700e5112585e1e3 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Thu, 22 Oct 2015 23:56:51 -0400
Subject: [PATCH 6/6] 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 performance, but it makes
the performance of the parallel executor code comparable to what we
do for cases involving cursors.
---
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 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..0351774 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 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..0b82e65 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.)
@@ -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.3.8 (Apple Git-58)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
