Clean up handling of inherited-table update queries, per bug report

from Sebastian Böck.  The fix involves being more consistent about
when rangetable entries are copied or modified.  Someday we really
need to fix this stuff to not scribble on its input data structures
in the first place...

src/backend/optimizer/path/allpaths.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/path/allpaths.c,v 1.121 2004/08/29 05:06:43 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/path/allpaths.c,v 1.122 2004/10/02 22:39:45 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -124,8 +124,7 @@ set_base_rel_pathlists(Query *root)
 			/* RangeFunction --- generate a separate plan for it */
 			set_function_pathlist(root, rel, rte);
 		}
-		else if ((inheritlist = expand_inherited_rtentry(root, rti, true))
-				 != NIL)
+		else if ((inheritlist = expand_inherited_rtentry(root, rti)) != NIL)
 		{
 			/* Relation is root of an inheritance tree, process specially */
 			set_inherited_rel_pathlist(root, rel, rti, rte, inheritlist);
@@ -223,13 +222,6 @@ set_inherited_rel_pathlist(Query *root, RelOptInfo *rel,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("SELECT FOR UPDATE is not supported for inheritance queries")));
 
-	/*
-	 * The executor will check the parent table's access permissions when
-	 * it examines the parent's inheritlist entry.  There's no need to
-	 * check twice, so turn off access check bits in the original RTE.
-	 */
-	rte->requiredPerms = 0;
-
 	/*
 	 * Initialize to compute size estimates for whole inheritance tree
 	 */

src/backend/optimizer/plan/planner.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/plan/planner.c,v 1.175 2004/08/30 02:54:38 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/plan/planner.c,v 1.176 2004/10/02 22:39:46 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -320,8 +320,7 @@ subquery_planner(Query *parse, double tuple_fraction)
 	 * grouping_planner.
 	 */
 	if (parse->resultRelation &&
-		(lst = expand_inherited_rtentry(parse, parse->resultRelation,
-										false)) != NIL)
+		(lst = expand_inherited_rtentry(parse, parse->resultRelation)) != NIL)
 		plan = inheritance_planner(parse, lst);
 	else
 		plan = grouping_planner(parse, tuple_fraction);
