Thread: ERROR: ORDER/GROUP BY expression not found in targetlist

Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> writes:
> Below test case is failing with ERROR:  ORDER/GROUP BY expression not found
> in targetlist. this issue is reproducible from PGv10.

Thanks for the report.  I traced through this, and the problem seems to
be that split_pathtarget_at_srfs() is neglecting to attach sortgroupref
labeling to the extra PathTargets it constructs.  I don't remember
whether that code is my fault or Andres', but I'll take a look at
fixing it.

            regards, tom lane

I wrote:
> Thanks for the report.  I traced through this, and the problem seems to
> be that split_pathtarget_at_srfs() is neglecting to attach sortgroupref
> labeling to the extra PathTargets it constructs.  I don't remember
> whether that code is my fault or Andres', but I'll take a look at
> fixing it.

Here's a proposed patch for that.

            regards, tom lane

diff --git a/src/backend/optimizer/util/tlist.c b/src/backend/optimizer/util/tlist.c
index 32160d5..5500f33 100644
*** a/src/backend/optimizer/util/tlist.c
--- b/src/backend/optimizer/util/tlist.c
***************
*** 25,44 ****
      ((IsA(node, FuncExpr) && ((FuncExpr *) (node))->funcretset) || \
       (IsA(node, OpExpr) && ((OpExpr *) (node))->opretset))

! /* Workspace for split_pathtarget_walker */
  typedef struct
  {
      List       *input_target_exprs; /* exprs available from input */
!     List       *level_srfs;        /* list of lists of SRF exprs */
!     List       *level_input_vars;    /* vars needed by SRFs of each level */
!     List       *level_input_srfs;    /* SRFs needed by SRFs of each level */
      List       *current_input_vars; /* vars needed in current subexpr */
      List       *current_input_srfs; /* SRFs needed in current subexpr */
      int            current_depth;    /* max SRF depth in current subexpr */
  } split_pathtarget_context;

  static bool split_pathtarget_walker(Node *node,
                          split_pathtarget_context *context);


  /*****************************************************************************
--- 25,62 ----
      ((IsA(node, FuncExpr) && ((FuncExpr *) (node))->funcretset) || \
       (IsA(node, OpExpr) && ((OpExpr *) (node))->opretset))

! /*
!  * Data structures for split_pathtarget_at_srfs().  To preserve the identity
!  * of sortgroupref items even if they are textually equal(), what we track is
!  * not just bare expressions but expressions plus their sortgroupref indexes.
!  */
  typedef struct
  {
+     Node       *expr;            /* some subexpression of a PathTarget */
+     Index        sortgroupref;    /* its sortgroupref, or 0 if none */
+ } split_pathtarget_item;
+
+ typedef struct
+ {
+     /* This is a List of bare expressions: */
      List       *input_target_exprs; /* exprs available from input */
!     /* These are Lists of Lists of split_pathtarget_items: */
!     List       *level_srfs;        /* SRF exprs to evaluate at each level */
!     List       *level_input_vars;    /* input vars needed at each level */
!     List       *level_input_srfs;    /* input SRFs needed at each level */
!     /* These are Lists of split_pathtarget_items: */
      List       *current_input_vars; /* vars needed in current subexpr */
      List       *current_input_srfs; /* SRFs needed in current subexpr */
+     /* Auxiliary data for current split_pathtarget_walker traversal: */
      int            current_depth;    /* max SRF depth in current subexpr */
+     Index        current_sgref;    /* current subexpr's sortgroupref, or 0 */
  } split_pathtarget_context;

  static bool split_pathtarget_walker(Node *node,
                          split_pathtarget_context *context);
