Simplify view-expansion code in rewriteHandler.c.

In the wake of commit 50c6bb022, it's not necessary for ApplyRetrieveRule
to have a forUpdatePushedDown parameter.  By the time control gets here for
any given view-referencing RTE, we should already have pushed down the
effects of any FOR UPDATE/SHARE clauses affecting the view from outer query
levels.  Hence if we don't find a RowMarkClause at the current query level,
that's sufficient proof that there is no outer one either.  This in turn
means we need no forUpdatePushedDown parameter for fireRIRrules.

I wonder whether we oughtn't also revert commit cba2d2717, since it now
seems likely that that was band-aiding around the bad effects of doing
FOR UPDATE pushdown and view expansion in the wrong order.  However,
in the absence of evidence that the current coding of markQueryForLocking
is actually buggy (i.e. missing RTEs it ought to mark), it seems best to
leave it alone.

Discussion: https://postgr.es/m/24db7b8f-3de5-e25f-7ab9-d8848351d42c@gmail.com
This commit is contained in:
Tom Lane 2018-04-14 21:00:54 -04:00
parent 4d64abc2fe
commit 49ac4039b2
1 changed files with 21 additions and 24 deletions

View File

@ -77,8 +77,7 @@ static void markQueryForLocking(Query *qry, Node *jtnode,
bool pushedDown); bool pushedDown);
static List *matchLocks(CmdType event, RuleLock *rulelocks, static List *matchLocks(CmdType event, RuleLock *rulelocks,
int varno, Query *parsetree, bool *hasUpdate); int varno, Query *parsetree, bool *hasUpdate);
static Query *fireRIRrules(Query *parsetree, List *activeRIRs, static Query *fireRIRrules(Query *parsetree, List *activeRIRs);
bool forUpdatePushedDown);
static bool view_has_instead_trigger(Relation view, CmdType event); static bool view_has_instead_trigger(Relation view, CmdType event);
static Bitmapset *adjust_view_column_set(Bitmapset *cols, List *targetlist); static Bitmapset *adjust_view_column_set(Bitmapset *cols, List *targetlist);
@ -1453,8 +1452,7 @@ ApplyRetrieveRule(Query *parsetree,
RewriteRule *rule, RewriteRule *rule,
int rt_index, int rt_index,
Relation relation, Relation relation,
List *activeRIRs, List *activeRIRs)
bool forUpdatePushedDown)
{ {
Query *rule_action; Query *rule_action;
RangeTblEntry *rte, RangeTblEntry *rte,
@ -1545,28 +1543,27 @@ ApplyRetrieveRule(Query *parsetree,
} }
/* /*
* If FOR [KEY] UPDATE/SHARE of view, be sure we get right initial lock on * Check if there's a FOR [KEY] UPDATE/SHARE clause applying to this view.
* the relations it references. *
* Note: we needn't explicitly consider any such clauses appearing in
* ancestor query levels; their effects have already been pushed down to
* here by markQueryForLocking, and will be reflected in "rc".
*/ */
rc = get_parse_rowmark(parsetree, rt_index); rc = get_parse_rowmark(parsetree, rt_index);
forUpdatePushedDown |= (rc != NULL);
/* /*
* Make a modifiable copy of the view query, and acquire needed locks on * Make a modifiable copy of the view query, and acquire needed locks on
* the relations it mentions. * the relations it mentions. Force at least RowShareLock for all such
* rels if there's a FOR [KEY] UPDATE/SHARE clause affecting this view.
*/ */
rule_action = copyObject(linitial(rule->actions)); rule_action = copyObject(linitial(rule->actions));
AcquireRewriteLocks(rule_action, true, forUpdatePushedDown); AcquireRewriteLocks(rule_action, true, (rc != NULL));
/* /*
* If FOR [KEY] UPDATE/SHARE of view, mark all the contained tables as * If FOR [KEY] UPDATE/SHARE of view, mark all the contained tables as
* implicit FOR [KEY] UPDATE/SHARE, the same as the parser would have done * implicit FOR [KEY] UPDATE/SHARE, the same as the parser would have done
* if the view's subquery had been written out explicitly. * if the view's subquery had been written out explicitly.
*
* Note: we needn't consider forUpdatePushedDown for this; if there was an
* ancestor query level with a relevant FOR [KEY] UPDATE/SHARE clause,
* that's already been pushed down to here and is reflected in "rc".
*/ */
if (rc != NULL) if (rc != NULL)
markQueryForLocking(rule_action, (Node *) rule_action->jointree, markQueryForLocking(rule_action, (Node *) rule_action->jointree,
@ -1581,7 +1578,7 @@ ApplyRetrieveRule(Query *parsetree,
* OLD rangetable entries by the action below (in a recursive call of this * OLD rangetable entries by the action below (in a recursive call of this
* routine). * routine).
*/ */
rule_action = fireRIRrules(rule_action, activeRIRs, forUpdatePushedDown); rule_action = fireRIRrules(rule_action, activeRIRs);
/* /*
* Now, plug the view query in as a subselect, replacing the relation's * Now, plug the view query in as a subselect, replacing the relation's
@ -1698,7 +1695,7 @@ fireRIRonSubLink(Node *node, List *activeRIRs)
/* Do what we came for */ /* Do what we came for */
sub->subselect = (Node *) fireRIRrules((Query *) sub->subselect, sub->subselect = (Node *) fireRIRrules((Query *) sub->subselect,
activeRIRs, false); activeRIRs);
/* Fall through to process lefthand args of SubLink */ /* Fall through to process lefthand args of SubLink */
} }
@ -1713,10 +1710,13 @@ fireRIRonSubLink(Node *node, List *activeRIRs)
/* /*
* fireRIRrules - * fireRIRrules -
* Apply all RIR rules on each rangetable entry in a query * Apply all RIR rules on each rangetable entry in the given query
*
* activeRIRs is a list of the OIDs of views we're already processing RIR
* rules for, used to detect/reject recursion.
*/ */
static Query * static Query *
fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown) fireRIRrules(Query *parsetree, List *activeRIRs)
{ {
int origResultRelation = parsetree->resultRelation; int origResultRelation = parsetree->resultRelation;
int rt_index; int rt_index;
@ -1747,9 +1747,7 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
*/ */
if (rte->rtekind == RTE_SUBQUERY) 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; continue;
} }
@ -1835,8 +1833,7 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
rule, rule,
rt_index, rt_index,
rel, rel,
activeRIRs, activeRIRs);
forUpdatePushedDown);
} }
activeRIRs = list_delete_first(activeRIRs); activeRIRs = list_delete_first(activeRIRs);
@ -1852,7 +1849,7 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
CommonTableExpr *cte = (CommonTableExpr *) lfirst(lc); CommonTableExpr *cte = (CommonTableExpr *) lfirst(lc);
cte->ctequery = (Node *) cte->ctequery = (Node *)
fireRIRrules((Query *) cte->ctequery, activeRIRs, false); fireRIRrules((Query *) cte->ctequery, activeRIRs);
} }
/* /*
@ -3604,7 +3601,7 @@ QueryRewrite(Query *parsetree)
{ {
Query *query = (Query *) lfirst(l); Query *query = (Query *) lfirst(l);
query = fireRIRrules(query, NIL, false); query = fireRIRrules(query, NIL);
query->queryId = input_query_id; query->queryId = input_query_id;