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

Fix failure with initplans used conditionally during EvalPlanQual rechecks. 

The EvalPlanQual machinery assumes that any initplans (that is,
uncorrelated sub-selects) used during an EPQ recheck would have already
been evaluated during the main query; this is implicit in the fact that
execPlan pointers are not copied into the EPQ estate's es_param_exec_vals.
But it's possible for that assumption to fail, if the initplan is only
reached conditionally.  For example, a sub-select inside a CASE expression
could be reached during a recheck when it had not been previously, if the
CASE test depends on a column that was just updated.

This bug is old, appearing to date back to my rewrite of EvalPlanQual in
commit 9f2ee8f28, but was not detected until Kyle Samson reported a case.

To fix, force all not-yet-evaluated initplans used within the EPQ plan
subtree to be evaluated at the start of the recheck, before entering the
EPQ environment.  This could be inefficient, if such an initplan is
expensive and goes unused again during the recheck --- but that's piling
one layer of improbability atop another.  It doesn't seem worth adding
more complexity to prevent that, at least not in the back branches.

It was convenient to use the new-in-v11 ExecEvalParamExecParams function
to implement this, but I didn't like either its name or the specifics of
its API, so revise that.

Back-patch all the way.  Rather than rewrite the patch to avoid depending
on bms_next_member() in the oldest branches, I chose to back-patch that
function into 9.4 and 9.3.  (This isn't the first time back-patches have
needed that, and it exhausted my patience.)  I also chose to back-patch
some test cases added by commits 71404af2a and 342a1ffa2 into 9.4 and 9.3,
so that the 9.x versions of eval-plan-qual.spec are all the same.

Andrew Gierth diagnosed the problem and contributed the added test cases,
though the actual code changes are by me.

Discussion: https://postgr.es/m/A033A40A-B234-4324-BE37-272279F7B627@tripadvisor.com
This commit is contained in:
Tom Lane 2018-09-15 13:42:33 -04:00
parent 6b78231d91
commit 1f4a920b73
8 changed files with 147 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
src/backend/executor/execExprInterp.c
View File