+ static void add_sp_item_to_pathtarget(PathTarget *target,
+                           split_pathtarget_item *item);
+ static void add_sp_items_to_pathtarget(PathTarget *target, List *items);


  /*****************************************************************************
*************** apply_pathtarget_labeling_to_tlist(List
*** 822,827 ****
--- 840,848 ----
   * already meant as a reference to a lower subexpression).  So, don't expand
   * any tlist expressions that appear in input_target, if that's not NULL.
   *
+  * It's also important that we preserve any sortgroupref annotation appearing
+  * in the given target, especially on expressions matching input_target items.
+  *
   * The outputs of this function are two parallel lists, one a list of
   * PathTargets and the other an integer list of bool flags indicating
   * whether the corresponding PathTarget contains any evaluatable SRFs.
*************** split_pathtarget_at_srfs(PlannerInfo *ro
*** 845,850 ****
--- 866,872 ----
      int            max_depth;
      bool        need_extra_projection;
      List       *prev_level_tlist;
+     int            lci;
      ListCell   *lc,
                 *lc1,
                 *lc2,
*************** split_pathtarget_at_srfs(PlannerInfo *ro
*** 884,893 ****
--- 906,920 ----
      need_extra_projection = false;

      /* Scan each expression in the PathTarget looking for SRFs */
