Thread: pgsql: Add regression test for bug fixed by recent refactoring.

pgsql: Add regression test for bug fixed by recent refactoring.

From
Kevin Grittner
Date:
Add regression test for bug fixed by recent refactoring.

Test case by Andres Freund for bug fixed by Tom Lane's refactoring
in commit 5194024d72f33fb209e10f9ab0ada7cc67df45b7

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/200ba1667b3a8d7a9d559d2f05f83d209c9d8267

Modified Files
--------------
src/test/regress/expected/matview.out |   12 ++++++++++++
src/test/regress/sql/matview.sql      |    7 +++++++
2 files changed, 19 insertions(+), 0 deletions(-)


Re: pgsql: Add regression test for bug fixed by recent refactoring.

From
Tom Lane
Date:
Kevin Grittner <kgrittn@postgresql.org> writes:
> Add regression test for bug fixed by recent refactoring.
> Test case by Andres Freund for bug fixed by Tom Lane's refactoring
> in commit 5194024d72f33fb209e10f9ab0ada7cc67df45b7

Hm, that actually has got nothing much to do with matviews:

regression=# create view vv1 as select false;
CREATE VIEW
regression=# create view vv2 as select false where false;
CREATE VIEW
regression=# create user joe;
CREATE ROLE
regression=# \c - joe
You are now connected to database "regression" as user "joe".
regression=> select * from vv1;
ERROR:  permission denied for relation vv1
regression=> select * from vv2;
 bool
------
(0 rows)

Of course the select from vv2 should fail as well, but it doesn't,
because vv2 is nowhere to be seen in the rangetable passed to the
executor.  I think the planner is probably trashing the rangetable
once it realizes that the query is completely dummy; but this is wrong
because we need to leave view rangetable entries behind for permissions
checks, even when they're unreferenced in the finished plan.

            regards, tom lane


Re: pgsql: Add regression test for bug fixed by recent refactoring.

From
Tom Lane
Date:
I wrote:
> Of course the select from vv2 should fail as well, but it doesn't,
> because vv2 is nowhere to be seen in the rangetable passed to the
> executor.  I think the planner is probably trashing the rangetable
> once it realizes that the query is completely dummy; but this is wrong
> because we need to leave view rangetable entries behind for permissions
> checks, even when they're unreferenced in the finished plan.

I found the problem.  set_subquery_pathlist does this when the subquery
is demonstrably empty (ie "select something where false"):

    /*
     * 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 level.
     */
    if (is_dummy_plan(rel->subplan))
    {
        set_dummy_rel_pathlist(rel);
        return;
    }

The trouble is that, with no SubqueryScan path, setrefs.c doesn't
realize that it needs to pull up the subquery's rangetable into the main
query.  We could just delete the above chunk of code, but that would
result in worse planning choices in some complicated queries.
What I'm planning to do instead is teach createplan.c to compensate for
this by wrapping the dummy Result plan in a SubqueryScan, thus producing
the same plan tree as if set_subquery_pathlist had not done the above.
This is a tad Rube Goldbergian, perhaps, because after setrefs.c has
done its thing it will conclude that the SubqueryScan is pointless and
strip it off again :-).  (So we end up with the same finished plan tree
as now, only it will have all the rangetable entries it should have for
permissions checking purposes.)  However, the alternative seems to be
to devise some entirely new method for setrefs.c to figure out which
subqueries' rangetables still need to be pulled up into the main query
and which don't.  That would be a much more invasive patch, without a
lot of redeeming social value AFAICS.

            regards, tom lane

diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 86d5bb71b0a0a3811a2d5fa2f0682b8964ff037d..8fc933bc0769d5c6957be7f81e53a1ed1b53f43d 100644
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
*************** set_subquery_pathlist(PlannerInfo *root,
*** 1159,1165 ****
      /*
       * 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 level.
       */
      if (is_dummy_plan(rel->subplan))
      {
--- 1159,1166 ----
      /*
       * 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 level.  (But see
!      * create_append_plan in createplan.c, which has to partially undo this.)
       */
      if (is_dummy_plan(rel->subplan))
      {
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 65a2e38fc33e168621bbe9c0becb9d101129d862..b8055c79da7131752d555d86ad0a44506954cea7 100644
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
*************** static Plan *
*** 678,707 ****
  create_append_plan(PlannerInfo *root, AppendPath *best_path)
  {
      Append       *plan;
!     List       *tlist = build_relation_tlist(best_path->path.parent);
      List       *subplans = NIL;
      ListCell   *subpaths;

      /*
!      * It is possible for the subplans list to contain only one entry, or even
!      * no entries.    Handle these cases specially.
       *
!      * XXX ideally, if there's just one entry, we'd not bother to generate an
!      * Append node but just return the single child.  At the moment this does
!      * not work because the varno of the child scan plan won't match the
!      * parent-rel Vars it'll be asked to emit.
       */
      if (best_path->subpaths == NIL)
      {
          /* Generate a Result plan with constant-FALSE gating qual */
!         return (Plan *) make_result(root,
!                                     tlist,
!                                     (Node *) list_make1(makeBoolConst(false,
!                                                                       false)),
!                                     NULL);
      }

!     /* Normal case with multiple subpaths */
      foreach(subpaths, best_path->subpaths)
      {
          Path       *subpath = (Path *) lfirst(subpaths);
--- 678,727 ----
  create_append_plan(PlannerInfo *root, AppendPath *best_path)
  {
      Append       *plan;
!     RelOptInfo *rel = best_path->path.parent;
!     List       *tlist = build_relation_tlist(rel);
      List       *subplans = NIL;
      ListCell   *subpaths;

      /*
!      * The subpaths list could be empty, if every child was proven empty by
!      * constraint exclusion.  In that case generate a dummy plan that returns
!      * no rows.
       *
!      * Note that an AppendPath with no members is also generated in certain
!      * cases where there was no appending construct at all, but we know the
!      * relation is empty (see set_dummy_rel_pathlist).
       */
      if (best_path->subpaths == NIL)
      {
+         Plan       *res;
+
          /* Generate a Result plan with constant-FALSE gating qual */
!         res = (Plan *) make_result(root,
!                                    tlist,
!                                    (Node *) list_make1(makeBoolConst(false,
!                                                                      false)),
!                                    NULL);
!
!         /*
!          * If this is a dummy path for a subquery, we have to wrap it 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));
!             res = (Plan *) make_subqueryscan(tlist,
!                                              NIL,
!                                              rel->relid,
!                                              rel->subplan);
!         }
!
!         return res;
      }

!     /* Build the plan for each child */
      foreach(subpaths, best_path->subpaths)
      {
          Path       *subpath = (Path *) lfirst(subpaths);
*************** create_append_plan(PlannerInfo *root, Ap
*** 709,714 ****
--- 729,741 ----
          subplans = lappend(subplans, create_plan_recurse(root, subpath));
      }

+     /*
+      * XXX ideally, if there's just one child, we'd not bother to generate an
+      * Append node but just return the single child.  At the moment this does
+      * not work because the varno of the child scan plan won't match the
+      * parent-rel Vars it'll be asked to emit.
+      */
+
      plan = make_append(subplans, tlist);

      return (Plan *) plan;