Re: pgsql: Add regression test for bug fixed by recent refactoring. - Mailing list pgsql-committers

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;