Modify the parser's error reporting to include a specific hint for the case

of referencing a WITH item that's not yet in scope according to the SQL
spec's semantics.  This seems to be an easy error to make, and the bare
"relation doesn't exist" message doesn't lead one's mind in the correct
direction to fix it.

Author: tglsfdc

src/backend/access/heap/heapam.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.264 2008/09/30 10:52:10 heikki Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.265 2008/10/08 01:14:44 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -1016,6 +1016,45 @@ relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
 	return relation_open(relOid, lockmode);
 }
 
+/* ----------------
+ *		try_relation_openrv - open any relation specified by a RangeVar
+ *
+ *		Same as relation_openrv, but return NULL instead of failing for
+ *		relation-not-found.  (Note that some other causes, such as
+ *		permissions problems, will still result in an ereport.)
+ * ----------------
+ */
+Relation
+try_relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
+{
+	Oid			relOid;
+
+	/*
+	 * Check for shared-cache-inval messages before trying to open the
+	 * relation.  This is needed to cover the case where the name identifies a
+	 * rel that has been dropped and recreated since the start of our
+	 * transaction: if we don't flush the old syscache entry then we'll latch
+	 * onto that entry and suffer an error when we do RelationIdGetRelation.
+	 * Note that relation_open does not need to do this, since a relation's
+	 * OID never changes.
+	 *
+	 * We skip this if asked for NoLock, on the assumption that the caller has
+	 * already ensured some appropriate lock is held.
+	 */
+	if (lockmode != NoLock)
+		AcceptInvalidationMessages();
+
+	/* Look up the appropriate relation using namespace search */
+	relOid = RangeVarGetRelid(relation, true);
+
+	/* Return NULL on not-found */
+	if (!OidIsValid(relOid))
+		return NULL;
+
+	/* Let relation_open do the rest */
+	return relation_open(relOid, lockmode);
+}
+
 /* ----------------
  *		relation_close - close any relation
  *
@@ -1097,6 +1136,37 @@ heap_openrv(const RangeVar *relation, LOCKMODE lockmode)
 	return r;
 }
 
+/* ----------------
+ *		try_heap_openrv - open a heap relation specified
+ *		by a RangeVar node
+ *
+ *		As above, but return NULL instead of failing for relation-not-found.
+ * ----------------
+ */
+Relation
+try_heap_openrv(const RangeVar *relation, LOCKMODE lockmode)
+{
+	Relation	r;
+
+	r = try_relation_openrv(relation, lockmode);
+
+	if (r)
+	{
+		if (r->rd_rel->relkind == RELKIND_INDEX)
+			ereport(ERROR,
+					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+					 errmsg("\"%s\" is an index",
+							RelationGetRelationName(r))));
+		else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
+			ereport(ERROR,
+					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+					 errmsg("\"%s\" is a composite type",
+							RelationGetRelationName(r))));
+	}
+
+	return r;
+}
+
 
 /* ----------------
  *		heap_beginscan	- begin relation scan

src/backend/parser/parse_cte.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/parser/parse_cte.c,v 2.3 2008/10/07 19:27:04 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/parser/parse_cte.c,v 2.4 2008/10/08 01:14:44 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -108,6 +108,7 @@ transformWithClause(ParseState *pstate, WithClause *withClause)
 
 	/* Only one WITH clause per query level */
 	Assert(pstate->p_ctenamespace == NIL);
+	Assert(pstate->p_future_ctes == NIL);
 
 	/*
 	 * For either type of WITH, there must not be duplicate CTE names in
@@ -216,14 +217,20 @@ transformWithClause(ParseState *pstate, WithClause *withClause)
 		/*
 		 * For non-recursive WITH, just analyze each CTE in sequence and then
 		 * add it to the ctenamespace.  This corresponds to the spec's
-		 * definition of the scope of each WITH name.
+		 * definition of the scope of each WITH name.  However, to allow
+		 * error reports to be aware of the possibility of an erroneous
+		 * reference, we maintain a list in p_future_ctes of the
+		 * not-yet-visible CTEs.
 		 */
+		pstate->p_future_ctes = list_copy(withClause->ctes);
+
 		foreach(lc, withClause->ctes)
 		{
 			CommonTableExpr *cte = (CommonTableExpr *) lfirst(lc);
 
 			analyzeCTE(pstate, cte);
 			pstate->p_ctenamespace = lappend(pstate->p_ctenamespace, cte);
+			pstate->p_future_ctes = list_delete_first(pstate->p_future_ctes);
 		}
  	}
 

src/backend/parser/parse_relation.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/parser/parse_relation.c,v 1.138 2008/10/06 15:15:22 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/parser/parse_relation.c,v 1.139 2008/10/08 01:14:44 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -214,6 +214,29 @@ scanNameSpaceForCTE(ParseState *pstate, const char *refname,
 	return NULL;
 }
 
