Re: Bug with updateable Views and inherited tables?

=?ISO-8859-1?Q?Sebastian_B=F6ck?= <sebastianboeck(at)freenet(dot)de> writes:
> I investigated a little bit further and can be more precisely
> about the whole thing. This (wrong) behaviour only occurs, if
> the view has an order by clause.

The bug is triggered by the combination of an inherited UPDATE target
and an unflattenable sub-Query. I verified that it's been broken for
as long as we've had such features :-(.

I've applied the attached patch to 8.0. You could probably adapt it for
7.4, but I'm hesitant to put such a nontrivial change into a stable
branch without a lot more testing.

regards, tom lane

*** src/backend/optimizer/path/allpaths.c.orig Sun Aug 29 09:55:03 2004
--- src/backend/optimizer/path/allpaths.c Sat Oct 2 17:23:54 2004
***************
*** 124,131 ****
/* RangeFunction --- generate a separate plan for it */
set_function_pathlist(root, rel, rte);
}
! else if ((inheritlist = expand_inherited_rtentry(root, rti, true))
! != NIL)
{
/* Relation is root of an inheritance tree, process specially */
set_inherited_rel_pathlist(root, rel, rti, rte, inheritlist);
--- 124,130 ----
/* RangeFunction --- generate a separate plan for it */
set_function_pathlist(root, rel, rte);
}
! 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);
***************
*** 222,234 ****
ereport(ERROR,
(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
--- 221,226 ----
*** src/backend/optimizer/plan/planner.c.orig Sun Aug 29 22:57:31 2004
--- src/backend/optimizer/plan/planner.c Sat Oct 2 17:57:29 2004
***************
*** 320,327 ****
* grouping_planner.
*/
if (parse->resultRelation &&
! (lst = expand_inherited_rtentry(parse, parse->resultRelation,
! false)) != NIL)
plan = inheritance_planner(parse, lst);
else
plan = grouping_planner(parse, tuple_fraction);
--- 320,326 ----
* grouping_planner.
*/
if (parse->resultRelation &&
! (lst = expand_inherited_rtentry(parse, parse->resultRelation)) != NIL)
plan = inheritance_planner(parse, lst);
else
plan = grouping_planner(parse, tuple_fraction);
***************
*** 513,519 ****
{
int childRTindex = lfirst_int(l);
Oid childOID = getrelid(childRTindex, parse->rtable);
- int subrtlength;
Query *subquery;
Plan *subplan;

--- 512,517 ----
***************
*** 526,547 ****
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 is ugly. Really need to think about ways to
! * rein in planner's habit of scribbling on its input.
*/
! subrtlength = list_length(subquery->rtable);
! if (subrtlength > mainrtlength)
{
! List *subrt;

! subrt = list_copy_tail(subquery->rtable, mainrtlength);
! 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;
--- 524,563 ----
subplans = lappend(subplans, subplan);

/*
! * 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.
*
! * 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;
!
! subrt = list_copy_tail(subquery->rtable, mainrtlength);
! 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.orig Sun Aug 29 09:55:05 2004
--- src/backend/optimizer/prep/prepunion.c Sat Oct 2 17:24:22 2004
***************
*** 705,728 ****
* 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.
*
* 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?
*/
List *
! expand_inherited_rtentry(Query *parse, Index rti, bool dup_parent)
{
RangeTblEntry *rte = rt_fetch(rti, parse->rtable);
Oid parentOID;
--- 705,727 ----
* whole inheritance set (parent and children).
* If not, return NIL.
*
! * 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: 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)
{
RangeTblEntry *rte = rt_fetch(rti, parse->rtable);
Oid parentOID;
***************
*** 734,745 ****
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))
return NIL;
/* Scan for all members of inheritance set */
inhOIDs = find_all_inheritors(parentOID);

--- 733,746 ----
if (!rte->inh)
return NIL;
Assert(rte->rtekind == RTE_RELATION);
/* 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,784 ****
* table once had a child but no longer does.
*/
if (list_length(inhOIDs) < 2)
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.
*/
childrte = copyObject(rte);
childrte->relid = childOID;
parse->rtable = lappend(parse->rtable, childrte);
childRTindex = list_length(parse->rtable);
-
inhRTIs = lappend_int(inhRTIs, childRTindex);
}

return inhRTIs;
}
--- 750,790 ----
* 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 */
+ inhRTIs = NIL;
foreach(l, inhOIDs)
{
Oid childOID = lfirst_oid(l);
RangeTblEntry *childrte;
Index childRTindex;

/*
* 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 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.orig Sun Aug 29 09:55:06 2004
--- src/backend/optimizer/util/clauses.c Sat Oct 2 15:41:30 2004
***************
*** 3254,3263 ****
--- 3254,3273 ----
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.orig Sun Aug 29 09:56:05 2004
--- src/include/optimizer/prep.h Sat Oct 2 17:23:45 2004
***************
*** 52,59 ****

extern List *find_all_inheritors(Oid parentrel);

! extern List *expand_inherited_rtentry(Query *parse, Index rti,
! bool dup_parent);

extern Node *adjust_inherited_attrs(Node *node,
Index old_rt_index, Oid old_relid,
--- 52,58 ----

extern List *find_all_inheritors(Oid parentrel);

! extern List *expand_inherited_rtentry(Query *parse, Index rti);

extern Node *adjust_inherited_attrs(Node *node,
Index old_rt_index, Oid old_relid,