@@ -513,7 +512,6 @@ inheritance_planner(Query *parse, List *inheritlist)
 	{
 		int			childRTindex = lfirst_int(l);
 		Oid			childOID = getrelid(childRTindex, parse->rtable);
-		int			subrtlength;
 		Query	   *subquery;
 		Plan	   *subplan;
 
@@ -526,14 +524,30 @@ inheritance_planner(Query *parse, List *inheritlist)
 		subplans = lappend(subplans, subplan);
 
 		/*
-		 * It's possible that additional RTEs got added to the rangetable
-		 * due to expansion of inherited source tables (see allpaths.c).
-		 * If so, we must copy 'em back to the main parse tree's rtable.
+		 * XXX my goodness this next bit is ugly.  Really need to think about
+		 * ways to rein in planner's habit of scribbling on its input.
 		 *
-		 * XXX my goodness this is ugly.  Really need to think about ways to
-		 * rein in planner's habit of scribbling on its input.
-		 */
-		subrtlength = list_length(subquery->rtable);
+		 * Planning of the subquery might have modified the rangetable,
+		 * either by addition of RTEs due to expansion of inherited source
+		 * tables, or by changes of the Query structures inside subquery
+		 * RTEs.  We have to ensure that this gets propagated back to the
+		 * master copy.  However, if we aren't done planning yet, we also
+		 * need to ensure that subsequent calls to grouping_planner have
+		 * virgin sub-Queries to work from.  So, if we are at the last
+		 * list entry, just copy the subquery rangetable back to the master
+		 * copy; if we are not, then extend the master copy by adding
+		 * whatever the subquery added.  (We assume these added entries
+		 * will go untouched by the future grouping_planner calls.  We are
+		 * also effectively assuming that sub-Queries will get planned
+		 * identically each time, or at least that the impacts on their
+		 * rangetables will be the same each time.  Did I say this is ugly?)
+		 */
+		if (lnext(l) == NULL)
+			parse->rtable = subquery->rtable;
+		else
+		{
+			int		subrtlength = list_length(subquery->rtable);
+
 			if (subrtlength > mainrtlength)
 			{
 				List	   *subrt;
@@ -542,6 +556,8 @@ inheritance_planner(Query *parse, List *inheritlist)
 				parse->rtable = list_concat(parse->rtable, subrt);
 				mainrtlength = subrtlength;
 			}
+		}
+
 		/* Save preprocessed tlist from first rel for use in Append */
 		if (tlist == NIL)
 			tlist = subplan->targetlist;

src/backend/optimizer/prep/prepunion.c

@@ -14,7 +14,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/prep/prepunion.c,v 1.116 2004/08/29 05:06:44 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/prep/prepunion.c,v 1.117 2004/10/02 22:39:47 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -705,24 +705,23 @@ find_all_inheritors(Oid parentrel)
  *		whole inheritance set (parent and children).
  *		If not, return NIL.
  *
- * When dup_parent is false, the initially given RT index is part of the
- * returned list (if any).	When dup_parent is true, the given RT index
- * is *not* in the returned list; a duplicate RTE will be made for the
- * parent table.
+ * Note that the original RTE is considered to represent the whole
+ * inheritance set.  The first member of the returned list is an RTE
+ * for the same table, but with inh = false, to represent the parent table
+ * in its role as a simple member of the set.  The original RT index is
+ * never a member of the returned list.
  *
  * A childless table is never considered to be an inheritance set; therefore
  * the result will never be a one-element list.  It'll be either empty
  * or have two or more elements.
  *
- * NOTE: after this routine executes, the specified RTE will always have
- * its inh flag cleared, whether or not there were any children.  This
- * ensures we won't expand the same RTE twice, which would otherwise occur
- * for the case of an inherited UPDATE/DELETE target relation.
- *
- * XXX probably should convert the result type to Relids?
+ * Note: there are cases in which this routine will be invoked multiple
+ * times on the same RTE.  We will generate a separate set of child RTEs
+ * for each invocation.  This is somewhat wasteful but seems not worth
+ * trying to avoid.
  */
 List *
-expand_inherited_rtentry(Query *parse, Index rti, bool dup_parent)
+expand_inherited_rtentry(Query *parse, Index rti)
 {
 	RangeTblEntry *rte = rt_fetch(rti, parse->rtable);
 	Oid			parentOID;
@@ -734,12 +733,14 @@ expand_inherited_rtentry(Query *parse, Index rti, bool dup_parent)
 	if (!rte->inh)
 		return NIL;
 	Assert(rte->rtekind == RTE_RELATION);
-	/* Always clear the parent's inh flag, see above comments */
-	rte->inh = false;
 	/* Fast path for common case of childless table */
 	parentOID = rte->relid;
 	if (!has_subclass(parentOID))
+	{
+		/* Clear flag to save repeated tests if called again */
+		rte->inh = false;
 		return NIL;
+	}
 	/* Scan for all members of inheritance set */
 	inhOIDs = find_all_inheritors(parentOID);
 
@@ -749,37 +750,42 @@ expand_inherited_rtentry(Query *parse, Index rti, bool dup_parent)
 	 * table once had a child but no longer does.
 	 */
 	if (list_length(inhOIDs) < 2)
+	{
+		/* Clear flag to save repeated tests if called again */
+		rte->inh = false;
 		return NIL;
+	}
+
 	/* OK, it's an inheritance set; expand it */
-	if (dup_parent)
 	inhRTIs = NIL;
-	else
-		inhRTIs = list_make1_int(rti);	/* include original RTE in result */
-
 	foreach(l, inhOIDs)
 	{
 		Oid			childOID = lfirst_oid(l);
 		RangeTblEntry *childrte;
 		Index		childRTindex;
 
-		/* parent will be in the list too; skip it if not dup requested */
-		if (childOID == parentOID && !dup_parent)
-			continue;
-
 		/*
 		 * Build an RTE for the child, and attach to query's rangetable
 		 * list. We copy most fields of the parent's RTE, but replace
-		 * relation real name and OID.	Note that inh will be false at
-		 * this point.
+		 * relation OID, and set inh = false.
 		 */
 		childrte = copyObject(rte);
 		childrte->relid = childOID;
+		childrte->inh = false;
 		parse->rtable = lappend(parse->rtable, childrte);
 		childRTindex = list_length(parse->rtable);
-
 		inhRTIs = lappend_int(inhRTIs, childRTindex);
 	}
 
+	/*
+	 * The executor will check the parent table's access permissions when
+	 * it examines the parent's inheritlist entry.  There's no need to
+	 * check twice, so turn off access check bits in the original RTE.
+	 * (If we are invoked more than once, extra copies of the child RTEs
+	 * will also not cause duplicate permission checks.)
+	 */
+	rte->requiredPerms = 0;
+
 	return inhRTIs;
 }
 

src/backend/optimizer/util/clauses.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/util/clauses.c,v 1.180 2004/08/29 05:06:44 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/util/clauses.c,v 1.181 2004/10/02 22:39:48 tgl Exp $
  *
  * HISTORY
  *	  AUTHOR			DATE			MAJOR EVENT
@@ -3254,10 +3254,20 @@ query_tree_mutator(Query *query,
 					CHECKFLATCOPY(newrte->subquery, rte->subquery, Query);
 					MUTATE(newrte->subquery, newrte->subquery, Query *);
 				}
+				else
+				{
+					/* else, copy RT subqueries as-is */
+					newrte->subquery = copyObject(rte->subquery);
+				}
 				break;
 			case RTE_JOIN:
 				if (!(flags & QTW_IGNORE_JOINALIASES))
 					MUTATE(newrte->joinaliasvars, rte->joinaliasvars, List *);
+				else
+				{
+					/* else, copy join aliases as-is */
+					newrte->joinaliasvars = copyObject(rte->joinaliasvars);
+				}
 				break;
 			case RTE_FUNCTION:
 				MUTATE(newrte->funcexpr, rte->funcexpr, Node *);

src/include/optimizer/prep.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2004, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/optimizer/prep.h,v 1.45 2004/08/29 04:13:08 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/optimizer/prep.h,v 1.46 2004/10/02 22:39:49 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -52,8 +52,7 @@ extern Plan *plan_set_operations(Query *parse, List **sortClauses);
 
 extern List *find_all_inheritors(Oid parentrel);
 
-extern List *expand_inherited_rtentry(Query *parse, Index rti,
-						 bool dup_parent);
+extern List *expand_inherited_rtentry(Query *parse, Index rti);
 
 extern Node *adjust_inherited_attrs(Node *node,
 					   Index old_rt_index, Oid old_relid,