Revise handling of dropped columns in JOIN alias lists to avoid a

performance problem pointed out by phil@vodafone: to wit, we were
spending O(N^2) time to check dropped-ness in an N-deep join tree,
even in the case where the tree was freshly constructed and couldn't
possibly mention any dropped columns.  Instead of recursing in
get_rte_attribute_is_dropped(), change the data structure definition:
the joinaliasvars list of a JOIN RTE must have a NULL Const instead
of a Var at any position that references a now-dropped column.  This
costs nothing during normal parse-rewrite-plan path, and instead we
have a linear-time update to make when loading a stored rule that
might contain now-dropped columns.  While at it, move the responsibility
for acquring locks on relations referenced by rules into this separate
function (which I therefore chose to call AcquireRewriteLocks).
This saves effort --- namely, duplicated lock grabs in parser and rewriter
--- in the normal path at a cost of one extra non-locked heap_open()
in the stored-rule path; seems a good tradeoff.  A fringe benefit is
that it is now *much* clearer that we acquire lock on relations referenced
in rules before we make any rewriter decisions based on their properties.
(I don't know of any bug of that ilk, but it wasn't exactly clear before.)

src/backend/commands/explain.c

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994-5, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/explain.c,v 1.135 2005/04/25 01:30:12 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/explain.c,v 1.136 2005/06/03 23:05:28 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -100,6 +100,12 @@ ExplainQuery(ExplainStmt *stmt, DestReceiver *dest)
 	}
 	else
 	{
+		/*
+		 * Must acquire locks in case we didn't come fresh from the parser.
+		 * XXX this also scribbles on query, another reason for copyObject
+		 */
+		AcquireRewriteLocks(query);
+
 		/* Rewrite through rule system */
 		rewritten = QueryRewrite(query);
 
@@ -166,6 +172,8 @@ ExplainOneQuery(Query *query, ExplainStmt *stmt, TupOutputState *tstate)
 			cursorOptions = dcstmt->options;
 			/* Still need to rewrite cursor command */
 			Assert(query->commandType == CMD_SELECT);
+			/* get locks (we assume ExplainQuery already copied tree) */
+			AcquireRewriteLocks(query);
 			rewritten = QueryRewrite(query);
 			if (list_length(rewritten) != 1)
 				elog(ERROR, "unexpected rewrite result");

src/backend/commands/portalcmds.c

@@ -14,7 +14,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/portalcmds.c,v 1.41 2005/04/28 21:47:11 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/portalcmds.c,v 1.42 2005/06/03 23:05:28 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -76,6 +76,7 @@ PerformCursorOpen(DeclareCursorStmt *stmt, ParamListInfo params)
 	 * query, so we are not expecting rule rewriting to do anything
 	 * strange.
 	 */
+	AcquireRewriteLocks(query);
 	rewritten = QueryRewrite(query);
 	if (list_length(rewritten) != 1 || !IsA(linitial(rewritten), Query))
 		elog(ERROR, "unexpected rewrite result");

src/backend/commands/prepare.c

@@ -10,7 +10,7 @@
  * Copyright (c) 2002-2005, PostgreSQL Global Development Group
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/prepare.c,v 1.38 2005/05/29 04:23:03 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/prepare.c,v 1.39 2005/06/03 23:05:28 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -99,6 +99,7 @@ PrepareQuery(PrepareStmt *stmt)
 	query = copyObject(stmt->query);
 
 	/* Rewrite the query. The result could be 0, 1, or many queries. */
+	AcquireRewriteLocks(query);
 	query_list = QueryRewrite(query);
 
 	/* Generate plans for queries.	Snapshot is already set. */

src/backend/optimizer/util/var.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/util/var.c,v 1.63 2004/12/31 22:00:23 pgsql Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/util/var.c,v 1.64 2005/06/03 23:05:28 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -524,9 +524,7 @@ flatten_join_alias_vars_mutator(Node *node,
 				newvar = (Node *) lfirst(l);
 				attnum++;
 				/* Ignore dropped columns */
-				if (get_rte_attribute_is_dropped(context->root->rtable,
-												 var->varno,
-												 attnum))
+				if (IsA(newvar, Const))
 					continue;
 
 				/*

src/backend/parser/parse_relation.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/parser/parse_relation.c,v 1.108 2005/05/29 17:10:23 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/parser/parse_relation.c,v 1.109 2005/06/03 23:05:28 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1202,8 +1202,7 @@ addImplicitRTE(ParseState *pstate, RangeVar *relation)
  * results.  If include_dropped is TRUE then empty strings and NULL constants
  * (not Vars!) are returned for dropped columns.
  *
- * The target RTE is the rtindex'th entry of rtable.  (The whole rangetable
- * must be passed since we need it to determine dropped-ness for JOIN columns.)
+ * The target RTE is the rtindex'th entry of rtable.
  * sublevels_up is the varlevelsup value to use in the created Vars.
  *
  * The output lists go into *colnames and *colvars.
@@ -1358,6 +1357,8 @@ expandRTE(List *rtable, int rtindex, int sublevels_up,
 				varattno = 0;
 				forboth(colname, rte->eref->colnames, aliasvar, rte->joinaliasvars)
 				{
+					Node	   *avar = (Node *) lfirst(aliasvar);
+
 					varattno++;
 
 					/*
@@ -1365,26 +1366,19 @@ expandRTE(List *rtable, int rtindex, int sublevels_up,
 					 * deleted columns in the join; but we have to check
 					 * since this routine is also used by the rewriter,
 					 * and joins found in stored rules might have join
-					 * columns for since-deleted columns.
+					 * columns for since-deleted columns.  This will be
+					 * signaled by a NULL Const in the alias-vars list.
 					 */
-					if (get_rte_attribute_is_dropped(rtable, rtindex,
-													 varattno))
+					if (IsA(avar, Const))
 					{
 						if (include_dropped)
 						{
 							if (colnames)
 								*colnames = lappend(*colnames,
-												makeString(pstrdup("")));
+													makeString(pstrdup("")));
 							if (colvars)
-							{
-								/*
-								 * can't use atttypid here, but it doesn't
-								 * really matter what type the Const
-								 * claims to be.
-								 */
 								*colvars = lappend(*colvars,
-												 makeNullConst(INT4OID));
-							}
+												   copyObject(avar));
 						}
 						continue;
 					}
