Avoid getting more than AccessShareLock when deparsing a query.

In make_ruledef and get_query_def, we have long used AcquireRewriteLocks
to ensure that the querytree we are about to deparse is up-to-date and
the schemas of the underlying relations aren't changing.  Howwever, that
function thinks the query is about to be executed, so it acquires locks
that are stronger than necessary for the purpose of deparsing.  Thus for
example, if pg_dump asks to deparse a rule that includes "INSERT INTO t",
we'd acquire RowExclusiveLock on t.  That results in interference with
concurrent transactions that might for example ask for ShareLock on t.
Since pg_dump is documented as being purely read-only, this is unexpected.
(Worse, it used to actually be read-only; this behavior dates back only
to 8.1, cf commit ba4200246.)

Fix this by adding a parameter to AcquireRewriteLocks to tell it whether
we want the "real" execution locks or only AccessShareLock.

Report, diagnosis, and patch by Dean Rasheed.  Back-patch to all supported
branches.

diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index a331f3e7176cf16c554573786a72d6f680e67ec3..d64f1657066120ab6a9cc8bf98336440503a1819 100644 (file)
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -303,7 +303,7 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
 
    /* Lock and rewrite, using a copy to preserve the original query. */
    copied_query = copyObject(query);
-   AcquireRewriteLocks(copied_query, false);
+   AcquireRewriteLocks(copied_query, true, false);
    rewritten = QueryRewrite(copied_query);
 
    /* SELECT should never rewrite to more or less than one SELECT query */

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 0b136451d8047150daf599d7c8452ae2cbe2499c..f4f49d21f1f22ebf35336f7cd1a03095e44f7eac 100644 (file)
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -37,7 +37,13 @@ typedef struct rewrite_event
    CmdType     event;          /* type of rule being fired */
 } rewrite_event;
 
-static bool acquireLocksOnSubLinks(Node *node, void *context);
+typedef struct acquireLocksOnSubLinks_context
+{
+   bool        for_execute;    /* AcquireRewriteLocks' forExecute param */
+} acquireLocksOnSubLinks_context;
+
+static bool acquireLocksOnSubLinks(Node *node,
+                      acquireLocksOnSubLinks_context *context);
 static Query *rewriteRuleAction(Query *parsetree,
                  Query *rule_action,
                  Node *rule_qual,
@@ -71,9 +77,19 @@ static Bitmapset *adjust_view_column_set(Bitmapset *cols, List *targetlist);
  *   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 [KEY] 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.
+ * forExecute indicates that the query is about to be executed.
+ * If so, we'll acquire RowExclusiveLock on the query's resultRelation,
+ * RowShareLock on any relation accessed FOR [KEY] UPDATE/SHARE, and
+ * AccessShareLock on all other relations mentioned.
+ *
+ * If forExecute is false, AccessShareLock is acquired on all relations.
+ * This case is suitable for ruleutils.c, for example, where we only need
+ * schema stability and we don't intend to actually modify any relations.
+ *
+ * forUpdatePushedDown indicates that a pushed-down FOR [KEY] UPDATE/SHARE
+ * applies to the current subquery, requiring all rels to be opened with at
+ * least RowShareLock.  This should always be false at the top of the
+ * recursion.  This flag is ignored if forExecute is false.
  *
  * 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
@@ -101,10 +117,15 @@ static Bitmapset *adjust_view_column_set(Bitmapset *cols, List *targetlist);
  * construction of a nested join was O(N^2) in the nesting depth.)
  */
 void
