From a102f98e26bc7d21eb5246fd345f8e3ab31be109 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 3 Jun 2016 15:14:35 -0400 Subject: [PATCH] Mark read/write expanded values as read-only in ExecProject(). If a plan node output expression returns an "expanded" datum, and that output column is referenced in more than one place in upper-level plan nodes, we need to ensure that what is returned is a read-only reference not a read/write reference. Otherwise one of the referencing sites could scribble on or even delete the expanded datum before we have evaluated the others. Commit 1dc5ebc9077ab742, which introduced this feature, supposed that it'd be sufficient to make SubqueryScan nodes force their output columns to read-only state. The folly of that was revealed by bug #14174 from Andrew Gierth, and really should have been immediately obvious considering that the planner will happily optimize SubqueryScan nodes out of the plan without any regard for this issue. The safest fix seems to be to make ExecProject() force its results into read-only state; that will cover every case where a plan node returns expression results. Actually we can delegate this to ExecTargetList() since we can recursively assume that plain Vars will not reference read-write datums. That should keep the extra overhead down to something minimal. We no longer need ExecMakeSlotContentsReadOnly(), which was introduced only in support of the idea that just a few plan node types would need to do this. In the future it would be nice to have the planner account for this problem and inject force-to-read-only expression evaluation nodes into only the places where there's a risk of multiple evaluation. That's not a suitable solution for 9.5 or even 9.6 at this point, though. Report: <20160603124628.9932.41279@wrigleys.postgresql.org> --- src/backend/executor/execQual.c | 19 ++++++++++ src/backend/executor/execTuples.c | 47 ------------------------- src/backend/executor/nodeSubqueryscan.c | 8 ----- src/include/executor/tuptable.h | 1 - src/test/regress/expected/plpgsql.out | 40 ++++++++++++++++++++- src/test/regress/sql/plpgsql.sql | 32 ++++++++++++++++- 6 files changed, 89 insertions(+), 58 deletions(-) diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c index 16bc8fa5f6..f2e8ee2f77 100644 --- a/src/backend/executor/execQual.c +++ b/src/backend/executor/execQual.c @@ -5333,15 +5333,24 @@ ExecCleanTargetListLength(List *targetlist) * of *isDone = ExprMultipleResult signifies a set element, and a return * of *isDone = ExprEndResult signifies end of the set of tuple. * We assume that *isDone has been initialized to ExprSingleResult by caller. + * + * Since fields of the result tuple might be multiply referenced in higher + * plan nodes, we have to force any read/write expanded values to read-only + * status. It's a bit annoying to have to do that for every projected + * expression; in the future, consider teaching the planner to detect + * actually-multiply-referenced Vars and insert an expression node that + * would do that only where really required. */ static bool ExecTargetList(List *targetlist, + TupleDesc tupdesc, ExprContext *econtext, Datum *values, bool *isnull, ExprDoneCond *itemIsDone, ExprDoneCond *isDone) { + Form_pg_attribute *att = tupdesc->attrs; MemoryContext oldContext; ListCell *tl; bool haveDoneSets; @@ -5367,6 +5376,10 @@ ExecTargetList(List *targetlist, &isnull[resind], &itemIsDone[resind]); + values[resind] = MakeExpandedObjectReadOnly(values[resind], + isnull[resind], + att[resind]->attlen); + if (itemIsDone[resind] != ExprSingleResult) { /* We have a set-valued expression in the tlist */ @@ -5420,6 +5433,10 @@ ExecTargetList(List *targetlist, &isnull[resind], &itemIsDone[resind]); + values[resind] = MakeExpandedObjectReadOnly(values[resind], + isnull[resind], + att[resind]->attlen); + if (itemIsDone[resind] == ExprEndResult) { /* @@ -5453,6 +5470,7 @@ ExecTargetList(List *targetlist, econtext, &isnull[resind], &itemIsDone[resind]); + /* no need for MakeExpandedObjectReadOnly */ } } @@ -5578,6 +5596,7 @@ ExecProject(ProjectionInfo *projInfo, ExprDoneCond *isDone) if (projInfo->pi_targetlist) { if (!ExecTargetList(projInfo->pi_targetlist, + slot->tts_tupleDescriptor, econtext, slot->tts_values, slot->tts_isnull, diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c index 3b13e5be7e..6aab933fbc 100644 --- a/src/backend/executor/execTuples.c +++ b/src/backend/executor/execTuples.c @@ -88,7 +88,6 @@ #include "nodes/nodeFuncs.h" #include "storage/bufmgr.h" #include "utils/builtins.h" -#include "utils/expandeddatum.h" #include "utils/lsyscache.h" #include "utils/typcache.h" @@ -813,52 +812,6 @@ ExecCopySlot(TupleTableSlot *dstslot, TupleTableSlot *srcslot) return ExecStoreTuple(newTuple, dstslot, InvalidBuffer, true); } -/* -------------------------------- - * ExecMakeSlotContentsReadOnly - * Mark any R/W expanded datums in the slot as read-only. - * - * This is needed when a slot that might contain R/W datum references is to be - * used as input for general expression evaluation. Since the expression(s) - * might contain more than one Var referencing the same R/W datum, we could - * get wrong answers if functions acting on those Vars thought they could - * modify the expanded value in-place. - * - * For notational reasons, we return the same slot passed in. - * -------------------------------- - */ -TupleTableSlot * -ExecMakeSlotContentsReadOnly(TupleTableSlot *slot) -{ - /* - * sanity checks - */ - Assert(slot != NULL); - Assert(slot->tts_tupleDescriptor != NULL); - Assert(!slot->tts_isempty); - - /* - * If the slot contains a physical tuple, it can't contain any expanded - * datums, because we flatten those when making a physical tuple. This - * might change later; but for now, we need do nothing unless the slot is - * virtual. - */ - if (slot->tts_tuple == NULL) - { - Form_pg_attribute *att = slot->tts_tupleDescriptor->attrs; - int attnum; - - for (attnum = 0; attnum < slot->tts_nvalid; attnum++) - { - slot->tts_values[attnum] = - MakeExpandedObjectReadOnly(slot->tts_values[attnum], - slot->tts_isnull[attnum], - att[attnum]->attlen); - } - } - - return slot; -} - /* ---------------------------------------------------------------- * convenience initialization routines diff --git a/src/backend/executor/nodeSubqueryscan.c b/src/backend/executor/nodeSubqueryscan.c index e5d1e540c4..3f66e243d2 100644 --- a/src/backend/executor/nodeSubqueryscan.c +++ b/src/backend/executor/nodeSubqueryscan.c @@ -56,15 +56,7 @@ SubqueryNext(SubqueryScanState *node) * We just return the subplan's result slot, rather than expending extra * cycles for ExecCopySlot(). (Our own ScanTupleSlot is used only for * EvalPlanQual rechecks.) - * - * We do need to mark the slot contents read-only to prevent interference - * between different functions reading the same datum from the slot. It's - * a bit hokey to do this to the subplan's slot, but should be safe - * enough. */ - if (!TupIsNull(slot)) - slot = ExecMakeSlotContentsReadOnly(slot); - return slot; } diff --git a/src/include/executor/tuptable.h b/src/include/executor/tuptable.h index 00686b0441..48f84bfe20 100644 --- a/src/include/executor/tuptable.h +++ b/src/include/executor/tuptable.h @@ -163,7 +163,6 @@ extern Datum ExecFetchSlotTupleDatum(TupleTableSlot *slot); extern HeapTuple ExecMaterializeSlot(TupleTableSlot *slot); extern TupleTableSlot *ExecCopySlot(TupleTableSlot *dstslot, TupleTableSlot *srcslot); -extern TupleTableSlot *ExecMakeSlotContentsReadOnly(TupleTableSlot *slot); /* in access/common/heaptuple.c */ extern Datum slot_getattr(TupleTableSlot *slot, int attnum, bool *isnull); diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out index 7fd85ba4ce..d0e0556224 100644 --- a/src/test/regress/expected/plpgsql.out +++ b/src/test/regress/expected/plpgsql.out @@ -5304,7 +5304,45 @@ ERROR: value for domain orderedarray violates check constraint "sorted" CONTEXT: PL/pgSQL function testoa(integer,integer,integer) line 5 at assignment drop function arrayassign1(); drop function testoa(x1 int, x2 int, x3 int); --- access to call stack +-- +-- Test handling of expanded arrays +-- +create function returns_rw_array(int) returns int[] +language plpgsql as $$ + declare r int[]; + begin r := array[$1, $1]; return r; end; +$$ stable; +create function consumes_rw_array(int[]) returns int +language plpgsql as $$ + begin return $1[1]; end; +$$ stable; +-- bug #14174 +explain (verbose, costs off) +select i, a from + (select returns_rw_array(1) as a offset 0) ss, + lateral consumes_rw_array(a) i; + QUERY PLAN +----------------------------------------------------------------- + Nested Loop + Output: i.i, (returns_rw_array(1)) + -> Result + Output: returns_rw_array(1) + -> Function Scan on public.consumes_rw_array i + Output: i.i + Function Call: consumes_rw_array((returns_rw_array(1))) +(7 rows) + +select i, a from + (select returns_rw_array(1) as a offset 0) ss, + lateral consumes_rw_array(a) i; + i | a +---+------- + 1 | {1,1} +(1 row) + +-- +-- Test access to call stack +-- create function inner_func(int) returns int as $$ declare _context text; diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql index 879c6f2f3c..df436ee4a2 100644 --- a/src/test/regress/sql/plpgsql.sql +++ b/src/test/regress/sql/plpgsql.sql @@ -4199,7 +4199,37 @@ select testoa(1,2,1); -- fail at update drop function arrayassign1(); drop function testoa(x1 int, x2 int, x3 int); --- access to call stack + +-- +-- Test handling of expanded arrays +-- + +create function returns_rw_array(int) returns int[] +language plpgsql as $$ + declare r int[]; + begin r := array[$1, $1]; return r; end; +$$ stable; + +create function consumes_rw_array(int[]) returns int +language plpgsql as $$ + begin return $1[1]; end; +$$ stable; + +-- bug #14174 +explain (verbose, costs off) +select i, a from + (select returns_rw_array(1) as a offset 0) ss, + lateral consumes_rw_array(a) i; + +select i, a from + (select returns_rw_array(1) as a offset 0) ss, + lateral consumes_rw_array(a) i; + + +-- +-- Test access to call stack +-- + create function inner_func(int) returns int as $$ declare _context text;