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.

3 files changed, 40 insertions, 21 deletions

diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index c4f31154437..22334e238f5 100644
--- 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 ad4e3cd3472..3520ac96077 100644
--- 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 4192d9b805b..f679768cfe9 100644
--- 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;