-AcquireRewriteLocks(Query *parsetree, bool forUpdatePushedDown)
+AcquireRewriteLocks(Query *parsetree,
+                   bool forExecute,
+                   bool forUpdatePushedDown)
 {
    ListCell   *l;
    int         rt_index;
+   acquireLocksOnSubLinks_context context;
+
+   context.for_execute = forExecute;
 
    /*
     * First, process RTEs of the current query level.
@@ -130,14 +151,12 @@ AcquireRewriteLocks(Query *parsetree, bool forUpdatePushedDown)
                 * release it until end of transaction. This protects the
                 * rewriter and planner against schema changes mid-query.
                 *
-                * If the relation is the query's result relation, then we
-                * need RowExclusiveLock.  Otherwise, check to see if the
-                * relation is accessed FOR [KEY] UPDATE/SHARE or not.  We
-                * can't just grab AccessShareLock because then the executor
-                * would be trying to upgrade the lock, leading to possible
-                * deadlocks.
+                * Assuming forExecute is true, this logic must match what the
+                * executor will do, else we risk lock-upgrade deadlocks.
                 */
-               if (rt_index == parsetree->resultRelation)
+               if (!forExecute)
+                   lockmode = AccessShareLock;
+               else if (rt_index == parsetree->resultRelation)
                    lockmode = RowExclusiveLock;
                else if (forUpdatePushedDown ||
                         get_parse_rowmark(parsetree, rt_index) != NULL)
@@ -225,6 +244,7 @@ AcquireRewriteLocks(Query *parsetree, bool forUpdatePushedDown)
                 * recurse to process the represented subquery.
                 */
                AcquireRewriteLocks(rte->subquery,
+                                   forExecute,
                                    (forUpdatePushedDown ||
                            get_parse_rowmark(parsetree, rt_index) != NULL));
                break;
@@ -240,7 +260,7 @@ AcquireRewriteLocks(Query *parsetree, bool forUpdatePushedDown)
    {
        CommonTableExpr *cte = (CommonTableExpr *) lfirst(l);
 
-       AcquireRewriteLocks((Query *) cte->ctequery, false);
+       AcquireRewriteLocks((Query *) cte->ctequery, forExecute, false);
    }
 
    /*
@@ -248,7 +268,7 @@ AcquireRewriteLocks(Query *parsetree, bool forUpdatePushedDown)
     * the rtable and cteList.
     */
    if (parsetree->hasSubLinks)
-       query_tree_walker(parsetree, acquireLocksOnSubLinks, NULL,
+       query_tree_walker(parsetree, acquireLocksOnSubLinks, &context,
                          QTW_IGNORE_RC_SUBQUERIES);
 }
 
@@ -256,7 +276,7 @@ AcquireRewriteLocks(Query *parsetree, bool forUpdatePushedDown)
  * Walker to find sublink subqueries for AcquireRewriteLocks
  */
 static bool
-acquireLocksOnSubLinks(Node *node, void *context)
+acquireLocksOnSubLinks(Node *node, acquireLocksOnSubLinks_context *context)
 {
    if (node == NULL)
        return false;
@@ -265,7 +285,9 @@ acquireLocksOnSubLinks(Node *node, void *context)
        SubLink    *sub = (SubLink *) node;
 
        /* Do what we came for */
-       AcquireRewriteLocks((Query *) sub->subselect, false);
+       AcquireRewriteLocks((Query *) sub->subselect,
+                           context->for_execute,
+                           false);
        /* Fall through to process lefthand args of SubLink */
    }
 
@@ -307,6 +329,9 @@ rewriteRuleAction(Query *parsetree,
    int         rt_length;
    Query      *sub_action;
    Query     **sub_action_ptr;
+   acquireLocksOnSubLinks_context context;
+
+   context.for_execute = true;
 
    /*
     * Make modifiable copies of rule action and qual (what we're passed are
@@ -318,8 +343,8 @@ rewriteRuleAction(Query *parsetree,
    /*
     * Acquire necessary locks and fix any deleted JOIN RTE entries.
     */
-   AcquireRewriteLocks(rule_action, false);
-   (void) acquireLocksOnSubLinks(rule_qual, NULL);
+   AcquireRewriteLocks(rule_action, true, false);
+   (void) acquireLocksOnSubLinks(rule_qual, &context);
 
    current_varno = rt_index;
    rt_length = list_length(parsetree->rtable);
@@ -1389,7 +1414,7 @@ ApplyRetrieveRule(Query *parsetree,
     */
    rule_action = copyObject(linitial(rule->actions));
 
-   AcquireRewriteLocks(rule_action, forUpdatePushedDown);
+   AcquireRewriteLocks(rule_action, true, forUpdatePushedDown);
 
    /*
     * Recursively expand any view references inside the view.
@@ -1712,6 +1737,9 @@ CopyAndAddInvertedQual(Query *parsetree,
 {
    /* Don't scribble on the passed qual (it's in the relcache!) */
    Node       *new_qual = (Node *) copyObject(rule_qual);
+   acquireLocksOnSubLinks_context context;
+
+   context.for_execute = true;
 
    /*
     * In case there are subqueries in the qual, acquire necessary locks and
@@ -1719,7 +1747,7 @@ CopyAndAddInvertedQual(Query *parsetree,
     * rewriteRuleAction, but not entirely ... consider restructuring so that
     * we only need to process the qual this way once.)
     */
-   (void) acquireLocksOnSubLinks(new_qual, NULL);
+   (void) acquireLocksOnSubLinks(new_qual, &context);
 
    /* Fix references to OLD */
    ChangeVarNodes(new_qual, PRS2_OLD_VARNO, rt_index, 0);

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index add5cd1695b9307b69d15ef962ea47a313bec81d..566b4c910b34a9a251c9f289115d1575faad1421 100644 (file)
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -3966,7 +3966,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, false);
+       AcquireRewriteLocks(query, false, false);
 
        context.buf = buf;
        context.namespaces = list_make1(&dpns);
@@ -4108,8 +4108,11 @@ get_query_def(Query *query, StringInfo buf, List *parentnamespace,
     * relations, and fix up deleted columns in JOIN RTEs.  This ensures
     * consistent results.  Note we assume it's OK to scribble on the passed
     * querytree!
+    *
+    * We are only deparsing the query (we are not about to execute it), so we
+    * only need AccessShareLock on the relations it mentions.
     */
-   AcquireRewriteLocks(query, false);
+   AcquireRewriteLocks(query, false, 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 e4027bd9c56c234f319f2255fbf2197d85e1ed5b..22551ece3ec7caa449d55f04b878311f0b279a42 100644 (file)
--- a/src/include/rewrite/rewriteHandler.h
+++ b/src/include/rewrite/rewriteHandler.h
@@ -18,7 +18,9 @@
 #include "nodes/parsenodes.h"
 
 extern List *QueryRewrite(Query *parsetree);
-extern void AcquireRewriteLocks(Query *parsetree, bool forUpdatePushedDown);
+extern void AcquireRewriteLocks(Query *parsetree,
+                   bool forExecute,
+                   bool forUpdatePushedDown);
 
 extern Node *build_column_default(Relation rel, int attrno);
 extern Query *get_view_query(Relation view);