Re: Bug with updateable Views and inherited tables? - Mailing list pgsql-general

=?ISO-8859-1?Q?Sebastian_B=F6ck?= <sebastianboeck@freenet.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,