Avoid redundant relation lock grabs during planning, and make sure

that we acquire a lock on relations added to the query due to inheritance.
Formerly, no such lock was held throughout planning, which meant that
a schema change could occur to invalidate the plan before it's even
been completed.

diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index c4f3115443721debeff641cffa74738b4336030f..22334e238f51c275539d773d8c1344a361e140ab 100644 (file)
--- a/src/backend/optimizer/prep/preptlist.c
+++ b/src/backend/optimizer/prep/preptlist.c
@@ -15,7 +15,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/optimizer/prep/preptlist.c,v 1.75 2005/04/28 21:47:14 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/optimizer/prep/preptlist.c,v 1.76 2005/05/23 03:01:13 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -189,9 +189,11 @@ expand_targetlist(List *tlist, int command_type,
     * attributes.
     *
     * Scan the tuple description in the relation's relcache entry to make
-    * sure we have all the user attributes in the right order.
+    * sure we have all the user attributes in the right order.  We assume
+    * that the rewriter already acquired at least AccessShareLock on the
+    * relation, so we need no lock here.
     */
-   rel = heap_open(getrelid(result_relation, range_table), AccessShareLock);
+   rel = heap_open(getrelid(result_relation, range_table), NoLock);
 
    numattrs = RelationGetNumberOfAttributes(rel);
 
@@ -326,7 +328,7 @@ expand_targetlist(List *tlist, int command_type,
        tlist_item = lnext(tlist_item);
    }
 
-   heap_close(rel, AccessShareLock);
+   heap_close(rel, NoLock);
 
    return new_tlist;
 }

diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index ad4e3cd34724f0eb64542c0e1fc2361e6078ad79..3520ac96077be850ef113ece8bb834c14b97aa9b 100644 (file)
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -9,7 +9,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/optimizer/util/plancat.c,v 1.107 2005/05/22 22:30:20 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/optimizer/util/plancat.c,v 1.108 2005/05/23 03:01:14 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -67,7 +67,25 @@ get_relation_info(Oid relationObjectId, RelOptInfo *rel)
    bool        hasindex;
    List       *indexinfos = NIL;
 
-   relation = heap_open(relationObjectId, AccessShareLock);
+   /*
+    * Normally, we can assume the rewriter already acquired at least
+    * AccessShareLock on each relation used in the query.  However this
+    * will not be the case for relations added to the query because they
+    * are inheritance children of some relation mentioned explicitly.
+    * For them, this is the first access during the parse/rewrite/plan
+    * pipeline, and so we need to obtain and keep a suitable lock.
+    *
+    * XXX really, a suitable lock is RowShareLock if the relation is
+    * an UPDATE/DELETE target, and AccessShareLock otherwise.  However
+    * we cannot easily tell here which to get, so for the moment just
+    * get AccessShareLock always.  The executor will get the right lock
+    * when it runs, which means there is a very small chance of deadlock
+    * trying to upgrade our lock.
+    */
+   if (rel->reloptkind == RELOPT_BASEREL)
+       relation = heap_open(relationObjectId, NoLock);
+   else
+       relation = heap_open(relationObjectId, AccessShareLock);
 
    rel->min_attr = FirstLowInvalidHeapAttributeNumber + 1;
    rel->max_attr = RelationGetNumberOfAttributes(relation);
@@ -205,8 +223,8 @@ get_relation_info(Oid relationObjectId, RelOptInfo *rel)
 
    rel->indexlist = indexinfos;
 
-   /* XXX keep the lock here? */
-   heap_close(relation, AccessShareLock);
+   /* close rel, but keep lock if any */
+   heap_close(relation, NoLock);
 }
 
 /*
@@ -374,7 +392,8 @@ build_physical_tlist(Query *root, RelOptInfo *rel)
    switch (rte->rtekind)
    {
        case RTE_RELATION:
-           relation = heap_open(rte->relid, AccessShareLock);
+           /* Assume we already have adequate lock */
+           relation = heap_open(rte->relid, NoLock);
 
            numattrs = RelationGetNumberOfAttributes(relation);
            for (attrno = 1; attrno <= numattrs; attrno++)
@@ -401,7 +420,7 @@ build_physical_tlist(Query *root, RelOptInfo *rel)
                                                false));
            }
 
-           heap_close(relation, AccessShareLock);
+           heap_close(relation, NoLock);
            break;
 
        case RTE_SUBQUERY:

diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index 4192d9b805b6e9c021a35f3ee29a59c6a4093a54..f679768cfe9c3f697fe85ab3023a444fceaa8b14 100644 (file)
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/optimizer/util/relnode.c,v 1.65 2005/04/22 21:58:31 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/optimizer/util/relnode.c,v 1.66 2005/05/23 03:01:14 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -23,7 +23,8 @@
 #include "parser/parsetree.h"
 
 
-static RelOptInfo *make_base_rel(Query *root, int relid);
+static RelOptInfo *make_reloptinfo(Query *root, int relid,
+                                  RelOptKind reloptkind);
 static void build_joinrel_tlist(Query *root, RelOptInfo *joinrel);
 static List *build_joinrel_restrictlist(Query *root,
                           RelOptInfo *joinrel,
@@ -67,7 +68,7 @@ build_base_rel(Query *root, int relid)
    }
 
    /* No existing RelOptInfo for this base rel, so make a new one */
-   rel = make_base_rel(root, relid);
+   rel = make_reloptinfo(root, relid, RELOPT_BASEREL);
 
    /* and add it to the list */
    root->base_rel_list = lcons(rel, root->base_rel_list);
@@ -102,11 +103,8 @@ build_other_rel(Query *root, int relid)
    }
 
    /* No existing RelOptInfo for this other rel, so make a new one */
-   rel = make_base_rel(root, relid);
-
    /* presently, must be an inheritance child rel */
-   Assert(rel->reloptkind == RELOPT_BASEREL);
-   rel->reloptkind = RELOPT_OTHER_CHILD_REL;
+   rel = make_reloptinfo(root, relid, RELOPT_OTHER_CHILD_REL);
 
    /* and add it to the list */
    root->other_rel_list = lcons(rel, root->other_rel_list);
@@ -115,18 +113,18 @@ build_other_rel(Query *root, int relid)
 }
 
 /*
- * make_base_rel
- *   Construct a base-relation RelOptInfo for the specified rangetable index.
+ * make_reloptinfo
+ *   Construct a RelOptInfo for the specified rangetable index.
  *
  * Common code for build_base_rel and build_other_rel.
  */
 static RelOptInfo *
-make_base_rel(Query *root, int relid)
+make_reloptinfo(Query *root, int relid, RelOptKind reloptkind)
 {
    RelOptInfo *rel = makeNode(RelOptInfo);
    RangeTblEntry *rte = rt_fetch(relid, root->rtable);
 
-   rel->reloptkind = RELOPT_BASEREL;
+   rel->reloptkind = reloptkind;
    rel->relids = bms_make_singleton(relid);
    rel->rows = 0;
    rel->width = 0;