@ -2252,33 +2252,6 @@ ExecEvalParamExec(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
*op->resnull = prm->isnull;
}
/*
* ExecEvalParamExecParams
*
* Execute the subplan stored in PARAM_EXEC initplans params, if not executed
* till now.
*/
void
ExecEvalParamExecParams(Bitmapset *params, EState *estate)
{
ParamExecData *prm;
int paramid;
paramid = -1;
while ((paramid = bms_next_member(params, paramid)) >= 0)
{
prm = &(estate->es_param_exec_vals[paramid]);
if (prm->execPlan != NULL)
{
/* Parameter not evaluated yet, so go do it */
ExecSetParamPlan(prm->execPlan, GetPerTupleExprContext(estate));
/* ExecSetParamPlan should have processed this param... */
Assert(prm->execPlan == NULL);
}
}
}
/*
* Evaluate a PARAM_EXTERN parameter.
*

40
src/backend/executor/execMain.c
View File

@ -46,6 +46,7 @@
#include "commands/matview.h"
#include "commands/trigger.h"
#include "executor/execdebug.h"
#include "executor/nodeSubplan.h"
#include "foreign/fdwapi.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
@ -1727,8 +1728,8 @@ ExecutePlan(EState *estate,
if (TupIsNull(slot))
{
/*
* If we know we won't need to back up, we can release
* resources at this point.
* If we know we won't need to back up, we can release resources
* at this point.
*/
if (!(estate->es_top_eflags & EXEC_FLAG_BACKWARD))
(void) ExecShutdownNode(planstate);
@ -1778,8 +1779,8 @@ ExecutePlan(EState *estate,
if (numberTuples && numberTuples == current_tuple_count)
{
/*
* If we know we won't need to back up, we can release
* resources at this point.
* If we know we won't need to back up, we can release resources
* at this point.
*/
if (!(estate->es_top_eflags & EXEC_FLAG_BACKWARD))
(void) ExecShutdownNode(planstate);
@ -3078,6 +3079,14 @@ EvalPlanQualBegin(EPQState *epqstate, EState *parentestate)
{
int i;
/*
* Force evaluation of any InitPlan outputs that could be needed
* by the subplan, just in case they got reset since
* EvalPlanQualStart (see comments therein).
*/
ExecSetParamPlanMulti(planstate->plan->extParam,
GetPerTupleExprContext(parentestate));
i = list_length(parentestate->es_plannedstmt->paramExecTypes);
while (--i >= 0)
@ -3170,9 +3179,32 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree)
{
int i;
/*
* Force evaluation of any InitPlan outputs that could be needed by
* the subplan. (With more complexity, maybe we could postpone this
* till the subplan actually demands them, but it doesn't seem worth
* the trouble; this is a corner case already, since usually the
* InitPlans would have been evaluated before reaching EvalPlanQual.)
*
* This will not touch output params of InitPlans that occur somewhere
* within the subplan tree, only those that are attached to the
* ModifyTable node or above it and are referenced within the subplan.
* That's OK though, because the planner would only attach such
* InitPlans to a lower-level SubqueryScan node, and EPQ execution
* will not descend into a SubqueryScan.
*
* The EState's per-output-tuple econtext is sufficiently short-lived
* for this, since it should get reset before there is any chance of
* doing EvalPlanQual again.
*/
ExecSetParamPlanMulti(planTree->extParam,
GetPerTupleExprContext(parentestate));
/* now make the internal param workspace ... */
i = list_length(parentestate->es_plannedstmt->paramExecTypes);
estate->es_param_exec_vals = (ParamExecData *)
palloc0(i * sizeof(ParamExecData));
/* ... and copy down all values, whether really needed or not */
while (--i >= 0)
{
/* copy value if any, but not execPlan link */

24
src/backend/executor/execParallel.c
View File

@ -23,7 +23,6 @@
#include "postgres.h"
#include "executor/execExpr.h"
#include "executor/execParallel.h"
#include "executor/executor.h"
#include "executor/nodeAppend.h"
@ -36,6 +35,7 @@
#include "executor/nodeIndexonlyscan.h"
#include "executor/nodeSeqscan.h"
#include "executor/nodeSort.h"
#include "executor/nodeSubplan.h"
#include "executor/tqueue.h"
#include "nodes/nodeFuncs.h"
#include "optimizer/planmain.h"
@ -581,8 +581,18 @@ ExecInitParallelPlan(PlanState *planstate, EState *estate,
char *query_string;
int query_len;
/* Force parameters we're going to pass to workers to be evaluated. */
ExecEvalParamExecParams(sendParams, estate);
/*
* Force any initplan outputs that we're going to pass to workers to be
* evaluated, if they weren't already.
*
* For simplicity, we use the EState's per-output-tuple ExprContext here.
* That risks intra-query memory leakage, since we might pass through here
* many times before that ExprContext gets reset; but ExecSetParamPlan
* doesn't normally leak any memory in the context (see its comments), so
* it doesn't seem worth complicating this function's API to pass it a
* shorter-lived ExprContext. This might need to change someday.
*/
ExecSetParamPlanMulti(sendParams, GetPerTupleExprContext(estate));
/* Allocate object for return value. */
pei = palloc0(sizeof(ParallelExecutorInfo));
@ -831,8 +841,12 @@ ExecParallelReinitialize(PlanState *planstate,
/* Old workers must already be shut down */
Assert(pei->finished);
/* Force parameters we're going to pass to workers to be evaluated. */
ExecEvalParamExecParams(sendParams, estate);
/*
* Force any initplan outputs that we're going to pass to workers to be
* evaluated, if they weren't already (see comments in
* ExecInitParallelPlan).
*/
ExecSetParamPlanMulti(sendParams, GetPerTupleExprContext(estate));
ReinitializeParallelDSM(pei->pcxt);
pei->tqueue = ExecParallelSetupTupleQueues(pei->pcxt, true);

42
src/backend/executor/nodeSubplan.c
View File

@ -1009,6 +1009,17 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
* of initplans: we don't run the subplan until/unless we need its output.
* Note that this routine MUST clear the execPlan fields of the plan's
* output parameters after evaluating them!
*
* The results of this function are stored in the EState associated with the
* ExprContext (particularly, its ecxt_param_exec_vals); any pass-by-ref
* result Datums are allocated in the EState's per-query memory. The passed
* econtext can be any ExprContext belonging to that EState; which one is
* important only to the extent that the ExprContext's per-tuple memory
* context is used to evaluate any parameters passed down to the subplan.
* (Thus in principle, the shorter-lived the ExprContext the better, since
* that data isn't needed after we return. In practice, because initplan
* parameters are never more complex than Vars, Aggrefs, etc, evaluating them
* currently never leaks any memory anyway.)
* ----------------------------------------------------------------
*/
void
@ -1195,6 +1206,37 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
estate->es_direction = dir;
}
/*
* ExecSetParamPlanMulti
*
* Apply ExecSetParamPlan to evaluate any not-yet-evaluated initplan output
* parameters whose ParamIDs are listed in "params". Any listed params that
* are not initplan outputs are ignored.
*
* As with ExecSetParamPlan, any ExprContext belonging to the current EState
* can be used, but in principle a shorter-lived ExprContext is better than a
* longer-lived one.
*/
void
ExecSetParamPlanMulti(const Bitmapset *params, ExprContext *econtext)
{
int paramid;
paramid = -1;
while ((paramid = bms_next_member(params, paramid)) >= 0)
{
ParamExecData *prm = &(econtext->ecxt_param_exec_vals[paramid]);
if (prm->execPlan != NULL)
{
/* Parameter not evaluated yet, so go do it */
ExecSetParamPlan(prm->execPlan, econtext);
/* ExecSetParamPlan should have processed this param... */
Assert(prm->execPlan == NULL);
}
}
}
/*
* Mark an initplan as needing recalculation
*/

1
src/include/executor/execExpr.h
View File

@ -697,7 +697,6 @@ extern void ExecEvalFuncExprStrictFusage(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
extern void ExecEvalParamExec(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
extern void ExecEvalParamExecParams(Bitmapset *params, EState *estate);
extern void ExecEvalParamExtern(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
extern void ExecEvalSQLValueFunction(ExprState *state, ExprEvalStep *op);

2
src/include/executor/nodeSubplan.h
View File

@ -28,4 +28,6 @@ extern void ExecReScanSetParamPlan(SubPlanState *node, PlanState *parent);
extern void ExecSetParamPlan(SubPlanState *node, ExprContext *econtext);
extern void ExecSetParamPlanMulti(const Bitmapset *params, ExprContext *econtext);
#endif /* NODESUBPLAN_H */

31
src/test/isolation/expected/eval-plan-qual.out
View File

@ -184,6 +184,37 @@ ta_id ta_value tb_row
1 newTableAValue (1,tableBValue)
step c2: COMMIT;
starting permutation: updateforcip updateforcip2 c1 c2 read_a
step updateforcip:
UPDATE table_a SET value = NULL WHERE id = 1;
step updateforcip2:
UPDATE table_a SET value = COALESCE(value, (SELECT text 'newValue')) WHERE id = 1;
<waiting ...>
step c1: COMMIT;
step updateforcip2: <... completed>
step c2: COMMIT;
step read_a: SELECT * FROM table_a ORDER BY id;
id value
1 newValue
starting permutation: updateforcip updateforcip3 c1 c2 read_a
step updateforcip:
UPDATE table_a SET value = NULL WHERE id = 1;
step updateforcip3:
WITH d(val) AS (SELECT text 'newValue' FROM generate_series(1,1))
UPDATE table_a SET value = COALESCE(value, (SELECT val FROM d)) WHERE id = 1;
<waiting ...>
step c1: COMMIT;
step updateforcip3: <... completed>
step c2: COMMIT;
step read_a: SELECT * FROM table_a ORDER BY id;
id value
1 newValue
starting permutation: wrtwcte readwcte c1 c2
step wrtwcte: UPDATE table_a SET value = 'tableAValue2' WHERE id = 1;
step readwcte:

17
src/test/isolation/specs/eval-plan-qual.spec
View File

@ -92,6 +92,13 @@ step "updateforss" {
UPDATE table_b SET value = 'newTableBValue' WHERE id = 1;
}
# these tests exercise EvalPlanQual with conditional InitPlans which
# have not been executed prior to the EPQ
step "updateforcip" {
UPDATE table_a SET value = NULL WHERE id = 1;
}
# these tests exercise mark/restore during EPQ recheck, cf bug #15032
step "selectjoinforupdate" {
@ -129,6 +136,13 @@ step "readforss" {
FROM table_a ta
WHERE ta.id = 1 FOR UPDATE OF ta;
}
step "updateforcip2" {
UPDATE table_a SET value = COALESCE(value, (SELECT text 'newValue')) WHERE id = 1;
}
step "updateforcip3" {
WITH d(val) AS (SELECT text 'newValue' FROM generate_series(1,1))
UPDATE table_a SET value = COALESCE(value, (SELECT val FROM d)) WHERE id = 1;
}
step "wrtwcte" { UPDATE table_a SET value = 'tableAValue2' WHERE id = 1; }
step "wrjt" { UPDATE jointest SET data = 42 WHERE id = 7; }
step "c2" { COMMIT; }
@ -137,6 +151,7 @@ session "s3"
setup { BEGIN ISOLATION LEVEL READ COMMITTED; }
step "read" { SELECT * FROM accounts ORDER BY accountid; }
step "read_ext" { SELECT * FROM accounts_ext ORDER BY accountid; }
step "read_a" { SELECT * FROM table_a ORDER BY id; }
# this test exercises EvalPlanQual with a CTE, cf bug #14328
step "readwcte" {
@ -171,6 +186,8 @@ permutation "wx2" "partiallock" "c2" "c1" "read"
permutation "wx2" "lockwithvalues" "c2" "c1" "read"
permutation "wx2_ext" "partiallock_ext" "c2" "c1" "read_ext"
permutation "updateforss" "readforss" "c1" "c2"
permutation "updateforcip" "updateforcip2" "c1" "c2" "read_a"
permutation "updateforcip" "updateforcip3" "c1" "c2" "read_a"
permutation "wrtwcte" "readwcte" "c1" "c2"
permutation "wrjt" "selectjoinforupdate" "c2" "c1"
permutation "wrtwcte" "multireadwcte" "c1" "c2"