Обсуждение: BUG #4350: 'select' acess given to views containing "union all" even though user has no grants

The following bug has been logged online:

Bug reference:      4350
Logged by:          Brendan O'Shea
Email address:      boshea@akamai.com
PostgreSQL version: 8.2.9
Operating system:   linux-2.4 and windows XP
Description:        'select' acess given to views containing "union all"
even though user has no grants
Details:

There appears to be a bug in the way that permissions are determined for
views that contain "UNION ALL" in their definition.

There is a simple test case to reproduce the bug.

1) As a superuser create the following objects:

CREATE ROLE test_perm LOGIN PASSWORD 'test_perm';

CREATE OR REPLACE VIEW public.simple_select AS SELECT 1;
CREATE OR REPLACE VIEW public.union_all AS SELECT 1 UNION ALL SELECT 2;


2) Now log in as the test_perm user and run the following SQL:

select * from public.simple_select;
select * from public.union_all;

The first SQL statement correctly produces an error, but the second
statement will return results with no error, it should instead generate a
permission error.

Brendan O'Shea wrote:
> There appears to be a bug in the way that permissions are determined for
> views that contain "UNION ALL" in their definition.
>
> There is a simple test case to reproduce the bug.
>
> 1) As a superuser create the following objects:
>
> CREATE ROLE test_perm LOGIN PASSWORD 'test_perm';
>
> CREATE OR REPLACE VIEW public.simple_select AS SELECT 1;
> CREATE OR REPLACE VIEW public.union_all AS SELECT 1 UNION ALL SELECT 2;
>
>
> 2) Now log in as the test_perm user and run the following SQL:
>
> select * from public.simple_select;
> select * from public.union_all;
>
> The first SQL statement correctly produces an error, but the second
> statement will return results with no error, it should instead generate a
> permission error.

Hmm, looks like pull_up_subqueries somehow loses the range table entry
referring the original view. It's reproducible on PG version 8.2 and
higher, 8.1 seems to work. I'll dig deeper tomorrow, unless someone else
beats me to it.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas wrote:
> Brendan O'Shea wrote:
>> There appears to be a bug in the way that permissions are determined for
>> views that contain "UNION ALL" in their definition.
>> There is a simple test case to reproduce the bug.
>>
>> 1) As a superuser create the following objects:
>>
>> CREATE ROLE test_perm LOGIN PASSWORD 'test_perm';
>>
>> CREATE OR REPLACE VIEW public.simple_select AS SELECT 1;
>> CREATE OR REPLACE VIEW public.union_all AS SELECT 1 UNION ALL SELECT 2;
>>
>>
>> 2) Now log in as the test_perm user and run the following SQL:
>>
>> select * from public.simple_select;
>> select * from public.union_all;
>>
>> The first SQL statement correctly produces an error, but the second
>> statement will return results with no error, it should instead generate a
>> permission error.
>
> Hmm, looks like pull_up_subqueries somehow loses the range table entry
> referring the original view. It's reproducible on PG version 8.2 and
> higher, 8.1 seems to work. I'll dig deeper tomorrow, unless someone else
> beats me to it.

Yep, pull_up_simple_union_all neglects to copy those range table
references that are not directly referenced in any of the UNION ALL
subqueries.

Attached is a patch for this. I have to go to sleep now, but will commit
this tomorrow unless someone comes up with a better idea. One thing that
I'm not totally happy about is that I'm using list_union, which uses
O(n^2) algorithm.

Thanks for the report!

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com
Index: src/backend/optimizer/prep/prepjointree.c
===================================================================
RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/optimizer/prep/prepjointree.c,v
retrieving revision 1.44
diff -c -r1.44 prepjointree.c
*** src/backend/optimizer/prep/prepjointree.c    4 Oct 2006 00:29:54 -0000    1.44
--- src/backend/optimizer/prep/prepjointree.c    11 Aug 2008 19:54:30 -0000
***************
*** 487,492 ****
--- 487,500 ----
      pull_up_union_leaf_queries(subquery->setOperations, root, varno, subquery);

      /*
+      * pull_up_union_leaf_queries copied those range table entries that were
+      * referenced from one of the UNION ALL subqueries, but we need to make
+      * sure that all non-referenced ones are copied as well. They are needed
+      * for permission checks during executor startup (ExecCheckRTPerms).
+      */
+     root->parse->rtable = list_union(root->parse->rtable, subquery->rtable);
+
+     /*
       * Mark the parent as an append relation.
       */
      rte->inh = true;

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:
> +     root->parse->rtable = list_union(root->parse->rtable, subquery->rtable);

That's one heck of a scary patch: nowhere in list_union's API is there
any guarantee that it preserves list ordering, but we *must not* change
the positions of the existing rtable entries.

