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;