+/*
+ * Search for a possible "future CTE", that is one that is not yet in scope
+ * according to the WITH scoping rules.  This has nothing to do with valid
+ * SQL semantics, but it's important for error reporting purposes.
+ */
+static bool
+isFutureCTE(ParseState *pstate, const char *refname)
+{
+	for (; pstate != NULL; pstate = pstate->parentParseState)
+	{
+		ListCell *lc;
+
+		foreach(lc, pstate->p_future_ctes)
+		{
+			CommonTableExpr *cte = (CommonTableExpr *) lfirst(lc);
+
+			if (strcmp(cte->ctename, refname) == 0)
+				return true;
+		}
+	}
+	return false;
+}
+
 /*
  * searchRangeTable
  *	  See if any RangeTblEntry could possibly match the RangeVar.
@@ -702,8 +725,9 @@ buildScalarFunctionAlias(Node *funcexpr, char *funcname,
 /*
  * Open a table during parse analysis
  *
- * This is essentially just the same as heap_openrv(), except that it
- * arranges to include the RangeVar's parse location in any resulting error.
+ * This is essentially just the same as heap_openrv(), except that it caters
+ * to some parser-specific error reporting needs, notably that it arranges
+ * to include the RangeVar's parse location in any resulting error.
  *
  * Note: properly, lockmode should be declared LOCKMODE not int, but that
  * would require importing storage/lock.h into parse_relation.h.  Since
@@ -716,7 +740,37 @@ parserOpenTable(ParseState *pstate, const RangeVar *relation, int lockmode)
 	ParseCallbackState pcbstate;
 
 	setup_parser_errposition_callback(&pcbstate, pstate, relation->location);
-	rel = heap_openrv(relation, lockmode);
+	rel = try_heap_openrv(relation, lockmode);
+	if (rel == NULL)
+	{
+		if (relation->schemaname)
+			ereport(ERROR,
+					(errcode(ERRCODE_UNDEFINED_TABLE),
+					 errmsg("relation \"%s.%s\" does not exist",
+							relation->schemaname, relation->relname)));
+		else
+		{
+			/*
+			 * An unqualified name might have been meant as a reference to
+			 * some not-yet-in-scope CTE.  The bare "does not exist" message
+			 * has proven remarkably unhelpful for figuring out such problems,
+			 * so we take pains to offer a specific hint.
+			 */
+			if (isFutureCTE(pstate, relation->relname))
+				ereport(ERROR,
+						(errcode(ERRCODE_UNDEFINED_TABLE),
+						 errmsg("relation \"%s\" does not exist",
+								relation->relname),
+						 errdetail("There is a WITH item named \"%s\", but it cannot be referenced from this part of the query.",
+								   relation->relname),
+						 errhint("Use WITH RECURSIVE, or re-order the WITH items to remove forward references.")));
+			else
+				ereport(ERROR,
+						(errcode(ERRCODE_UNDEFINED_TABLE),
+						 errmsg("relation \"%s\" does not exist",
+								relation->relname)));
+		}
+	}
 	cancel_parser_errposition_callback(&pcbstate);
 	return rel;
 }

src/include/access/heapam.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/access/heapam.h,v 1.138 2008/08/11 11:05:11 heikki Exp $
+ * $PostgreSQL: pgsql/src/include/access/heapam.h,v 1.139 2008/10/08 01:14:44 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -45,10 +45,12 @@ extern Relation relation_open(Oid relationId, LOCKMODE lockmode);
 extern Relation try_relation_open(Oid relationId, LOCKMODE lockmode);
 extern Relation relation_open_nowait(Oid relationId, LOCKMODE lockmode);
 extern Relation relation_openrv(const RangeVar *relation, LOCKMODE lockmode);
+extern Relation try_relation_openrv(const RangeVar *relation, LOCKMODE lockmode);
 extern void relation_close(Relation relation, LOCKMODE lockmode);
 
 extern Relation heap_open(Oid relationId, LOCKMODE lockmode);
 extern Relation heap_openrv(const RangeVar *relation, LOCKMODE lockmode);
+extern Relation try_heap_openrv(const RangeVar *relation, LOCKMODE lockmode);
 
 #define heap_close(r,l)  relation_close(r,l)
 

src/include/parser/parse_node.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/parser/parse_node.h,v 1.57 2008/10/04 21:56:55 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/parser/parse_node.h,v 1.58 2008/10/08 01:14:44 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -54,6 +54,9 @@
  * at the moment.  This is different from p_relnamespace because you have
  * to make an RTE before you can access a CTE.
  *
+ * p_future_ctes: list of CommonTableExprs (WITH items) that are not yet
+ * visible due to scope rules.  This is used to help improve error messages.
+ *
  * p_paramtypes: an array of p_numparams type OIDs for $n parameter symbols
  * (zeroth entry in array corresponds to $1).  If p_variableparams is true, the
  * set of param types is not predetermined; in that case, a zero array entry
@@ -73,6 +76,7 @@ typedef struct ParseState
 	List	   *p_relnamespace; /* current namespace for relations */
 	List	   *p_varnamespace; /* current namespace for columns */
 	List	   *p_ctenamespace; /* current namespace for common table exprs */
+	List	   *p_future_ctes;	/* common table exprs not yet in namespace */
 	Oid		   *p_paramtypes;	/* OIDs of types for $n parameter symbols */
 	int			p_numparams;	/* allocated size of p_paramtypes[] */
 	int			p_next_resno;	/* next targetlist resno to assign */