Better fix for permissions tests in excluded subqueries.

This reverts the code changes in 50c137487c96e629e0e5372bb3d1b5f1a2f71a88,
which turned out to induce crashes and not completely fix the problem
anyway.  That commit only considered single subqueries that were excluded
by constraint-exclusion logic, but actually the problem also exists for
subqueries that are appendrel members (ie part of a UNION ALL list).  In
such cases we can't add a dummy subpath to the appendrel's AppendPath list
without defeating the logic that recognizes when an appendrel is completely
excluded.  Instead, fix the problem by having setrefs.c scan the rangetable
an extra time looking for subqueries that didn't get into the plan tree.
(This approach depends on the 9.2 change that made set_subquery_pathlist
generate dummy paths for excluded single subqueries, so that the exclusion
behavior is the same for single subqueries and appendrel members.)

Note: it turns out that the appendrel form of the missed-permissions-checks
bug exists as far back as 8.4.  However, since the practical effect of that
bug seems pretty minimal, consensus is to not attempt to fix it in the back
branches, at least not yet.  Possibly we could back-port this patch once
it's gotten a reasonable amount of testing in HEAD.  For the moment I'm
just going to revert the previous patch in 9.2.

diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 474cd7b6eba4ccbf241473aa9ade839fe81355d6..105718ff371cb0e980f3860161277317511ca767 100644 (file)
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1159,9 +1159,7 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
        /*
         * It's possible that constraint exclusion proved the subquery empty. If
         * so, it's convenient to turn it back into a dummy path so that we will
-        * recognize appropriate optimizations at this query level.  (But see
-        * create_append_plan in createplan.c, which has to reverse this
-        * substitution.)
+        * recognize appropriate optimizations at this query level.
         */
        if (is_dummy_plan(rel->subplan))
        {

diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index e43ef3aa898d47883508253a7e60ac635caf6886..52bab79007e66d80205a07da08bc4437c424232c 100644 (file)
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -678,8 +678,7 @@ static Plan *
 create_append_plan(PlannerInfo *root, AppendPath *best_path)
 {
        Append     *plan;
-       RelOptInfo *rel = best_path->path.parent;
-       List       *tlist = build_relation_tlist(rel);
+       List       *tlist = build_relation_tlist(best_path->path.parent);
        List       *subplans = NIL;
        ListCell   *subpaths;
 
@@ -694,30 +693,12 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path)
         */
        if (best_path->subpaths == NIL)
        {
-               /*
-                * If this is a dummy path for a subquery, we have to wrap the
-                * subquery's original plan in a SubqueryScan so that setrefs.c will
-                * do the right things.  (In particular, it must pull up the
-                * subquery's rangetable so that the executor will apply permissions
-                * checks to those rels at runtime.)
-                */
-               if (rel->rtekind == RTE_SUBQUERY)
-               {
-                       Assert(is_dummy_plan(rel->subplan));
-                       return (Plan *) make_subqueryscan(tlist,
-                                                                                         NIL,
-                                                                                         rel->relid,
-                                                                                         rel->subplan);
-               }
-               else
-               {
-                       /* Generate a Result plan with constant-FALSE gating qual */
-                       return (Plan *) make_result(root,
-                                                                               tlist,
-                                                                        (Node *) list_make1(makeBoolConst(false,
-                                                                                                                                        false)),
-                                                                               NULL);
-               }
+               /* Generate a Result plan with constant-FALSE gating qual */
+               return (Plan *) make_result(root,
+                                                                       tlist,
+                                                                       (Node *) list_make1(makeBoolConst(false,
+                                                                                                                                         false)),
+                                                                       NULL);
        }
 
        /* Build the plan for each child */

diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 6b8faf52b0a231449b559aaa15a1685f19eef71f..b78d72701b3d6a5890850c41b8de6609c4a7c066 100644 (file)
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -21,6 +21,7 @@
 #include "nodes/nodeFuncs.h"
 #include "optimizer/pathnode.h"
 #include "optimizer/planmain.h"
+#include "optimizer/planner.h"
 #include "optimizer/tlist.h"
 #include "tcop/utility.h"
 #include "utils/lsyscache.h"
@@ -81,6 +82,10 @@ typedef struct
 #define fix_scan_list(root, lst, rtoffset) \
        ((List *) fix_scan_expr(root, (Node *) (lst), rtoffset))
 
+static void add_rtes_to_flat_rtable(PlannerInfo *root, bool recursing);
+static void flatten_unplanned_rtes(PlannerGlobal *glob, RangeTblEntry *rte);
+static bool flatten_rtes_walker(Node *node, PlannerGlobal *glob);
+static void add_rte_to_flat_rtable(PlannerGlobal *glob, RangeTblEntry *rte);
 static Plan *set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset);
 static Plan *set_indexonlyscan_references(PlannerInfo *root,
                                                         IndexOnlyScan *plan,
@@ -196,63 +201,11 @@ set_plan_references(PlannerInfo *root, Plan *plan)
        ListCell   *lc;
 
        /*
-        * In the flat rangetable, we zero out substructure pointers that are not
-        * needed by the executor; this reduces the storage space and copying cost
-        * for cached plans.  We keep only the alias and eref Alias fields, which
-        * are needed by EXPLAIN, and the selectedCols and modifiedCols bitmaps,
-        * which are needed for executor-startup permissions checking and for
-        * trigger event checking.
+        * Add all the query's RTEs to the flattened rangetable.  The live ones
+        * will have their rangetable indexes increased by rtoffset.  (Additional
+        * RTEs, not referenced by the Plan tree, might get added after those.)
         */
-       foreach(lc, root->parse->rtable)
-       {
-               RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
-               RangeTblEntry *newrte;
-
-               /* flat copy to duplicate all the scalar fields */
-               newrte = (RangeTblEntry *) palloc(sizeof(RangeTblEntry));
-               memcpy(newrte, rte, sizeof(RangeTblEntry));
-
-               /* zap unneeded sub-structure */
-               newrte->subquery = NULL;
-               newrte->joinaliasvars = NIL;
-               newrte->funcexpr = NULL;
-               newrte->funccoltypes = NIL;
-               newrte->funccoltypmods = NIL;
-               newrte->funccolcollations = NIL;
-               newrte->values_lists = NIL;
-               newrte->values_collations = NIL;
-               newrte->ctecoltypes = NIL;
-               newrte->ctecoltypmods = NIL;
-               newrte->ctecolcollations = NIL;
-
-               glob->finalrtable = lappend(glob->finalrtable, newrte);
-
-               /*
-                * If it's a plain relation RTE, add the table to relationOids.
-                *
-                * We do this even though the RTE might be unreferenced in the plan
-                * tree; this would correspond to cases such as views that were
-                * expanded, child tables that were eliminated by constraint
-                * exclusion, etc.      Schema invalidation on such a rel must still force
-                * rebuilding of the plan.
-                *
-                * Note we don't bother to avoid duplicate list entries.  We could,
-                * but it would probably cost more cycles than it would save.
-                */
-               if (newrte->rtekind == RTE_RELATION)
-                       glob->relationOids = lappend_oid(glob->relationOids,
-                                                                                        newrte->relid);
-       }
-
-       /*
-        * Check for RT index overflow; it's very unlikely, but if it did happen,
-        * the executor would get confused by varnos that match the special varno
-        * values.
-        */
-       if (IS_SPECIAL_VARNO(list_length(glob->finalrtable)))
-               ereport(ERROR,
-                               (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
-                                errmsg("too many range table entries")));
+       add_rtes_to_flat_rtable(root, false);
 
        /*
         * Adjust RT indexes of PlanRowMarks and add to final rowmarks list
@@ -279,6 +232,192 @@ set_plan_references(PlannerInfo *root, Plan *plan)
        return set_plan_refs(root, plan, rtoffset);
 }
 
+/*
+ * Extract RangeTblEntries from the plan's rangetable, and add to flat rtable
+ *
+ * This can recurse into subquery plans; "recursing" is true if so.
+ */
+static void
+add_rtes_to_flat_rtable(PlannerInfo *root, bool recursing)
+{
+       PlannerGlobal *glob = root->glob;
+       Index           rti;
+       ListCell   *lc;
+
+       /*
+        * Add the query's own RTEs to the flattened rangetable.
+        *
+        * At top level, we must add all RTEs so that their indexes in the
+        * flattened rangetable match up with their original indexes.  When
+        * recursing, we only care about extracting relation RTEs.
+        */
+       foreach(lc, root->parse->rtable)
+       {
+               RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
+
+               if (!recursing || rte->rtekind == RTE_RELATION)
+                       add_rte_to_flat_rtable(glob, rte);
+       }
+
+       /*
+        * If there are any dead subqueries, they are not referenced in the Plan
+        * tree, so we must add RTEs contained in them to the flattened rtable
+        * separately.  (If we failed to do this, the executor would not perform
+        * expected permission checks for tables mentioned in such subqueries.)
+        *
+        * Note: this pass over the rangetable can't be combined with the previous
+        * one, because that would mess up the numbering of the live RTEs in the
+        * flattened rangetable.
+        */
+       rti = 1;
+       foreach(lc, root->parse->rtable)
+       {
+               RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
+
+               /*
+                * We should ignore inheritance-parent RTEs: their contents have been
+                * pulled up into our rangetable already.  Also ignore any subquery
+                * RTEs without matching RelOptInfos, as they likewise have been
+                * pulled up.
+                */
+               if (rte->rtekind == RTE_SUBQUERY && !rte->inh)
+               {
+                       RelOptInfo *rel = root->simple_rel_array[rti];
+
+                       if (rel != NULL)
+                       {
+                               Assert(rel->relid == rti);              /* sanity check on array */
+
+                               /*
+                                * The subquery might never have been planned at all, if it
+                                * was excluded on the basis of self-contradictory constraints
+                                * in our query level.  In this case apply
+                                * flatten_unplanned_rtes.
+                                *
+                                * If it was planned but the plan is dummy, we assume that it
+                                * has been omitted from our plan tree (see
+                                * set_subquery_pathlist), and recurse to pull up its RTEs.
+                                *
+                                * Otherwise, it should be represented by a SubqueryScan node
+                                * somewhere in our plan tree, and we'll pull up its RTEs when
+                                * we process that plan node.
+                                *
+                                * However, if we're recursing, then we should pull up RTEs
+                                * whether the subplan is dummy or not, because we've found
+                                * that some upper query level is treating this one as dummy,
+                                * and so we won't scan this level's plan tree at all.
+                                */
+                               if (rel->subplan == NULL)
+                                       flatten_unplanned_rtes(glob, rte);
+                               else if (recursing || is_dummy_plan(rel->subplan))
+                               {
+                                       Assert(rel->subroot != NULL);
+                                       add_rtes_to_flat_rtable(rel->subroot, true);
+                               }
+                       }
+               }
+               rti++;
+       }
+}
+
+/*
+ * Extract RangeTblEntries from a subquery that was never planned at all
+ */
+static void
+flatten_unplanned_rtes(PlannerGlobal *glob, RangeTblEntry *rte)
+{
+       /* Use query_tree_walker to find all RTEs in the parse tree */
+       (void) query_tree_walker(rte->subquery,
+                                                        flatten_rtes_walker,
+                                                        (void *) glob,
+                                                        QTW_EXAMINE_RTES);
+}
+
+static bool
+flatten_rtes_walker(Node *node, PlannerGlobal *glob)
+{
+       if (node == NULL)
+               return false;
+       if (IsA(node, RangeTblEntry))
+       {
+               RangeTblEntry *rte = (RangeTblEntry *) node;
+
+               /* As above, we need only save relation RTEs */
+               if (rte->rtekind == RTE_RELATION)
+                       add_rte_to_flat_rtable(glob, rte);
+               return false;
+       }
+       if (IsA(node, Query))
+       {
+               /* Recurse into subselects */
+               return query_tree_walker((Query *) node,
+                                                                flatten_rtes_walker,
+                                                                (void *) glob,
+                                                                QTW_EXAMINE_RTES);
+       }
+       return expression_tree_walker(node, flatten_rtes_walker,
+                                                                 (void *) glob);
+}
+
+/*
+ * Add (a copy of) the given RTE to the final rangetable
+ *
+ * In the flat rangetable, we zero out substructure pointers that are not
+ * needed by the executor; this reduces the storage space and copying cost
+ * for cached plans.  We keep only the alias and eref Alias fields, which
+ * are needed by EXPLAIN, and the selectedCols and modifiedCols bitmaps,
+ * which are needed for executor-startup permissions checking and for
+ * trigger event checking.
+ */
+static void
+add_rte_to_flat_rtable(PlannerGlobal *glob, RangeTblEntry *rte)
+{
+       RangeTblEntry *newrte;
+
+       /* flat copy to duplicate all the scalar fields */
+       newrte = (RangeTblEntry *) palloc(sizeof(RangeTblEntry));
+       memcpy(newrte, rte, sizeof(RangeTblEntry));
+
+       /* zap unneeded sub-structure */
+       newrte->subquery = NULL;
+       newrte->joinaliasvars = NIL;
+       newrte->funcexpr = NULL;
+       newrte->funccoltypes = NIL;
+       newrte->funccoltypmods = NIL;
+       newrte->funccolcollations = NIL;
+       newrte->values_lists = NIL;
+       newrte->values_collations = NIL;
+       newrte->ctecoltypes = NIL;
+       newrte->ctecoltypmods = NIL;
+       newrte->ctecolcollations = NIL;
+
+       glob->finalrtable = lappend(glob->finalrtable, newrte);
+
+       /*
+        * Check for RT index overflow; it's very unlikely, but if it did happen,
+        * the executor would get confused by varnos that match the special varno
+        * values.
+        */
+       if (IS_SPECIAL_VARNO(list_length(glob->finalrtable)))
+               ereport(ERROR,
+                               (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+                                errmsg("too many range table entries")));
+
+       /*
+        * If it's a plain relation RTE, add the table to relationOids.
+        *
+        * We do this even though the RTE might be unreferenced in the plan tree;
+        * this would correspond to cases such as views that were expanded, child
+        * tables that were eliminated by constraint exclusion, etc. Schema
+        * invalidation on such a rel must still force rebuilding of the plan.
+        *
+        * Note we don't bother to avoid making duplicate list entries.  We could,
+        * but it would probably cost more cycles than it would save.
+        */
+       if (newrte->rtekind == RTE_RELATION)
+               glob->relationOids = lappend_oid(glob->relationOids, newrte->relid);
+}
+
 /*
  * set_plan_refs: recurse through the Plan nodes of a single subquery level
  */

diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 68afecc91f7760b9d7fbc50f58dfd59b2b70ea46..3da03fc9ae26666895aece652276199bc92826f7 100644 (file)
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -228,6 +228,21 @@ SELECT * FROM atestv3; -- ok
 
 SELECT * FROM atestv0; -- fail
 ERROR:  permission denied for relation atestv0
+-- Appendrels excluded by constraints failed to check permissions in 8.4-9.2.
+select * from
+  ((select a.q1 as x from int8_tbl a offset 0)
+   union all
+   (select b.q2 as x from int8_tbl b offset 0)) ss
+where false;
+ERROR:  permission denied for relation int8_tbl
+set constraint_exclusion = on;
+select * from
+  ((select a.q1 as x, random() from int8_tbl a where q1 > 0)
+   union all
+   (select b.q2 as x, random() from int8_tbl b where q2 > 0)) ss
+where x < 0;
+ERROR:  permission denied for relation int8_tbl
+reset constraint_exclusion;
 CREATE VIEW atestv4 AS SELECT * FROM atestv3; -- nested view
 SELECT * FROM atestv4; -- ok
  one | two | three 

diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 6ac3378a8d003e8204b1bc1fd3e85b046cd7fd2c..cb993ae2b0907475efa616eafa1292c74afa1e35 100644 (file)
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -162,6 +162,21 @@ SELECT * FROM atestv2; -- fail
 SELECT * FROM atestv3; -- ok
 SELECT * FROM atestv0; -- fail
 
+-- Appendrels excluded by constraints failed to check permissions in 8.4-9.2.
+select * from
+  ((select a.q1 as x from int8_tbl a offset 0)
+   union all
+   (select b.q2 as x from int8_tbl b offset 0)) ss
+where false;
+
+set constraint_exclusion = on;
+select * from
+  ((select a.q1 as x, random() from int8_tbl a where q1 > 0)
+   union all
+   (select b.q2 as x, random() from int8_tbl b where q2 > 0)) ss
+where x < 0;
+reset constraint_exclusion;
+
 CREATE VIEW atestv4 AS SELECT * FROM atestv3; -- nested view
 SELECT * FROM atestv4; -- ok
 GRANT SELECT ON atestv4 TO regressuser2;