diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 63f057704e6..478e12484b9 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -822,13 +822,14 @@ postgresGetForeignPlan(PlannerInfo *root, } else { - RowMarkClause *rc = get_parse_rowmark(root->parse, baserel->relid); + PlanRowMark *rc = get_plan_rowmark(root->rowMarks, baserel->relid); if (rc) { /* * Relation is specified as a FOR UPDATE/SHARE target, so handle - * that. + * that. (But we could also see LCS_NONE, meaning this isn't a + * target relation after all.) * * For now, just ignore any [NO] KEY specification, since (a) it's * not clear what that means for a remote table that we don't have @@ -837,6 +838,9 @@ postgresGetForeignPlan(PlannerInfo *root, */ switch (rc->strength) { + case LCS_NONE: + /* No locking needed */ + break; case LCS_FORKEYSHARE: case LCS_FORSHARE: appendStringInfoString(&sql, " FOR SHARE"); diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 291e6a7c391..3c6a964a659 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -991,6 +991,8 @@ _copyPlanRowMark(const PlanRowMark *from) COPY_SCALAR_FIELD(prti); COPY_SCALAR_FIELD(rowmarkId); COPY_SCALAR_FIELD(markType); + COPY_SCALAR_FIELD(allMarkTypes); + COPY_SCALAR_FIELD(strength); COPY_SCALAR_FIELD(waitPolicy); COPY_SCALAR_FIELD(isParent); @@ -2510,7 +2512,7 @@ _copyXmlSerialize(const XmlSerialize *from) static RoleSpec * _copyRoleSpec(const RoleSpec *from) { - RoleSpec *newnode = makeNode(RoleSpec); + RoleSpec *newnode = makeNode(RoleSpec); COPY_SCALAR_FIELD(roletype); COPY_STRING_FIELD(rolename); diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index fc418fcf9de..385b289bed2 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -852,7 +852,9 @@ _outPlanRowMark(StringInfo str, const PlanRowMark *node) WRITE_UINT_FIELD(prti); WRITE_UINT_FIELD(rowmarkId); WRITE_ENUM_FIELD(markType, RowMarkType); - WRITE_BOOL_FIELD(waitPolicy); + WRITE_INT_FIELD(allMarkTypes); + WRITE_ENUM_FIELD(strength, LockClauseStrength); + WRITE_ENUM_FIELD(waitPolicy, LockWaitPolicy); WRITE_BOOL_FIELD(isParent); } diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 88b91f1b205..05687a48c9a 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -2219,35 +2219,42 @@ preprocess_rowmarks(PlannerInfo *root) if (rte->rtekind != RTE_RELATION) continue; - /* - * Similarly, ignore RowMarkClauses for foreign tables; foreign tables - * will instead get ROW_MARK_COPY items in the next loop. (FDWs might - * choose to do something special while fetching their rows, but that - * is of no concern here.) - */ - if (rte->relkind == RELKIND_FOREIGN_TABLE) - continue; - rels = bms_del_member(rels, rc->rti); newrc = makeNode(PlanRowMark); newrc->rti = newrc->prti = rc->rti; newrc->rowmarkId = ++(root->glob->lastRowMarkId); - switch (rc->strength) + if (rte->relkind == RELKIND_FOREIGN_TABLE) { - case LCS_FORUPDATE: - newrc->markType = ROW_MARK_EXCLUSIVE; - break; - case LCS_FORNOKEYUPDATE: - newrc->markType = ROW_MARK_NOKEYEXCLUSIVE; - break; - case LCS_FORSHARE: - newrc->markType = ROW_MARK_SHARE; - break; - case LCS_FORKEYSHARE: - newrc->markType = ROW_MARK_KEYSHARE; - break; + /* For now, we force all foreign tables to use ROW_MARK_COPY */ + newrc->markType = ROW_MARK_COPY; } + else + { + /* regular table, apply the appropriate lock type */ + switch (rc->strength) + { + case LCS_NONE: + /* we intentionally throw an error for LCS_NONE */ + elog(ERROR, "unrecognized LockClauseStrength %d", + (int) rc->strength); + break; + case LCS_FORKEYSHARE: + newrc->markType = ROW_MARK_KEYSHARE; + break; + case LCS_FORSHARE: + newrc->markType = ROW_MARK_SHARE; + break; + case LCS_FORNOKEYUPDATE: + newrc->markType = ROW_MARK_NOKEYEXCLUSIVE; + break; + case LCS_FORUPDATE: + newrc->markType = ROW_MARK_EXCLUSIVE; + break; + } + } + newrc->allMarkTypes = (1 << newrc->markType); + newrc->strength = rc->strength; newrc->waitPolicy = rc->waitPolicy; newrc->isParent = false; @@ -2276,6 +2283,8 @@ preprocess_rowmarks(PlannerInfo *root) newrc->markType = ROW_MARK_REFERENCE; else newrc->markType = ROW_MARK_COPY; + newrc->allMarkTypes = (1 << newrc->markType); + newrc->strength = LCS_NONE; newrc->waitPolicy = LockWaitBlock; /* doesn't matter */ newrc->isParent = false; diff --git a/src/backend/optimizer/prep/prepsecurity.c b/src/backend/optimizer/prep/prepsecurity.c index b382f13504e..0be44c1c2f7 100644 --- a/src/backend/optimizer/prep/prepsecurity.c +++ b/src/backend/optimizer/prep/prepsecurity.c @@ -73,8 +73,8 @@ expand_security_quals(PlannerInfo *root, List *tlist) rt_index = 0; foreach(cell, parse->rtable) { - bool targetRelation = false; - RangeTblEntry *rte = (RangeTblEntry *) lfirst(cell); + bool targetRelation = false; + RangeTblEntry *rte = (RangeTblEntry *) lfirst(cell); rt_index++; @@ -241,30 +241,10 @@ expand_security_qual(PlannerInfo *root, List *tlist, int rt_index, rc = get_plan_rowmark(root->rowMarks, rt_index); if (rc != NULL) { - switch (rc->markType) - { - case ROW_MARK_EXCLUSIVE: - applyLockingClause(subquery, 1, LCS_FORUPDATE, - rc->waitPolicy, false); - break; - case ROW_MARK_NOKEYEXCLUSIVE: - applyLockingClause(subquery, 1, LCS_FORNOKEYUPDATE, - rc->waitPolicy, false); - break; - case ROW_MARK_SHARE: - applyLockingClause(subquery, 1, LCS_FORSHARE, - rc->waitPolicy, false); - break; - case ROW_MARK_KEYSHARE: - applyLockingClause(subquery, 1, LCS_FORKEYSHARE, - rc->waitPolicy, false); - break; - case ROW_MARK_REFERENCE: - case ROW_MARK_COPY: - /* No locking needed */ - break; - } - root->rowMarks = list_delete(root->rowMarks, rc); + if (rc->strength != LCS_NONE) + applyLockingClause(subquery, 1, rc->strength, + rc->waitPolicy, false); + root->rowMarks = list_delete_ptr(root->rowMarks, rc); } /* @@ -276,6 +256,7 @@ expand_security_qual(PlannerInfo *root, List *tlist, int rt_index, if (targetRelation) applyLockingClause(subquery, 1, LCS_FORUPDATE, LockWaitBlock, false); + /* * Replace any variables in the outer query that refer to the * original relation RTE with references to columns that we will diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c index 8a6c3cc5e5c..08e7c446d88 100644 --- a/src/backend/optimizer/prep/preptlist.c +++ b/src/backend/optimizer/prep/preptlist.c @@ -92,9 +92,9 @@ preprocess_targetlist(PlannerInfo *root, List *tlist) if (rc->rti != rc->prti) continue; - if (rc->markType != ROW_MARK_COPY) + if (rc->allMarkTypes & ~(1 << ROW_MARK_COPY)) { - /* It's a regular table, so fetch its TID */ + /* Need to fetch TID */ var = makeVar(rc->rti, SelfItemPointerAttributeNumber, TIDOID, @@ -125,9 +125,9 @@ preprocess_targetlist(PlannerInfo *root, List *tlist) tlist = lappend(tlist, tle); } } - else + if (rc->allMarkTypes & (1 << ROW_MARK_COPY)) { - /* Not a table, so we need the whole row as a junk var */ + /* Need the whole row as a junk var */ var = makeWholeRowVar(rt_fetch(rc->rti, range_table), rc->rti, 0, diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c index b90fee387b4..cd40afce767 100644 --- a/src/backend/optimizer/prep/prepunion.c +++ b/src/backend/optimizer/prep/prepunion.c @@ -1389,9 +1389,14 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti) newrc->prti = rti; newrc->rowmarkId = oldrc->rowmarkId; newrc->markType = oldrc->markType; + newrc->allMarkTypes = (1 << newrc->markType); + newrc->strength = oldrc->strength; newrc->waitPolicy = oldrc->waitPolicy; newrc->isParent = false; + /* Include child's rowmark type in parent's allMarkTypes */ + oldrc->allMarkTypes |= newrc->allMarkTypes; + root->rowMarks = lappend(root->rowMarks, newrc); } diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index a68f2e8bb14..4a5a5205391 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -2254,11 +2254,18 @@ transformCreateTableAsStmt(ParseState *pstate, CreateTableAsStmt *stmt) } -char * +/* + * Produce a string representation of a LockClauseStrength value. + * This should only be applied to valid values (not LCS_NONE). + */ +const char * LCS_asString(LockClauseStrength strength) { switch (strength) { + case LCS_NONE: + Assert(false); + break; case LCS_FORKEYSHARE: return "FOR KEY SHARE"; case LCS_FORSHARE: @@ -2279,6 +2286,8 @@ LCS_asString(LockClauseStrength strength) void CheckSelectLocking(Query *qry, LockClauseStrength strength) { + Assert(strength != LCS_NONE); /* else caller error */ + if (qry->setOperations) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), @@ -2498,6 +2507,8 @@ applyLockingClause(Query *qry, Index rtindex, { RowMarkClause *rc; + Assert(strength != LCS_NONE); /* else caller error */ + /* If it's an explicit clause, make sure hasForUpdate gets set */ if (!pushedDown) qry->hasForUpdate = true; @@ -2506,20 +2517,21 @@ applyLockingClause(Query *qry, Index rtindex, if ((rc = get_parse_rowmark(qry, rtindex)) != NULL) { /* - * If the same RTE is specified for more than one locking strength, - * treat is as the strongest. (Reasonable, since you can't take both - * a shared and exclusive lock at the same time; it'll end up being - * exclusive anyway.) + * If the same RTE is specified with more than one locking strength, + * use the strongest. (Reasonable, since you can't take both a shared + * and exclusive lock at the same time; it'll end up being exclusive + * anyway.) * - * Similarly, if the same RTE is specified with more than one lock wait - * policy, consider that NOWAIT wins over SKIP LOCKED, which in turn - * wins over waiting for the lock (the default). This is a bit more - * debatable but raising an error doesn't seem helpful. (Consider for - * instance SELECT FOR UPDATE NOWAIT from a view that internally + * Similarly, if the same RTE is specified with more than one lock + * wait policy, consider that NOWAIT wins over SKIP LOCKED, which in + * turn wins over waiting for the lock (the default). This is a bit + * more debatable but raising an error doesn't seem helpful. (Consider + * for instance SELECT FOR UPDATE NOWAIT from a view that internally * contains a plain FOR UPDATE spec.) Having NOWAIT win over SKIP * LOCKED is reasonable since the former throws an error in case of - * coming across a locked tuple, which may be undesirable in some cases - * but it seems better than silently returning inconsistent results. + * coming across a locked tuple, which may be undesirable in some + * cases but it seems better than silently returning inconsistent + * results. * * And of course pushedDown becomes false if any clause is explicit. */ diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 126e38d7f73..065475dda2a 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -2395,26 +2395,22 @@ CreateCommandTag(Node *parsetree) else if (stmt->rowMarks != NIL) { /* not 100% but probably close enough */ - switch (((PlanRowMark *) linitial(stmt->rowMarks))->markType) + switch (((PlanRowMark *) linitial(stmt->rowMarks))->strength) { - case ROW_MARK_EXCLUSIVE: - tag = "SELECT FOR UPDATE"; - break; - case ROW_MARK_NOKEYEXCLUSIVE: - tag = "SELECT FOR NO KEY UPDATE"; - break; - case ROW_MARK_SHARE: - tag = "SELECT FOR SHARE"; - break; - case ROW_MARK_KEYSHARE: + case LCS_FORKEYSHARE: tag = "SELECT FOR KEY SHARE"; break; - case ROW_MARK_REFERENCE: - case ROW_MARK_COPY: - tag = "SELECT"; + case LCS_FORSHARE: + tag = "SELECT FOR SHARE"; + break; + case LCS_FORNOKEYUPDATE: + tag = "SELECT FOR NO KEY UPDATE"; + break; + case LCS_FORUPDATE: + tag = "SELECT FOR UPDATE"; break; default: - tag = "???"; + tag = "SELECT"; break; } } diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 2fa30be401f..28e1acfb86a 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -4453,6 +4453,11 @@ get_select_query_def(Query *query, deparse_context *context, switch (rc->strength) { + case LCS_NONE: + /* we intentionally throw an error for LCS_NONE */ + elog(ERROR, "unrecognized LockClauseStrength %d", + (int) rc->strength); + break; case LCS_FORKEYSHARE: appendContextKeyword(context, " FOR KEY SHARE", -PRETTYINDENT_STD, PRETTYINDENT_STD, 0); diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h index 76c64cd1227..479fc91d466 100644 --- a/src/include/catalog/catversion.h +++ b/src/include/catalog/catversion.h @@ -53,6 +53,6 @@ */ /* yyyymmddN */ -#define CATALOG_VERSION_NO 201503061 +#define CATALOG_VERSION_NO 201503151 #endif diff --git a/src/include/nodes/lockoptions.h b/src/include/nodes/lockoptions.h index 55324baf40f..2e55622b043 100644 --- a/src/include/nodes/lockoptions.h +++ b/src/include/nodes/lockoptions.h @@ -20,6 +20,7 @@ */ typedef enum LockClauseStrength { + LCS_NONE, /* no such clause - only used in PlanRowMark */ LCS_FORKEYSHARE, /* FOR KEY SHARE */ LCS_FORSHARE, /* FOR SHARE */ LCS_FORNOKEYUPDATE, /* FOR NO KEY UPDATE */ diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index af44ddf5dc5..21cbfa8cf0f 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -820,7 +820,7 @@ typedef enum RowMarkType ROW_MARK_NOKEYEXCLUSIVE, /* obtain no-key exclusive tuple lock */ ROW_MARK_SHARE, /* obtain shared tuple lock */ ROW_MARK_KEYSHARE, /* obtain keyshare tuple lock */ - ROW_MARK_REFERENCE, /* just fetch the TID */ + ROW_MARK_REFERENCE, /* just fetch the TID, don't lock it */ ROW_MARK_COPY /* physically copy the row value */ } RowMarkType; @@ -841,7 +841,9 @@ typedef enum RowMarkType * list for each child relation (including the target rel itself in its role * as a child). The child entries have rti == child rel's RT index and * prti == parent's RT index, and can therefore be recognized as children by - * the fact that prti != rti. + * the fact that prti != rti. The parent's allMarkTypes field gets the OR + * of (1<