I think it might be better to fix the problem in
pull_up_union_leaf_queries instead; given that it wasn't broken till
recently, I think it's arguably that function's fault.  Can we redesign
it to pull up everything in the subquery rtable, not just what was
referenced?

            regards, tom lane

I wrote:
> That's one heck of a scary patch: nowhere in list_union's API is there
> any guarantee that it preserves list ordering, but we *must not* change
> the positions of the existing rtable entries.

Actually there's a more fundamental problem, namely that pulled-up
subqueries aren't necessarily equal() to the originals.  They will
definitely be different if there were any uplevel Var references.

While you could argue that it doesn't matter because we'll only
end up redundantly checking permissions on multiple copies of the
RTEs, that's a bit beyond my threshold of ugliness...

            regards, tom lane

Tom Lane wrote:
> I wrote:
>> That's one heck of a scary patch: nowhere in list_union's API is there
>> any guarantee that it preserves list ordering, but we *must not* change
>> the positions of the existing rtable entries.

Good point.

> Actually there's a more fundamental problem, namely that pulled-up
> subqueries aren't necessarily equal() to the originals.  They will
> definitely be different if there were any uplevel Var references.

Another good point. I just saw that they're copied with copyObject and
are thus equal, but missed the IncrementVarSublevelsUp call.

> While you could argue that it doesn't matter because we'll only
> end up redundantly checking permissions on multiple copies of the
> RTEs, that's a bit beyond my threshold of ugliness...

Yeah, that wasn't the intention.

Attached is a patch with slightly different approach. Instead of
list_union, I'm keeping track of which rtes are copied by
pull_up_union_leaf_queries in a bitmapset.

BTW, I wonder if it's possible to end up with multiple copies of the
same RTE anyway, if there's multiple references to the same RTE. I'm
guessing that it's either not possible or we don't care, because that
can happen without the patch just as well.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com
Index: src/backend/optimizer/prep/prepjointree.c
===================================================================
RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/optimizer/prep/prepjointree.c,v
retrieving revision 1.44
diff -c -r1.44 prepjointree.c
*** src/backend/optimizer/prep/prepjointree.c    4 Oct 2006 00:29:54 -0000    1.44
--- src/backend/optimizer/prep/prepjointree.c    12 Aug 2008 07:39:30 -0000
***************
*** 46,52 ****
  static Node *pull_up_simple_union_all(PlannerInfo *root, Node *jtnode,
                           RangeTblEntry *rte);
  static void pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root,
!                            int parentRTindex, Query *setOpQuery);
  static void make_setop_translation_lists(Query *query,
                               Index newvarno,
                               List **col_mappings, List **translated_vars);
--- 46,52 ----
  static Node *pull_up_simple_union_all(PlannerInfo *root, Node *jtnode,
                           RangeTblEntry *rte);
  static void pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root,
!                int parentRTindex, Query *setOpQuery, Bitmapset **pulledUpRtes);
  static void make_setop_translation_lists(Query *query,
                               Index newvarno,
                               List **col_mappings, List **translated_vars);
