From: Tom Lane Date: Wed, 28 Oct 2009 17:36:50 +0000 (+0000) Subject: Fix AcquireRewriteLocks to be sure that it acquires the right lock strength X-Git-Tag: REL8_5_ALPHA3~175 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=cbcd1701f12d133a04b8a104bff20738b2c29e4b;p=postgresql Fix AcquireRewriteLocks to be sure that it acquires the right lock strength when FOR UPDATE is propagated down into a sub-select expanded from a view. Similar bug to parser's isLockedRel issue that I fixed yesterday; likewise seems not quite worth the effort to back-patch. --- diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index 8f5b300227..59205f4d55 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -7,7 +7,7 @@ * Portions Copyright (c) 1994, Regents of the University of California * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/rewrite/rewriteHandler.c,v 1.190 2009/10/28 14:55:43 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/rewrite/rewriteHandler.c,v 1.191 2009/10/28 17:36:50 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -55,7 +55,8 @@ static void markQueryForLocking(Query *qry, Node *jtnode, bool forUpdate, bool noWait, bool pushedDown); static List *matchLocks(CmdType event, RuleLock *rulelocks, int varno, Query *parsetree); -static Query *fireRIRrules(Query *parsetree, List *activeRIRs); +static Query *fireRIRrules(Query *parsetree, List *activeRIRs, + bool forUpdatePushedDown); /* @@ -64,6 +65,10 @@ static Query *fireRIRrules(Query *parsetree, List *activeRIRs); * These locks will ensure that the relation schemas don't change under us * while we are rewriting and planning the query. * + * forUpdatePushedDown indicates that a pushed-down FOR UPDATE/SHARE applies + * to the current subquery, requiring all rels to be opened with RowShareLock. + * This should always be false at the start of the recursion. + * * A secondary purpose of this routine is to fix up JOIN RTE references to * dropped columns (see details below). Because the RTEs are modified in * place, it is generally appropriate for the caller of this routine to have @@ -91,7 +96,7 @@ static Query *fireRIRrules(Query *parsetree, List *activeRIRs); * construction of a nested join was O(N^2) in the nesting depth.) */ void -AcquireRewriteLocks(Query *parsetree) +AcquireRewriteLocks(Query *parsetree, bool forUpdatePushedDown) { ListCell *l; int rt_index; @@ -129,7 +134,8 @@ AcquireRewriteLocks(Query *parsetree) */ if (rt_index == parsetree->resultRelation) lockmode = RowExclusiveLock; - else if (get_parse_rowmark(parsetree, rt_index) != NULL) + else if (forUpdatePushedDown || + get_parse_rowmark(parsetree, rt_index) != NULL) lockmode = RowShareLock; else lockmode = AccessShareLock; @@ -206,7 +212,9 @@ AcquireRewriteLocks(Query *parsetree) * The subquery RTE itself is all right, but we have to * recurse to process the represented subquery. */ - AcquireRewriteLocks(rte->subquery); + AcquireRewriteLocks(rte->subquery, + (forUpdatePushedDown || + get_parse_rowmark(parsetree, rt_index) != NULL)); break; default: @@ -220,7 +228,7 @@ AcquireRewriteLocks(Query *parsetree) { CommonTableExpr *cte = (CommonTableExpr *) lfirst(l); - AcquireRewriteLocks((Query *) cte->ctequery); + AcquireRewriteLocks((Query *) cte->ctequery, false); } /* @@ -245,7 +253,7 @@ acquireLocksOnSubLinks(Node *node, void *context) SubLink *sub = (SubLink *) node; /* Do what we came for */ - AcquireRewriteLocks((Query *) sub->subselect); + AcquireRewriteLocks((Query *) sub->subselect, false); /* Fall through to process lefthand args of SubLink */ } @@ -298,7 +306,7 @@ rewriteRuleAction(Query *parsetree, /* * Acquire necessary locks and fix any deleted JOIN RTE entries. */ - AcquireRewriteLocks(rule_action); + AcquireRewriteLocks(rule_action, false); (void) acquireLocksOnSubLinks(rule_qual, NULL); current_varno = rt_index; @@ -1134,7 +1142,8 @@ ApplyRetrieveRule(Query *parsetree, int rt_index, bool relation_level, Relation relation, - List *activeRIRs) + List *activeRIRs, + bool forUpdatePushedDown) { Query *rule_action; RangeTblEntry *rte, @@ -1148,18 +1157,25 @@ ApplyRetrieveRule(Query *parsetree, if (!relation_level) elog(ERROR, "cannot handle per-attribute ON SELECT rule"); + /* + * If FOR UPDATE/SHARE of view, be sure we get right initial lock on the + * relations it references. + */ + rc = get_parse_rowmark(parsetree, rt_index); + forUpdatePushedDown |= (rc != NULL); + /* * Make a modifiable copy of the view query, and acquire needed locks on * the relations it mentions. */ rule_action = copyObject(linitial(rule->actions)); - AcquireRewriteLocks(rule_action); + AcquireRewriteLocks(rule_action, forUpdatePushedDown); /* * Recursively expand any view references inside the view. */ - rule_action = fireRIRrules(rule_action, activeRIRs); + rule_action = fireRIRrules(rule_action, activeRIRs, forUpdatePushedDown); /* * VIEWs are really easy --- just plug the view query in as a subselect, @@ -1192,8 +1208,11 @@ ApplyRetrieveRule(Query *parsetree, * If FOR UPDATE/SHARE of view, mark all the contained tables as * implicit FOR UPDATE/SHARE, the same as the parser would have done * if the view's subquery had been written out explicitly. + * + * Note: we don't consider forUpdatePushedDown here; such marks will be + * made by recursing from the upper level in markQueryForLocking. */ - if ((rc = get_parse_rowmark(parsetree, rt_index)) != NULL) + if (rc != NULL) markQueryForLocking(rule_action, (Node *) rule_action->jointree, rc->forUpdate, rc->noWait, true); @@ -1281,7 +1300,7 @@ fireRIRonSubLink(Node *node, List *activeRIRs) /* Do what we came for */ sub->subselect = (Node *) fireRIRrules((Query *) sub->subselect, - activeRIRs); + activeRIRs, false); /* Fall through to process lefthand args of SubLink */ } @@ -1299,7 +1318,7 @@ fireRIRonSubLink(Node *node, List *activeRIRs) * Apply all RIR rules on each rangetable entry in a query */ static Query * -fireRIRrules(Query *parsetree, List *activeRIRs) +fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown) { int rt_index; ListCell *lc; @@ -1329,7 +1348,9 @@ fireRIRrules(Query *parsetree, List *activeRIRs) */ if (rte->rtekind == RTE_SUBQUERY) { - rte->subquery = fireRIRrules(rte->subquery, activeRIRs); + rte->subquery = fireRIRrules(rte->subquery, activeRIRs, + (forUpdatePushedDown || + get_parse_rowmark(parsetree, rt_index) != NULL)); continue; } @@ -1406,7 +1427,8 @@ fireRIRrules(Query *parsetree, List *activeRIRs) rt_index, rule->attrno == -1, rel, - activeRIRs); + activeRIRs, + forUpdatePushedDown); } activeRIRs = list_delete_first(activeRIRs); @@ -1421,7 +1443,7 @@ fireRIRrules(Query *parsetree, List *activeRIRs) CommonTableExpr *cte = (CommonTableExpr *) lfirst(lc); cte->ctequery = (Node *) - fireRIRrules((Query *) cte->ctequery, activeRIRs); + fireRIRrules((Query *) cte->ctequery, activeRIRs, false); } /* @@ -1850,7 +1872,7 @@ QueryRewrite(Query *parsetree) { Query *query = (Query *) lfirst(l); - query = fireRIRrules(query, NIL); + query = fireRIRrules(query, NIL, false); /* * If the query target was rewritten as a view, complain. diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 85e52293e7..99dfd88ba5 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -9,7 +9,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/utils/adt/ruleutils.c,v 1.311 2009/10/28 14:55:44 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/utils/adt/ruleutils.c,v 1.312 2009/10/28 17:36:50 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -2175,7 +2175,7 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, query = getInsertSelectQuery(query, NULL); /* Must acquire locks right away; see notes in get_query_def() */ - AcquireRewriteLocks(query); + AcquireRewriteLocks(query, false); context.buf = buf; context.namespaces = list_make1(&dpns); @@ -2320,7 +2320,7 @@ get_query_def(Query *query, StringInfo buf, List *parentnamespace, * consistent results. Note we assume it's OK to scribble on the passed * querytree! */ - AcquireRewriteLocks(query); + AcquireRewriteLocks(query, false); context.buf = buf; context.namespaces = lcons(&dpns, list_copy(parentnamespace)); diff --git a/src/include/rewrite/rewriteHandler.h b/src/include/rewrite/rewriteHandler.h index 4caef86757..68e1ad8d6f 100644 --- a/src/include/rewrite/rewriteHandler.h +++ b/src/include/rewrite/rewriteHandler.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/rewrite/rewriteHandler.h,v 1.31 2009/01/01 17:24:01 momjian Exp $ + * $PostgreSQL: pgsql/src/include/rewrite/rewriteHandler.h,v 1.32 2009/10/28 17:36:50 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -18,7 +18,7 @@ #include "nodes/parsenodes.h" extern List *QueryRewrite(Query *parsetree); -extern void AcquireRewriteLocks(Query *parsetree); +extern void AcquireRewriteLocks(Query *parsetree, bool forUpdatePushedDown); extern Node *build_column_default(Relation rel, int attrno); #endif /* REWRITEHANDLER_H */