Thread: ERROR: ORDER/GROUP BY expression not found in targetlist
Hi,
Below test case is failing with ERROR: ORDER/GROUP BY expression not found in targetlist. this issue is reproducible from PGv10. postgres=# CREATE TABLE test(c1 int, c2 text, c3 text);
CREATE TABLE
postgres=# INSERT INTO test SELECT i % 10, i % 250, to_char(i % 4, 'FM0000000') FROM GENERATE_SERIES(1,100000,1)i;
INSERT 0 100000
postgres=# ANALYZE test;
ANALYZE
postgres=# EXPLAIN (verbose) SELECT c1, generate_series(1,1), count(*) FROM test GROUP BY 1 HAVING count(*) > 1;
QUERY PLAN
-------------------------------------------------------------------------------
ProjectSet (cost=2291.00..2306.15 rows=3000 width=16)
Output: c1, generate_series(1, 1), (count(*))
-> HashAggregate (cost=2291.00..2291.12 rows=3 width=12)
Output: c1, count(*)
Group Key: test.c1
Filter: (count(*) > 1)
-> Seq Scan on public.test (cost=0.00..1541.00 rows=100000 width=4)
Output: c1, c2, c3
(8 rows)
postgres=# SET min_parallel_table_scan_size=0;
SET
postgres=# EXPLAIN (verbose) SELECT c1, generate_series(1,1), count(*) FROM test GROUP BY 1 HAVING count(*) > 1;
ERROR: ORDER/GROUP BY expression not found in targetlist
Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
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;
Thanks for commit. I have verified reported case. it is fixed now.![](https://ssl.gstatic.com/ui/v1/icons/mail/images/cleardot.gif)
![](https://ssl.gstatic.com/ui/v1/icons/mail/images/cleardot.gif)
Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
On Thu, Jun 21, 2018 at 1:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
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