***************
*** 477,482 ****
--- 477,485 ----
  {
      int            varno = ((RangeTblRef *) jtnode)->rtindex;
      Query       *subquery = rte->subquery;
+     Bitmapset  *pulledUpRtes = NULL;
+     ListCell   *l;
+     int            i;

      /*
       * Recursively scan the subquery's setOperations tree and copy the leaf
***************
*** 484,490 ****
       * them to the parent's append_rel_list, too.
       */
      Assert(subquery->setOperations);
!     pull_up_union_leaf_queries(subquery->setOperations, root, varno, subquery);

      /*
       * Mark the parent as an append relation.
--- 487,512 ----
       * them to the parent's append_rel_list, too.
       */
      Assert(subquery->setOperations);
!     pull_up_union_leaf_queries(subquery->setOperations, root, varno, subquery,
!                                &pulledUpRtes);
!
!     /*
!      * pull_up_union_leaf_queries already copied those range table entries
!      * from the subqueries to parent that were referenced in the subqueries,
!      * but we need to make sure that all non-referenced ones are copied as
!      * well. They are needed for permission checks during executor startup
!      * (ExecCheckRTPerms).
!      */
!     i = 1;
!     foreach(l, subquery->rtable)
!     {
!         if (!bms_is_member(i, pulledUpRtes))
!         {
!             RangeTblEntry *rte = rt_fetch(i, subquery->rtable);
!             root->parse->rtable = lappend(root->parse->rtable, rte);
!         }
!         i++;
!     }

      /*
       * Mark the parent as an append relation.
***************
*** 501,510 ****
   * is where to look up the RTE if setOp is a RangeTblRef.  This is *not* the
   * same as root->parse, which is the top-level Query we are pulling up into.
   * parentRTindex is the appendrel parent's index in root->parse->rtable.
   */
  static void
  pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root, int parentRTindex,
!                            Query *setOpQuery)
  {
      if (IsA(setOp, RangeTblRef))
      {
--- 523,534 ----
   * is where to look up the RTE if setOp is a RangeTblRef.  This is *not* the
   * same as root->parse, which is the top-level Query we are pulling up into.
   * parentRTindex is the appendrel parent's index in root->parse->rtable.
+  * pulledUpRtes is a bitmap of those range table entries in setOpQuery that
+  * have already been copied to the parent
   */
  static void
  pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root, int parentRTindex,
!                            Query *setOpQuery, Bitmapset **pulledUpRtes)
  {
      if (IsA(setOp, RangeTblRef))
      {
***************
*** 535,540 ****
--- 559,565 ----
           */
          parse->rtable = lappend(parse->rtable, rte);
          childRTindex = list_length(parse->rtable);
+         *pulledUpRtes = bms_add_member(*pulledUpRtes, rtr->rtindex);

          /*
           * Build a suitable AppendRelInfo, and attach to parent's list.
***************
*** 566,573 ****
          SetOperationStmt *op = (SetOperationStmt *) setOp;

          /* Recurse to reach leaf queries */
!         pull_up_union_leaf_queries(op->larg, root, parentRTindex, setOpQuery);
!         pull_up_union_leaf_queries(op->rarg, root, parentRTindex, setOpQuery);
      }
      else
      {
--- 591,600 ----
          SetOperationStmt *op = (SetOperationStmt *) setOp;

          /* Recurse to reach leaf queries */
!         pull_up_union_leaf_queries(op->larg, root, parentRTindex, setOpQuery,
!                                    pulledUpRtes);
!         pull_up_union_leaf_queries(op->rarg, root, parentRTindex, setOpQuery,
!                                    pulledUpRtes);
      }
      else
      {

Tom Lane wrote:
> I think it might be better to fix the problem in
> pull_up_union_leaf_queries instead; given that it wasn't broken till
> recently, I think it's arguably that function's fault.

Not sure what you mean. pull_up_union_leaf_queries was introduced at the
same time as the rest of the logic to pull up UNION ALL subqueries, in
8.2, and has been broken ever since.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:
> Tom Lane wrote:
>> I think it might be better to fix the problem in
>> pull_up_union_leaf_queries instead; given that it wasn't broken till
>> recently, I think it's arguably that function's fault.

> Not sure what you mean. pull_up_union_leaf_queries was introduced at the
> same time as the rest of the logic to pull up UNION ALL subqueries, in
> 8.2, and has been broken ever since.

I was defining 8.2 as "recent" ;-)

Seriously, I think what this shows is that piecemeal pullup is wrong in
principle, and that the right approach is always to concat the
subquery's rtable in toto to the upper level, and then go from there on
adjusting varnos.  Do you want to look into that approach?

            regards, tom lane

Tom Lane wrote:
> Seriously, I think what this shows is that piecemeal pullup is wrong in
> principle, and that the right approach is always to concat the
> subquery's rtable in toto to the upper level, and then go from there on
> adjusting varnos.  Do you want to look into that approach?

You mean like pull_up_simple_subquery() does? Yeah, I can try that. This
isn't really something I'm familiar with, but it's great exercise :-).

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas wrote:
> Tom Lane wrote:
>> Seriously, I think what this shows is that piecemeal pullup is wrong in
>> principle, and that the right approach is always to concat the
>> subquery's rtable in toto to the upper level, and then go from there on
>> adjusting varnos.  Do you want to look into that approach?
>
> You mean like pull_up_simple_subquery() does? Yeah, I can try that. This
> isn't really something I'm familiar with, but it's great exercise :-).

Here we go. Now that I see it, I do like this approach better.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com
Index: src/backend/optimizer/prep/prepjointree.c
===================================================================
RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/optimizer/prep/prepjointree.c,v
retrieving revision 1.44
diff -c -r1.44 prepjointree.c
*** src/backend/optimizer/prep/prepjointree.c    4 Oct 2006 00:29:54 -0000    1.44
--- src/backend/optimizer/prep/prepjointree.c    12 Aug 2008 18:22:28 -0000
***************
*** 46,52 ****
  static Node *pull_up_simple_union_all(PlannerInfo *root, Node *jtnode,
                           RangeTblEntry *rte);
  static void pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root,
!                            int parentRTindex, Query *setOpQuery);
  static void make_setop_translation_lists(Query *query,
                               Index newvarno,
                               List **col_mappings, List **translated_vars);
--- 46,52 ----
  static Node *pull_up_simple_union_all(PlannerInfo *root, Node *jtnode,
                           RangeTblEntry *rte);
  static void pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root,
!                int parentRTindex, Query *setOpQuery, int childRToffset);
  static void make_setop_translation_lists(Query *query,
                               Index newvarno,
                               List **col_mappings, List **translated_vars);