@@ -1399,7 +1393,6 @@ expandRTE(List *rtable, int rtindex, int sublevels_up,
 
 					if (colvars)
 					{
-						Node	   *avar = (Node *) lfirst(aliasvar);
 						Var		   *varnode;
 
 						varnode = makeVar(rtindex, varattno,
@@ -1711,9 +1704,8 @@ get_rte_attribute_type(RangeTblEntry *rte, AttrNumber attnum,
  *		Check whether attempted attribute ref is to a dropped column
  */
 bool
-get_rte_attribute_is_dropped(List *rtable, int rtindex, AttrNumber attnum)
+get_rte_attribute_is_dropped(RangeTblEntry *rte, AttrNumber attnum)
 {
-	RangeTblEntry *rte = rt_fetch(rtindex, rtable);
 	bool		result;
 
 	switch (rte->rtekind)
@@ -1750,8 +1742,8 @@ get_rte_attribute_is_dropped(List *rtable, int rtindex, AttrNumber attnum)
 				 * constructed, but one in a stored rule might contain
 				 * columns that were dropped from the underlying tables,
 				 * if said columns are nowhere explicitly referenced in
-				 * the rule.  So we have to recursively look at the
-				 * referenced column.
+				 * the rule.  This will be signaled to us by a NULL Const
+				 * in the joinaliasvars list.
 				 */
 				Var		   *aliasvar;
 
@@ -1760,18 +1752,7 @@ get_rte_attribute_is_dropped(List *rtable, int rtindex, AttrNumber attnum)
 					elog(ERROR, "invalid varattno %d", attnum);
 				aliasvar = (Var *) list_nth(rte->joinaliasvars, attnum - 1);
 
-				/*
-				 * If the list item isn't a simple Var, then it must
-				 * represent a merged column, ie a USING column, and so it
-				 * couldn't possibly be dropped (since it's referenced in
-				 * the join clause).
-				 */
-				if (!IsA(aliasvar, Var))
-					result = false;
-				else
-					result = get_rte_attribute_is_dropped(rtable,
-														  aliasvar->varno,
-													 aliasvar->varattno);
+				result = IsA(aliasvar, Const);
 			}
 			break;
 		case RTE_FUNCTION:

src/backend/tcop/postgres.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.446 2005/06/02 21:03:24 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.447 2005/06/03 23:05:29 tgl Exp $
  *
  * NOTES
  *	  this is the "main" module of the postgres backend and
@@ -158,6 +158,7 @@ static int	SocketBackend(StringInfo inBuf);
 static int	ReadCommand(StringInfo inBuf);
 static bool log_after_parse(List *raw_parsetree_list,
 				const char *query_string, char **prepare_string);
+static List *pg_rewrite_queries(List *querytree_list);
 static void start_xact_command(void);
 static void finish_xact_command(void);
 static void SigHupHandler(SIGNAL_ARGS);
@@ -642,8 +643,11 @@ pg_analyze_and_rewrite(Node *parsetree, Oid *paramTypes, int numParams)
 
 /*
  * Perform rewriting of a list of queries produced by parse analysis.
+ *
+ * Note: queries must just have come from the parser, because we do not do
+ * AcquireRewriteLocks() on them.
  */
-List *
+static List *
 pg_rewrite_queries(List *querytree_list)
 {
 	List	   *new_list = NIL;

src/backend/utils/adt/ruleutils.c

@@ -3,7 +3,7 @@
  *				back to source text
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/adt/ruleutils.c,v 1.198 2005/05/31 03:03:59 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/adt/ruleutils.c,v 1.199 2005/06/03 23:05:29 tgl Exp $
  *
  *	  This software is copyrighted by Jan Wieck - Hamburg.
  *
@@ -67,6 +67,7 @@
 #include "parser/parse_oper.h"
 #include "parser/parse_type.h"
 #include "parser/parsetree.h"
+#include "rewrite/rewriteHandler.h"
 #include "rewrite/rewriteManip.h"
 #include "rewrite/rewriteSupport.h"
 #include "utils/array.h"
@@ -1661,6 +1662,9 @@ 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);
+
 		context.buf = buf;
 		context.namespaces = list_make1(&dpns);
 		context.varprefix = (list_length(query->rtable) != 1);
@@ -1795,6 +1799,14 @@ get_query_def(Query *query, StringInfo buf, List *parentnamespace,
 	deparse_context context;
 	deparse_namespace dpns;
 
+	/*
+	 * Before we begin to examine the query, acquire locks on referenced
+	 * 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!
+	 */
+	AcquireRewriteLocks(query);
+
 	context.buf = buf;
 	context.namespaces = lcons(&dpns, list_copy(parentnamespace));
 	context.varprefix = (parentnamespace != NIL ||
@@ -4245,6 +4257,7 @@ get_from_clause_alias(Alias *alias, int varno,
 					  Query *query, deparse_context *context)
 {
 	StringInfo	buf = context->buf;
+	RangeTblEntry *rte = rt_fetch(varno, query->rtable);
 	ListCell   *col;
 	AttrNumber	attnum;
 	bool		first = true;
@@ -4256,7 +4269,7 @@ get_from_clause_alias(Alias *alias, int varno,
 	foreach(col, alias->colnames)
 	{
 		attnum++;
-		if (get_rte_attribute_is_dropped(query->rtable, varno, attnum))
+		if (get_rte_attribute_is_dropped(rte, attnum))
 			continue;
 		if (first)
 		{

src/include/nodes/parsenodes.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/nodes/parsenodes.h,v 1.278 2005/04/28 21:47:17 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/nodes/parsenodes.h,v 1.279 2005/06/03 23:05:29 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -465,7 +465,10 @@ typedef struct DefElem
  *	  those columns are known to be dropped at parse time.	Again, however,
  *	  a stored rule might contain entries for columns dropped since the rule
  *	  was created.	(This is only possible for columns not actually referenced
- *	  in the rule.)
+ *	  in the rule.)  When loading a stored rule, we replace the joinaliasvars
+ *	  items for any such columns with NULL Consts.  (We can't simply delete
+ *	  them from the joinaliasvars list, because that would affect the attnums
+ *	  of Vars referencing the rest of the list.)
  *
  *	  inh is TRUE for relation references that should be expanded to include
  *	  inheritance children, if the rel has any.  This *must* be FALSE for
@@ -535,7 +538,10 @@ typedef struct RangeTblEntry
 	 * to the columns of the join result.  An alias Var referencing column
 	 * K of the join result can be replaced by the K'th element of
 	 * joinaliasvars --- but to simplify the task of reverse-listing
-	 * aliases correctly, we do not do that until planning time.
+	 * aliases correctly, we do not do that until planning time.  In a Query
+	 * loaded from a stored rule, it is also possible for joinaliasvars
+	 * items to be NULL Consts, denoting columns dropped since the rule was
+	 * made.
 	 */
 	JoinType	jointype;		/* type of join */
 	List	   *joinaliasvars;	/* list of alias-var expansions */

src/include/parser/parsetree.h

@@ -8,7 +8,7 @@
  * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/parser/parsetree.h,v 1.29 2004/12/31 22:03:38 pgsql Exp $
+ * $PostgreSQL: pgsql/src/include/parser/parsetree.h,v 1.30 2005/06/03 23:05:30 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -59,8 +59,8 @@ extern void get_rte_attribute_type(RangeTblEntry *rte, AttrNumber attnum,
  * Check whether an attribute of an RTE has been dropped (note that
  * get_rte_attribute_type will fail on such an attr)
  */
-extern bool get_rte_attribute_is_dropped(List *rtable, int rtindex,
-							 AttrNumber attnum);
+extern bool get_rte_attribute_is_dropped(RangeTblEntry *rte,
+										 AttrNumber attnum);
 
 
 /* ----------------

src/include/rewrite/rewriteHandler.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/rewrite/rewriteHandler.h,v 1.24 2004/12/31 22:03:41 pgsql Exp $
+ * $PostgreSQL: pgsql/src/include/rewrite/rewriteHandler.h,v 1.25 2005/06/03 23:05:30 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -17,6 +17,7 @@
 #include "nodes/parsenodes.h"
 
 extern List *QueryRewrite(Query *parsetree);
+extern void AcquireRewriteLocks(Query *parsetree);
 extern Node *build_column_default(Relation rel, int attrno);
 
 #endif   /* REWRITEHANDLER_H */

src/include/tcop/tcopprot.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/tcop/tcopprot.h,v 1.74 2005/06/02 21:03:25 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/tcop/tcopprot.h,v 1.75 2005/06/03 23:05:30 tgl Exp $
  *
  * OLD COMMENTS
  *	  This file was created so that other c files could get the two
@@ -48,7 +48,6 @@ extern List *pg_parse_and_rewrite(const char *query_string,
 extern List *pg_parse_query(const char *query_string);
 extern List *pg_analyze_and_rewrite(Node *parsetree,
 					   Oid *paramTypes, int numParams);
-extern List *pg_rewrite_queries(List *querytree_list);
 extern Plan *pg_plan_query(Query *querytree, ParamListInfo boundParams);
 extern List *pg_plan_queries(List *querytrees, ParamListInfo boundParams,
 				bool needSnapshot);