From e617f0d7e4084f85104361568ef5f865ebfa0005 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 9 Feb 2011 23:27:07 -0500 Subject: [PATCH] Fix improper matching of resjunk column names for FOR UPDATE in subselect. Flattening of subquery range tables during setrefs.c could lead to the rangetable indexes in PlanRowMark nodes not matching up with the column names previously assigned to the corresponding resjunk ctid (resp. tableoid or wholerow) columns. Typical symptom would be either a "cannot extract system attribute from virtual tuple" error or an Assert failure. This wasn't a problem before 9.0 because we didn't support FOR UPDATE below the top query level, and so the final flattening could never renumber an RTE that was relevant to FOR UPDATE. Fix by using a plan-tree-wide unique number for each PlanRowMark to label the associated resjunk columns, so that the number need not change during flattening. Per report from David Johnston (though I'm darned if I can see how this got past initial testing of the relevant code). Back-patch to 9.0. --- src/backend/executor/execMain.c | 13 ++++++++++--- src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/outfuncs.c | 2 ++ src/backend/optimizer/plan/planner.c | 3 +++ src/backend/optimizer/plan/setrefs.c | 2 +- src/backend/optimizer/prep/preptlist.c | 6 +++--- src/backend/optimizer/prep/prepunion.c | 1 + src/include/nodes/execnodes.h | 1 + src/include/nodes/plannodes.h | 7 ++++++- src/include/nodes/relation.h | 2 ++ 10 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 600f7e0334..7df7f5cdf0 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -742,6 +742,7 @@ InitPlan(QueryDesc *queryDesc, int eflags) erm->relation = relation; erm->rti = rc->rti; erm->prti = rc->prti; + erm->rowmarkId = rc->rowmarkId; erm->markType = rc->markType; erm->noWait = rc->noWait; ItemPointerSetInvalid(&(erm->curCtid)); @@ -1425,23 +1426,29 @@ ExecBuildAuxRowMark(ExecRowMark *erm, List *targetlist) /* if child rel, need tableoid */ if (erm->rti != erm->prti) { - snprintf(resname, sizeof(resname), "tableoid%u", erm->prti); + snprintf(resname, sizeof(resname), "tableoid%u", erm->rowmarkId); aerm->toidAttNo = ExecFindJunkAttributeInTlist(targetlist, resname); + if (!AttributeNumberIsValid(aerm->toidAttNo)) + elog(ERROR, "could not find junk %s column", resname); } /* always need ctid for real relations */ - snprintf(resname, sizeof(resname), "ctid%u", erm->prti); + snprintf(resname, sizeof(resname), "ctid%u", erm->rowmarkId); aerm->ctidAttNo = ExecFindJunkAttributeInTlist(targetlist, resname); + if (!AttributeNumberIsValid(aerm->ctidAttNo)) + elog(ERROR, "could not find junk %s column", resname); } else { Assert(erm->markType == ROW_MARK_COPY); - snprintf(resname, sizeof(resname), "wholerow%u", erm->prti); + snprintf(resname, sizeof(resname), "wholerow%u", erm->rowmarkId); aerm->wholeAttNo = ExecFindJunkAttributeInTlist(targetlist, resname); + if (!AttributeNumberIsValid(aerm->wholeAttNo)) + elog(ERROR, "could not find junk %s column", resname); } return aerm; diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 3d898326d7..83630924f6 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -906,6 +906,7 @@ _copyPlanRowMark(PlanRowMark *from) COPY_SCALAR_FIELD(rti); COPY_SCALAR_FIELD(prti); + COPY_SCALAR_FIELD(rowmarkId); COPY_SCALAR_FIELD(markType); COPY_SCALAR_FIELD(noWait); COPY_SCALAR_FIELD(isParent); diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index 3b3e5448fd..5c943bc254 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -808,6 +808,7 @@ _outPlanRowMark(StringInfo str, PlanRowMark *node) WRITE_UINT_FIELD(rti); WRITE_UINT_FIELD(prti); + WRITE_UINT_FIELD(rowmarkId); WRITE_ENUM_FIELD(markType, RowMarkType); WRITE_BOOL_FIELD(noWait); WRITE_BOOL_FIELD(isParent); @@ -1609,6 +1610,7 @@ _outPlannerGlobal(StringInfo str, PlannerGlobal *node) WRITE_NODE_FIELD(relationOids); WRITE_NODE_FIELD(invalItems); WRITE_UINT_FIELD(lastPHId); + WRITE_UINT_FIELD(lastRowMarkId); WRITE_BOOL_FIELD(transientPlan); } diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index b52eebe2aa..01b90383cf 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -166,6 +166,7 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams) glob->relationOids = NIL; glob->invalItems = NIL; glob->lastPHId = 0; + glob->lastRowMarkId = 0; glob->transientPlan = false; /* Determine what fraction of the plan is likely to be scanned */ @@ -1885,6 +1886,7 @@ preprocess_rowmarks(PlannerInfo *root) newrc = makeNode(PlanRowMark); newrc->rti = newrc->prti = rc->rti; + newrc->rowmarkId = ++(root->glob->lastRowMarkId); if (rc->forUpdate) newrc->markType = ROW_MARK_EXCLUSIVE; else @@ -1910,6 +1912,7 @@ preprocess_rowmarks(PlannerInfo *root) newrc = makeNode(PlanRowMark); newrc->rti = newrc->prti = i; + newrc->rowmarkId = ++(root->glob->lastRowMarkId); /* real tables support REFERENCE, anything else needs COPY */ if (rte->rtekind == RTE_RELATION) newrc->markType = ROW_MARK_REFERENCE; diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 867238ecc8..d004f6cf12 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -252,7 +252,7 @@ set_plan_references(PlannerGlobal *glob, Plan *plan, newrc = (PlanRowMark *) palloc(sizeof(PlanRowMark)); memcpy(newrc, rc, sizeof(PlanRowMark)); - /* adjust indexes */ + /* adjust indexes ... but *not* the rowmarkId */ newrc->rti += rtoffset; newrc->prti += rtoffset; diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c index 34b38eb329..63447aa536 100644 --- a/src/backend/optimizer/prep/preptlist.c +++ b/src/backend/optimizer/prep/preptlist.c @@ -102,7 +102,7 @@ preprocess_targetlist(PlannerInfo *root, List *tlist) -1, InvalidOid, 0); - snprintf(resname, sizeof(resname), "ctid%u", rc->rti); + snprintf(resname, sizeof(resname), "ctid%u", rc->rowmarkId); tle = makeTargetEntry((Expr *) var, list_length(tlist) + 1, pstrdup(resname), @@ -118,7 +118,7 @@ preprocess_targetlist(PlannerInfo *root, List *tlist) -1, InvalidOid, 0); - snprintf(resname, sizeof(resname), "tableoid%u", rc->rti); + snprintf(resname, sizeof(resname), "tableoid%u", rc->rowmarkId); tle = makeTargetEntry((Expr *) var, list_length(tlist) + 1, pstrdup(resname), @@ -132,7 +132,7 @@ preprocess_targetlist(PlannerInfo *root, List *tlist) var = makeWholeRowVar(rt_fetch(rc->rti, range_table), rc->rti, 0); - snprintf(resname, sizeof(resname), "wholerow%u", rc->rti); + snprintf(resname, sizeof(resname), "wholerow%u", rc->rowmarkId); tle = makeTargetEntry((Expr *) var, list_length(tlist) + 1, pstrdup(resname), diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c index f62af6c37d..606e5fe60a 100644 --- a/src/backend/optimizer/prep/prepunion.c +++ b/src/backend/optimizer/prep/prepunion.c @@ -1294,6 +1294,7 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti) newrc->rti = childRTindex; newrc->prti = rti; + newrc->rowmarkId = oldrc->rowmarkId; newrc->markType = oldrc->markType; newrc->noWait = oldrc->noWait; newrc->isParent = false; diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 546b581b6d..44da70ff60 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -417,6 +417,7 @@ typedef struct ExecRowMark Relation relation; /* opened and suitably locked relation */ Index rti; /* its range table index */ Index prti; /* parent range table index, if child */ + Index rowmarkId; /* unique identifier for resjunk columns */ RowMarkType markType; /* see enum in nodes/plannodes.h */ bool noWait; /* NOWAIT option */ ItemPointerData curCtid; /* ctid of currently locked tuple, if any */ diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index 6ba2f6005a..b4fb4f211b 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -749,7 +749,11 @@ typedef enum RowMarkType * The tableoid column is only present for an inheritance hierarchy. * When markType == ROW_MARK_COPY, there is instead a single column named * wholerow%u whole-row value of relation - * In all three cases, %u represents the parent rangetable index (prti). + * In all three cases, %u represents the rowmark ID number (rowmarkId). + * This number is unique within a plan tree, except that child relation + * entries copy their parent's rowmarkId. (Assigning unique numbers + * means we needn't renumber rowmarkIds when flattening subqueries, which + * would require finding and renaming the resjunk columns as well.) * Note this means that all tables in an inheritance hierarchy share the * same resjunk column names. However, in an inherited UPDATE/DELETE the * columns could have different physical column numbers in each subplan. @@ -759,6 +763,7 @@ typedef struct PlanRowMark NodeTag type; Index rti; /* range table index of markable relation */ Index prti; /* range table index of parent relation */ + Index rowmarkId; /* unique identifier for resjunk columns */ RowMarkType markType; /* see enum above */ bool noWait; /* NOWAIT option */ bool isParent; /* true if this is a "dummy" parent entry */ diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h index b3ca5b5e32..49ce441e62 100644 --- a/src/include/nodes/relation.h +++ b/src/include/nodes/relation.h @@ -82,6 +82,8 @@ typedef struct PlannerGlobal Index lastPHId; /* highest PlaceHolderVar ID assigned */ + Index lastRowMarkId; /* highest PlanRowMark ID assigned */ + bool transientPlan; /* redo plan when TransactionXmin changes? */ } PlannerGlobal; -- GitLab