***************
*** 477,490 ****
  {
      int            varno = ((RangeTblRef *) jtnode)->rtindex;
      Query       *subquery = rte->subquery;

      /*
!      * Recursively scan the subquery's setOperations tree and copy the leaf
!      * subqueries into the parent rangetable.  Add AppendRelInfo nodes for
!      * them to the parent's append_rel_list, too.
       */
      Assert(subquery->setOperations);
!     pull_up_union_leaf_queries(subquery->setOperations, root, varno, subquery);

      /*
       * Mark the parent as an append relation.
--- 477,521 ----
  {
      int            varno = ((RangeTblRef *) jtnode)->rtindex;
      Query       *subquery = rte->subquery;
+     ListCell   *l;
+     int            rtoffset;

      /*
!      * Append the subquery rtable entries to upper query.
!      */
!     rtoffset = list_length(root->parse->rtable);
!     foreach(l, subquery->rtable)
!     {
!         RangeTblEntry *rte = (RangeTblEntry *) lfirst(l);
!
!         /*
!          * Make a modifiable copy of the child RTE and contained query.
!          */
!         rte = copyObject(rte);
!         Assert(rte->subquery != NULL);
!
!         /*
!          * Upper-level vars in subquery are now one level closer to their
!          * parent than before.    We don't have to worry about offsetting
!          * varnos, though, because any such vars must refer to stuff above the
!          * level of the query we are pulling into.
!          */
!         IncrementVarSublevelsUp((Node *) rte->subquery, -1, 1);
!
!         /*
!          * Attach child RTE to parent rtable.
!          */
!         root->parse->rtable = lappend(root->parse->rtable, rte);
!     }
!
!     /*
!      * Recursively scan the subquery's setOperations tree and add
!      * AppendRelInfo nodes for leaf subqueries to the parent's
!      * append_rel_list.
       */
      Assert(subquery->setOperations);
!     pull_up_union_leaf_queries(subquery->setOperations, root, varno, subquery,
!                                rtoffset);

      /*
       * Mark the parent as an append relation.
***************
*** 501,540 ****
   * is where to look up the RTE if setOp is a RangeTblRef.  This is *not* the
   * same as root->parse, which is the top-level Query we are pulling up into.
   * parentRTindex is the appendrel parent's index in root->parse->rtable.
   */
  static void
  pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root, int parentRTindex,
!                            Query *setOpQuery)
  {
      if (IsA(setOp, RangeTblRef))
      {
          RangeTblRef *rtr = (RangeTblRef *) setOp;
-         RangeTblEntry *rte = rt_fetch(rtr->rtindex, setOpQuery->rtable);
-         Query       *subquery;
          int            childRTindex;
          AppendRelInfo *appinfo;
-         Query       *parse = root->parse;

          /*
!          * Make a modifiable copy of the child RTE and contained query.
!          */
!         rte = copyObject(rte);
!         subquery = rte->subquery;
!         Assert(subquery != NULL);
!
!         /*
!          * Upper-level vars in subquery are now one level closer to their
!          * parent than before.    We don't have to worry about offsetting
!          * varnos, though, because any such vars must refer to stuff above the
!          * level of the query we are pulling into.
!          */
!         IncrementVarSublevelsUp((Node *) subquery, -1, 1);
!
!         /*
!          * Attach child RTE to parent rtable.
           */
!         parse->rtable = lappend(parse->rtable, rte);
!         childRTindex = list_length(parse->rtable);

          /*
           * Build a suitable AppendRelInfo, and attach to parent's list.
--- 532,555 ----
   * is where to look up the RTE if setOp is a RangeTblRef.  This is *not* the
   * same as root->parse, which is the top-level Query we are pulling up into.
   * parentRTindex is the appendrel parent's index in root->parse->rtable.
+  *
+  * The child RTEs have already been copied to the parent. childRToffset
+  * tells us where in the parent's range table they were copied.
   */
  static void
  pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root, int parentRTindex,
!                            Query *setOpQuery, int childRToffset)
  {
      if (IsA(setOp, RangeTblRef))
      {
          RangeTblRef *rtr = (RangeTblRef *) setOp;
          int            childRTindex;
          AppendRelInfo *appinfo;

          /*
!          * Calculate the index in the parent's range table
           */
!         childRTindex = childRToffset + rtr->rtindex;

          /*
           * Build a suitable AppendRelInfo, and attach to parent's list.
***************
*** 566,573 ****
          SetOperationStmt *op = (SetOperationStmt *) setOp;

          /* Recurse to reach leaf queries */
!         pull_up_union_leaf_queries(op->larg, root, parentRTindex, setOpQuery);
!         pull_up_union_leaf_queries(op->rarg, root, parentRTindex, setOpQuery);
      }
      else
      {
--- 581,590 ----
          SetOperationStmt *op = (SetOperationStmt *) setOp;

          /* Recurse to reach leaf queries */
!         pull_up_union_leaf_queries(op->larg, root, parentRTindex, setOpQuery,
!                                    childRToffset);
!         pull_up_union_leaf_queries(op->rarg, root, parentRTindex, setOpQuery,
!                                    childRToffset);
      }
      else
      {

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:
> Here we go. Now that I see it, I do like this approach better.

Hm, the "Assert(rte->subquery != NULL)" doesn't seem right ...
couldn't there be non-RTE_SUBQUERY rtes in the child?  I think the
original coding was guaranteed to visit only subquery-type RTEs
but I'm much less convinced about this one.  It might
be better to say
    if (rte->rtekind == RTE_SUBQUERY)
        IncrementVarSublevelsUp(...);

Or maybe it's okay; I'm too lazy to recheck the representation of
UNION ALL right now.

            regards, tom lane

Tom Lane wrote:
> Hm, the "Assert(rte->subquery != NULL)" doesn't seem right ...
> couldn't there be non-RTE_SUBQUERY rtes in the child?  I think the
> original coding was guaranteed to visit only subquery-type RTEs
> but I'm much less convinced about this one.  It might
> be better to say
>     if (rte->rtekind == RTE_SUBQUERY)
>         IncrementVarSublevelsUp(...);
>
> Or maybe it's okay; I'm too lazy to recheck the representation of
> UNION ALL right now.

Oh, indeed it's not okay. The original UNION ALL view is a prime example
of that. I didn't notice because I was testing without assertions.

Hmm, do we need the copyObject() call for non-subquery RTEs? I'm
guessing no, because they're not modified.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com
Index: src/backend/optimizer/prep/prepjointree.c
===================================================================
RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/optimizer/prep/prepjointree.c,v
retrieving revision 1.44
diff -c -r1.44 prepjointree.c
*** src/backend/optimizer/prep/prepjointree.c    4 Oct 2006 00:29:54 -0000    1.44
--- src/backend/optimizer/prep/prepjointree.c    13 Aug 2008 06:41:49 -0000
***************
*** 46,52 ****
  static Node *pull_up_simple_union_all(PlannerInfo *root, Node *jtnode,
                           RangeTblEntry *rte);
  static void pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root,
!                            int parentRTindex, Query *setOpQuery);
  static void make_setop_translation_lists(Query *query,
                               Index newvarno,
                               List **col_mappings, List **translated_vars);
--- 46,52 ----
  static Node *pull_up_simple_union_all(PlannerInfo *root, Node *jtnode,
                           RangeTblEntry *rte);
  static void pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root,
!                int parentRTindex, Query *setOpQuery, int childRToffset);
  static void make_setop_translation_lists(Query *query,
                               Index newvarno,
                               List **col_mappings, List **translated_vars);
***************
*** 477,490 ****
  {
      int            varno = ((RangeTblRef *) jtnode)->rtindex;
      Query       *subquery = rte->subquery;

      /*
!      * Recursively scan the subquery's setOperations tree and copy the leaf
!      * subqueries into the parent rangetable.  Add AppendRelInfo nodes for
!      * them to the parent's append_rel_list, too.
       */
      Assert(subquery->setOperations);
!     pull_up_union_leaf_queries(subquery->setOperations, root, varno, subquery);

      /*
       * Mark the parent as an append relation.
--- 477,520 ----
  {
      int            varno = ((RangeTblRef *) jtnode)->rtindex;
      Query       *subquery = rte->subquery;
+     ListCell   *l;
+     int            rtoffset;

      /*
!      * Append the subquery rtable entries to upper query.
!      */
!     rtoffset = list_length(root->parse->rtable);
!     foreach(l, subquery->rtable)
!     {
!         RangeTblEntry *rte = (RangeTblEntry *) lfirst(l);
!
!         /*
!          * Upper-level vars in subquery are now one level closer to their
!          * parent than before.    We don't have to worry about offsetting
!          * varnos, though, because any such vars must refer to stuff above the
!          * level of the query we are pulling into.
!          */
!         if (rte->subquery)
!         {
!             /* Make a modifiable copy of the child RTE and contained query. */
!             rte = copyObject(rte);
!             IncrementVarSublevelsUp((Node *) rte->subquery, -1, 1);
!         }
!
!         /*
!          * Attach child RTE to parent rtable.
!          */
!         root->parse->rtable = lappend(root->parse->rtable, rte);
!     }
!
!     /*
!      * Recursively scan the subquery's setOperations tree and add
!      * AppendRelInfo nodes for leaf subqueries to the parent's
!      * append_rel_list.
       */
      Assert(subquery->setOperations);
!     pull_up_union_leaf_queries(subquery->setOperations, root, varno, subquery,
!                                rtoffset);

      /*
       * Mark the parent as an append relation.
***************
*** 500,540 ****
   * Note that setOpQuery is the Query containing the setOp node, whose rtable
   * is where to look up the RTE if setOp is a RangeTblRef.  This is *not* the
   * same as root->parse, which is the top-level Query we are pulling up into.
   * parentRTindex is the appendrel parent's index in root->parse->rtable.
   */
  static void
  pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root, int parentRTindex,
!                            Query *setOpQuery)
  {
      if (IsA(setOp, RangeTblRef))
      {
          RangeTblRef *rtr = (RangeTblRef *) setOp;
-         RangeTblEntry *rte = rt_fetch(rtr->rtindex, setOpQuery->rtable);
-         Query       *subquery;
          int            childRTindex;
          AppendRelInfo *appinfo;
-         Query       *parse = root->parse;
-
-         /*
-          * Make a modifiable copy of the child RTE and contained query.
-          */
-         rte = copyObject(rte);
-         subquery = rte->subquery;
-         Assert(subquery != NULL);

          /*
!          * Upper-level vars in subquery are now one level closer to their
!          * parent than before.    We don't have to worry about offsetting
!          * varnos, though, because any such vars must refer to stuff above the
!          * level of the query we are pulling into.
!          */
!         IncrementVarSublevelsUp((Node *) subquery, -1, 1);
!
!         /*
!          * Attach child RTE to parent rtable.
           */
!         parse->rtable = lappend(parse->rtable, rte);
!         childRTindex = list_length(parse->rtable);

          /*
           * Build a suitable AppendRelInfo, and attach to parent's list.
--- 530,555 ----
   * Note that setOpQuery is the Query containing the setOp node, whose rtable
   * is where to look up the RTE if setOp is a RangeTblRef.  This is *not* the
   * same as root->parse, which is the top-level Query we are pulling up into.
+  *
   * parentRTindex is the appendrel parent's index in root->parse->rtable.
+  *
+  * The child RTEs have already been copied to the parent. childRToffset
+  * tells us where in the parent's range table they were copied.
   */
  static void
  pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root, int parentRTindex,
!                            Query *setOpQuery, int childRToffset)
  {
      if (IsA(setOp, RangeTblRef))
      {
          RangeTblRef *rtr = (RangeTblRef *) setOp;
          int            childRTindex;
          AppendRelInfo *appinfo;

          /*
!          * Calculate the index in the parent's range table
           */
!         childRTindex = childRToffset + rtr->rtindex;

          /*
           * Build a suitable AppendRelInfo, and attach to parent's list.
***************
*** 566,573 ****
          SetOperationStmt *op = (SetOperationStmt *) setOp;

          /* Recurse to reach leaf queries */
!         pull_up_union_leaf_queries(op->larg, root, parentRTindex, setOpQuery);
!         pull_up_union_leaf_queries(op->rarg, root, parentRTindex, setOpQuery);
      }
      else
      {
--- 581,590 ----
          SetOperationStmt *op = (SetOperationStmt *) setOp;

          /* Recurse to reach leaf queries */
!         pull_up_union_leaf_queries(op->larg, root, parentRTindex, setOpQuery,
!                                    childRToffset);
!         pull_up_union_leaf_queries(op->rarg, root, parentRTindex, setOpQuery,
!                                    childRToffset);
      }
      else
      {

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:
> Tom Lane wrote:
>> Hm, the "Assert(rte->subquery != NULL)" doesn't seem right ...
>> couldn't there be non-RTE_SUBQUERY rtes in the child?

> Oh, indeed it's not okay. The original UNION ALL view is a prime example
> of that. I didn't notice because I was testing without assertions.

Boo ...

> Hmm, do we need the copyObject() call for non-subquery RTEs? I'm
> guessing no, because they're not modified.

Probably not.  But it strikes me that there's another sin of omission
here: function and values RTEs need to be tweaked too, because they
contain expressions thst could have uplevel Vars in them.  I'm not
certain such RTEs could appear at top level in a UNION query, but I'm
not sure they couldn't either.

I think I'd recommend continuing to copy the RTE unconditionally,
because in the cases where it's not going to be modified, there's
not enough substructure to make this expensive.

            regards, tom lane

Tom Lane wrote:
> Probably not.  But it strikes me that there's another sin of omission
> here: function and values RTEs need to be tweaked too, because they
> contain expressions thst could have uplevel Vars in them.  I'm not
> certain such RTEs could appear at top level in a UNION query, but I'm
> not sure they couldn't either.

Hmm. Maybe through a rewrite or something?

We should use range_table_walker, which knows how to descend into all
kinds of RTEs...

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com
Index: src/backend/optimizer/prep/prepjointree.c
===================================================================
RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/optimizer/prep/prepjointree.c,v
retrieving revision 1.44
diff -c -r1.44 prepjointree.c
*** src/backend/optimizer/prep/prepjointree.c    4 Oct 2006 00:29:54 -0000    1.44
--- src/backend/optimizer/prep/prepjointree.c    14 Aug 2008 11:50:22 -0000
***************
*** 46,52 ****
  static Node *pull_up_simple_union_all(PlannerInfo *root, Node *jtnode,
                           RangeTblEntry *rte);
  static void pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root,
!                            int parentRTindex, Query *setOpQuery);
  static void make_setop_translation_lists(Query *query,
                               Index newvarno,
                               List **col_mappings, List **translated_vars);
--- 46,53 ----
  static Node *pull_up_simple_union_all(PlannerInfo *root, Node *jtnode,
                           RangeTblEntry *rte);
  static void pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root,
!                           int parentRTindex, Query *setOpQuery,
!                           int childRToffset);
  static void make_setop_translation_lists(Query *query,
                               Index newvarno,
                               List **col_mappings, List **translated_vars);
***************
*** 477,490 ****
  {
      int            varno = ((RangeTblRef *) jtnode)->rtindex;
      Query       *subquery = rte->subquery;

      /*
!      * Recursively scan the subquery's setOperations tree and copy the leaf
!      * subqueries into the parent rangetable.  Add AppendRelInfo nodes for
!      * them to the parent's append_rel_list, too.
       */
      Assert(subquery->setOperations);
!     pull_up_union_leaf_queries(subquery->setOperations, root, varno, subquery);

      /*
       * Mark the parent as an append relation.
--- 478,511 ----
  {
      int            varno = ((RangeTblRef *) jtnode)->rtindex;
      Query       *subquery = rte->subquery;
+     int            rtoffset;
+     List       *rtable;

      /*
!      * Append the subquery rtable entries to upper query.
!      */
!     rtoffset = list_length(root->parse->rtable);
!
!     /*
!      * Append child RTEs to parent rtable.
!      *
!      * Upper-level vars in subquery are now one level closer to their
!      * parent than before.    We don't have to worry about offsetting
!      * varnos, though, because any such vars must refer to stuff above the
!      * level of the query we are pulling into.
!      */
!     rtable = copyObject(subquery->rtable);
!     IncrementVarSublevelsUp_rtable(rtable, -1, 1);
!     root->parse->rtable = list_concat(root->parse->rtable, rtable);
!
!     /*
!      * Recursively scan the subquery's setOperations tree and add
!      * AppendRelInfo nodes for leaf subqueries to the parent's
!      * append_rel_list.
       */
      Assert(subquery->setOperations);
!     pull_up_union_leaf_queries(subquery->setOperations, root, varno, subquery,
!                                rtoffset);

      /*
       * Mark the parent as an append relation.
***************
*** 500,540 ****
   * Note that setOpQuery is the Query containing the setOp node, whose rtable
   * is where to look up the RTE if setOp is a RangeTblRef.  This is *not* the
   * same as root->parse, which is the top-level Query we are pulling up into.
   * parentRTindex is the appendrel parent's index in root->parse->rtable.
   */
  static void
  pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root, int parentRTindex,
!                            Query *setOpQuery)
  {
      if (IsA(setOp, RangeTblRef))
      {
          RangeTblRef *rtr = (RangeTblRef *) setOp;
-         RangeTblEntry *rte = rt_fetch(rtr->rtindex, setOpQuery->rtable);
-         Query       *subquery;
          int            childRTindex;
          AppendRelInfo *appinfo;
-         Query       *parse = root->parse;
-
-         /*
-          * Make a modifiable copy of the child RTE and contained query.
-          */
-         rte = copyObject(rte);
-         subquery = rte->subquery;
-         Assert(subquery != NULL);
-
-         /*
-          * Upper-level vars in subquery are now one level closer to their
-          * parent than before.    We don't have to worry about offsetting
-          * varnos, though, because any such vars must refer to stuff above the
-          * level of the query we are pulling into.
-          */
-         IncrementVarSublevelsUp((Node *) subquery, -1, 1);

          /*
!          * Attach child RTE to parent rtable.
           */
!         parse->rtable = lappend(parse->rtable, rte);
!         childRTindex = list_length(parse->rtable);

          /*
           * Build a suitable AppendRelInfo, and attach to parent's list.
--- 521,546 ----
   * Note that setOpQuery is the Query containing the setOp node, whose rtable
   * is where to look up the RTE if setOp is a RangeTblRef.  This is *not* the
   * same as root->parse, which is the top-level Query we are pulling up into.
+  *
   * parentRTindex is the appendrel parent's index in root->parse->rtable.
+  *
+  * The child RTEs have already been copied to the parent. childRToffset
+  * tells us where in the parent's range table they were copied.
   */
  static void
  pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root, int parentRTindex,
!                            Query *setOpQuery, int childRToffset)
  {
      if (IsA(setOp, RangeTblRef))
      {
          RangeTblRef *rtr = (RangeTblRef *) setOp;
          int            childRTindex;
          AppendRelInfo *appinfo;

          /*
!          * Calculate the index in the parent's range table
           */
!         childRTindex = childRToffset + rtr->rtindex;

          /*
           * Build a suitable AppendRelInfo, and attach to parent's list.
***************
*** 566,573 ****
          SetOperationStmt *op = (SetOperationStmt *) setOp;

          /* Recurse to reach leaf queries */
!         pull_up_union_leaf_queries(op->larg, root, parentRTindex, setOpQuery);
!         pull_up_union_leaf_queries(op->rarg, root, parentRTindex, setOpQuery);
      }
      else
      {
--- 572,581 ----
          SetOperationStmt *op = (SetOperationStmt *) setOp;

          /* Recurse to reach leaf queries */
!         pull_up_union_leaf_queries(op->larg, root, parentRTindex, setOpQuery,
!                                    childRToffset);
!         pull_up_union_leaf_queries(op->rarg, root, parentRTindex, setOpQuery,
!                                    childRToffset);
      }
      else
      {
Index: src/backend/rewrite/rewriteManip.c
===================================================================
RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/rewrite/rewriteManip.c,v
retrieving revision 1.102
diff -c -r1.102 rewriteManip.c
*** src/backend/rewrite/rewriteManip.c    4 Oct 2006 00:29:56 -0000    1.102
--- src/backend/rewrite/rewriteManip.c    14 Aug 2008 11:50:36 -0000
***************
*** 509,514 ****
--- 509,529 ----
                                      0);
  }

+ void
+ IncrementVarSublevelsUp_rtable(List *rtable, int delta_sublevels_up,
+                                int min_sublevels_up)
+ {
+     IncrementVarSublevelsUp_context context;
+
+     context.delta_sublevels_up = delta_sublevels_up;
+     context.min_sublevels_up = min_sublevels_up;
+
+     range_table_walker(rtable,
+                        IncrementVarSublevelsUp_walker,
+                        (void *) &context,
+                        0);
+ }
+

  /*
   * rangeTableEntry_used - detect whether an RTE is referenced somewhere
Index: src/include/rewrite/rewriteManip.h
===================================================================
RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/include/rewrite/rewriteManip.h,v
retrieving revision 1.42
diff -c -r1.42 rewriteManip.h
*** src/include/rewrite/rewriteManip.h    5 Mar 2006 15:58:58 -0000    1.42
--- src/include/rewrite/rewriteManip.h    14 Aug 2008 11:38:08 -0000
***************
*** 22,27 ****
--- 22,29 ----
                 int sublevels_up);
  extern void IncrementVarSublevelsUp(Node *node, int delta_sublevels_up,
                          int min_sublevels_up);
+ extern void IncrementVarSublevelsUp_rtable(List *rtable,
+                                int delta_sublevels_up,    int min_sublevels_up);

  extern bool rangeTableEntry_used(Node *node, int rt_index,
                       int sublevels_up);

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:
> We should use range_table_walker, which knows how to descend into all
> kinds of RTEs...

Yeah, I had been thinking of that but didn't see an easy way to apply
it; of course the answer is to use IncrementVarSublevelsUp_walker
directly as you have here.

Looks good except IncrementVarSublevelsUp_rtable really ought to have
a comment.  Please fix that, commit, and backpatch as needed.

            regards, tom lane

Tom Lane wrote:
> "Heikki Linnakangas" <heikki@enterprisedb.com> writes:
>> We should use range_table_walker, which knows how to descend into all
>> kinds of RTEs...
>
> Yeah, I had been thinking of that but didn't see an easy way to apply
> it; of course the answer is to use IncrementVarSublevelsUp_walker
> directly as you have here.
>
> Looks good except IncrementVarSublevelsUp_rtable really ought to have
> a comment.  Please fix that, commit, and backpatch as needed.

Done.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com