+     lci = 0;
      foreach(lc, target->exprs)
      {
          Node       *node = (Node *) lfirst(lc);

+         /* Tell split_pathtarget_walker about this expr's sortgroupref */
+         context.current_sgref = get_pathtarget_sortgroupref(target, lci);
+         lci++;
+
          /*
           * Find all SRFs and Vars (and Var-like nodes) in this expression, and
           * enter them into appropriate lists within the context struct.
*************** split_pathtarget_at_srfs(PlannerInfo *ro
*** 981,996 ****
               * This target should actually evaluate any SRFs of the current
               * level, and it needs to propagate forward any Vars needed by
               * later levels, as well as SRFs computed earlier and needed by
!              * later levels.  We rely on add_new_columns_to_pathtarget() to
!              * remove duplicate items.  Also, for safety, make a separate copy
!              * of each item for each PathTarget.
               */
!             add_new_columns_to_pathtarget(ntarget, copyObject(level_srfs));
              for_each_cell(lc, lnext(lc2))
              {
                  List       *input_vars = (List *) lfirst(lc);

!                 add_new_columns_to_pathtarget(ntarget, copyObject(input_vars));
              }
              for_each_cell(lc, lnext(lc3))
              {
--- 1008,1021 ----
               * This target should actually evaluate any SRFs of the current
               * level, and it needs to propagate forward any Vars needed by
               * later levels, as well as SRFs computed earlier and needed by
!              * later levels.
               */
!             add_sp_items_to_pathtarget(ntarget, level_srfs);
              for_each_cell(lc, lnext(lc2))
              {
                  List       *input_vars = (List *) lfirst(lc);

!                 add_sp_items_to_pathtarget(ntarget, input_vars);
              }
              for_each_cell(lc, lnext(lc3))
              {
*************** split_pathtarget_at_srfs(PlannerInfo *ro
*** 999,1008 ****

                  foreach(lcx, input_srfs)
                  {
!                     Expr       *srf = (Expr *) lfirst(lcx);

!                     if (list_member(prev_level_tlist, srf))
!                         add_new_column_to_pathtarget(ntarget, copyObject(srf));
                  }
              }
              set_pathtarget_cost_width(root, ntarget);
--- 1024,1033 ----

                  foreach(lcx, input_srfs)
                  {
!                     split_pathtarget_item *item = lfirst(lcx);

!                     if (list_member(prev_level_tlist, item->expr))
!                         add_sp_item_to_pathtarget(ntarget, item);
                  }
              }
              set_pathtarget_cost_width(root, ntarget);
*************** split_pathtarget_walker(Node *node, spli
*** 1037,1048 ****
       * input_target can be treated like a Var (which indeed it will be after
       * setrefs.c gets done with it), even if it's actually a SRF.  Record it
       * as being needed for the current expression, and ignore any
!      * substructure.
       */
      if (list_member(context->input_target_exprs, node))
      {
          context->current_input_vars = lappend(context->current_input_vars,
!                                               node);
          return false;
      }

--- 1062,1078 ----
       * input_target can be treated like a Var (which indeed it will be after
       * setrefs.c gets done with it), even if it's actually a SRF.  Record it
       * as being needed for the current expression, and ignore any
!      * substructure.  (Note in particular that this preserves the identity of
!      * any expressions that appear as sortgrouprefs in input_target.)
       */
      if (list_member(context->input_target_exprs, node))
      {
+         split_pathtarget_item *item = palloc(sizeof(split_pathtarget_item));
+
+         item->expr = node;
+         item->sortgroupref = context->current_sgref;
          context->current_input_vars = lappend(context->current_input_vars,
!                                               item);
          return false;
      }

*************** split_pathtarget_walker(Node *node, spli
*** 1057,1064 ****
          IsA(node, GroupingFunc) ||
          IsA(node, WindowFunc))
      {
          context->current_input_vars = lappend(context->current_input_vars,
!                                               node);
          return false;
      }

--- 1087,1098 ----
          IsA(node, GroupingFunc) ||
          IsA(node, WindowFunc))
      {
+         split_pathtarget_item *item = palloc(sizeof(split_pathtarget_item));
+
+         item->expr = node;
+         item->sortgroupref = context->current_sgref;
          context->current_input_vars = lappend(context->current_input_vars,
!                                               item);
          return false;
      }

*************** split_pathtarget_walker(Node *node, spli
*** 1068,1082 ****
--- 1102,1121 ----
       */
      if (IS_SRF_CALL(node))
      {
+         split_pathtarget_item *item = palloc(sizeof(split_pathtarget_item));
          List       *save_input_vars = context->current_input_vars;
          List       *save_input_srfs = context->current_input_srfs;
          int            save_current_depth = context->current_depth;
          int            srf_depth;
          ListCell   *lc;

+         item->expr = node;
+         item->sortgroupref = context->current_sgref;
+
          context->current_input_vars = NIL;
          context->current_input_srfs = NIL;
          context->current_depth = 0;
+         context->current_sgref = 0; /* subexpressions are not sortgroup items */

          (void) expression_tree_walker(node, split_pathtarget_walker,
                                        (void *) context);
*************** split_pathtarget_walker(Node *node, spli
*** 1094,1100 ****

          /* Record this SRF as needing to be evaluated at appropriate level */
          lc = list_nth_cell(context->level_srfs, srf_depth);
!         lfirst(lc) = lappend(lfirst(lc), node);

          /* Record its inputs as being needed at the same level */
          lc = list_nth_cell(context->level_input_vars, srf_depth);
--- 1133,1139 ----

          /* Record this SRF as needing to be evaluated at appropriate level */
          lc = list_nth_cell(context->level_srfs, srf_depth);
!         lfirst(lc) = lappend(lfirst(lc), item);

          /* Record its inputs as being needed at the same level */
          lc = list_nth_cell(context->level_input_vars, srf_depth);
*************** split_pathtarget_walker(Node *node, spli
*** 1108,1114 ****
           * surrounding expression.
           */
          context->current_input_vars = save_input_vars;
!         context->current_input_srfs = lappend(save_input_srfs, node);
          context->current_depth = Max(save_current_depth, srf_depth);

          /* We're done here */
--- 1147,1153 ----
           * surrounding expression.
           */
          context->current_input_vars = save_input_vars;
!         context->current_input_srfs = lappend(save_input_srfs, item);
          context->current_depth = Max(save_current_depth, srf_depth);

          /* We're done here */
*************** split_pathtarget_walker(Node *node, spli
*** 1119,1124 ****
--- 1158,1236 ----
       * Otherwise, the node is a scalar (non-set) expression, so recurse to
       * examine its inputs.
       */
+     context->current_sgref = 0; /* subexpressions are not sortgroup items */
      return expression_tree_walker(node, split_pathtarget_walker,
                                    (void *) context);
  }
+
+ /*
+  * Add a split_pathtarget_item to the PathTarget, unless a matching item is
+  * already present.  This is like add_new_column_to_pathtarget, but allows
+  * for sortgrouprefs to be handled.  An item having zero sortgroupref can
+  * be merged with one that has a sortgroupref, acquiring the latter's
+  * sortgroupref.
+  *
+  * Note that we don't worry about possibly adding duplicate sortgrouprefs
+  * to the PathTarget.  That would be bad, but it should be impossible unless
+  * the target passed to split_pathtarget_at_srfs already had duplicates.
+  * As long as it didn't, we can have at most one split_pathtarget_item with
+  * any particular nonzero sortgroupref.
+  */
+ static void
+ add_sp_item_to_pathtarget(PathTarget *target, split_pathtarget_item *item)
+ {
+     int            lci;
+     ListCell   *lc;
+
+     /*
+      * Look for a pre-existing entry that is equal() and does not have a
+      * conflicting sortgroupref already.
+      */
+     lci = 0;
+     foreach(lc, target->exprs)
+     {
+         Node       *node = (Node *) lfirst(lc);
+         Index        sgref = get_pathtarget_sortgroupref(target, lci);
+
+         if ((item->sortgroupref == sgref ||
+              item->sortgroupref == 0 ||
+              sgref == 0) &&
+             equal(item->expr, node))
+         {
+             /* Found a match.  Assign item's sortgroupref if it has one. */
+             if (item->sortgroupref)
+             {
+                 if (target->sortgrouprefs == NULL)
+                 {
+                     target->sortgrouprefs = (Index *)
+                         palloc0(list_length(target->exprs) * sizeof(Index));
+                 }
+                 target->sortgrouprefs[lci] = item->sortgroupref;
+             }
+             return;
+         }
+         lci++;
+     }
+
+     /*
+      * No match, so add item to PathTarget.  Copy the expr for safety.
+      */
+     add_column_to_pathtarget(target, (Expr *) copyObject(item->expr),
+                              item->sortgroupref);
+ }
+
+ /*
+  * Apply add_sp_item_to_pathtarget to each element of list.
+  */
+ static void
+ add_sp_items_to_pathtarget(PathTarget *target, List *items)
+ {
+     ListCell   *lc;
+
+     foreach(lc, items)
+     {
+         split_pathtarget_item *item = lfirst(lc);
+
+         add_sp_item_to_pathtarget(target, item);
+     }
+ }
diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out
index e48e394..cd0b945 100644
*** a/src/test/regress/expected/select_parallel.out
--- b/src/test/regress/expected/select_parallel.out
*************** explain (costs off, verbose)
*** 659,664 ****
--- 659,726 ----
  (11 rows)

  drop function sp_simple_func(integer);
+ -- test handling of SRFs in targetlist (bug in 10.0)
+ explain (costs off)
+    select count(*), generate_series(1,2) from tenk1 group by twenty;
+                         QUERY PLAN
+ ----------------------------------------------------------
+  ProjectSet
+    ->  Finalize GroupAggregate
+          Group Key: twenty
+          ->  Gather Merge
+                Workers Planned: 4
+                ->  Partial GroupAggregate
+                      Group Key: twenty
+                      ->  Sort
+                            Sort Key: twenty
+                            ->  Parallel Seq Scan on tenk1
+ (10 rows)
+
+ select count(*), generate_series(1,2) from tenk1 group by twenty;
+  count | generate_series
+ -------+-----------------
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+    500 |               1
+    500 |               2
+ (40 rows)
+
  -- test gather merge with parallel leader participation disabled
  set parallel_leader_participation = off;
  explain (costs off)
diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index 31045d7..7771e05 100644
*** a/src/test/regress/sql/select_parallel.sql
--- b/src/test/regress/sql/select_parallel.sql
*************** explain (costs off, verbose)
*** 261,266 ****
--- 261,273 ----

  drop function sp_simple_func(integer);

+ -- test handling of SRFs in targetlist (bug in 10.0)
+
+ explain (costs off)
+    select count(*), generate_series(1,2) from tenk1 group by twenty;
+
+ select count(*), generate_series(1,2) from tenk1 group by twenty;
+
  -- test gather merge with parallel leader participation disabled
  set parallel_leader_participation = off;