From 756241e2d3dbc9ffe8933eec21cc0a911c4de6dd Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Sat, 4 Jun 2016 17:05:59 +0300 Subject: [PATCH] Revert contain_vars_of_level() to match the upstream. The function was rewritten in GPDB, and its behaviour was changed to also return 'true' if the expression contains an Aggref of the given level. That change in behaviour was made back in 2006, as part of a commit containing a lot of subquery optimization changes. I could not find an explanation for that particular change, and all the regression tests pass without so I assume that it has become obsolete at some point over they years. This smoothens the way for future merges with upstream, by reducing the diff in both code and behaviour. Also, you get a more accurate error message in a few cases, as seen by the changes to expected output. --- src/backend/optimizer/util/var.c | 67 ++++++++++++++---------- src/test/regress/expected/limit_gp.out | 9 ++++ src/test/regress/expected/percentile.out | 2 +- src/test/regress/sql/limit_gp.sql | 5 ++ 4 files changed, 55 insertions(+), 28 deletions(-) diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c index f529423d73..3b6cd0abe5 100644 --- a/src/backend/optimizer/util/var.c +++ b/src/backend/optimizer/util/var.c @@ -52,6 +52,7 @@ typedef struct } flatten_join_alias_vars_context; static bool contain_var_clause_walker(Node *node, void *context); +static bool contain_vars_of_level_walker(Node *node, int *sublevels_up); static bool pull_var_clause_walker(Node *node, pull_var_clause_context *context); static Node *flatten_join_alias_vars_mutator(Node *node, @@ -273,38 +274,50 @@ contain_var_clause_walker(Node *node, void *context) * * Will recurse into sublinks. Also, may be invoked directly on a Query. */ -static bool -contain_vars_of_level_cbVar(Var *var, void *unused, int sublevelsup) -{ - if ((int)var->varlevelsup == sublevelsup) - return true; /* abort tree traversal and return true */ - return false; -} - -static bool -contain_vars_of_level_cbAggref(Aggref *aggref, void *unused, int sublevelsup) +bool +contain_vars_of_level(Node *node, int levelsup) { - if ((int)aggref->agglevelsup == sublevelsup) - return true; + int sublevels_up = levelsup; - /* visit aggregate's args */ - return cdb_walk_vars((Node *)aggref->args, - contain_vars_of_level_cbVar, - contain_vars_of_level_cbAggref, - NULL, - NULL, - sublevelsup); + return query_or_expression_tree_walker(node, + contain_vars_of_level_walker, + (void *) &sublevels_up, + 0); } -bool -contain_vars_of_level(Node *node, int levelsup) +static bool +contain_vars_of_level_walker(Node *node, int *sublevels_up) { - return cdb_walk_vars(node, - contain_vars_of_level_cbVar, - contain_vars_of_level_cbAggref, - NULL, - NULL, - levelsup); + if (node == NULL) + return false; + if (IsA(node, Var)) + { + if (((Var *) node)->varlevelsup == *sublevels_up) + return true; /* abort tree traversal and return true */ + return false; + } + if (IsA(node, CurrentOfExpr)) + { + if (*sublevels_up == 0) + return true; + return false; + } + if (IsA(node, Query)) + { + /* Recurse into subselects */ + bool result; + + (*sublevels_up)++; + result = query_tree_walker((Query *) node, + contain_vars_of_level_walker, + (void *) sublevels_up, + 0); + (*sublevels_up)--; + return result; + } + return expression_tree_walker(node, + contain_vars_of_level_walker, + (void *) sublevels_up); } /* diff --git a/src/test/regress/expected/limit_gp.out b/src/test/regress/expected/limit_gp.out index 18a6ba5350..6bb554f9f2 100644 --- a/src/test/regress/expected/limit_gp.out +++ b/src/test/regress/expected/limit_gp.out @@ -75,3 +75,12 @@ SELECT dkey, substring(tval from 1 for 2) as str from (SELECT * from mksort_lim (3 rows) DROP TABLE mksort_limit_test_table; +-- Check invalid things in LIMIT +select * from generate_series(1,10) g limit g; +ERROR: argument of LIMIT must not contain variables +LINE 1: select * from generate_series(1,10) g limit g; + ^ +select * from generate_series(1,10) g limit count(*); +ERROR: argument of LIMIT must not contain aggregates +LINE 1: select * from generate_series(1,10) g limit count(*); + ^ diff --git a/src/test/regress/expected/percentile.out b/src/test/regress/expected/percentile.out index 124db1f6ab..fe1583a951 100644 --- a/src/test/regress/expected/percentile.out +++ b/src/test/regress/expected/percentile.out @@ -1006,7 +1006,7 @@ select b, percentile_disc(0.1) within group (order by a) from perct; ERROR: column "perct.b" must appear in the GROUP BY clause or be used in an aggregate function -- nested aggregate select percentile_cont(count(*)) within group (order by a) from perct; -ERROR: argument of percentile function must not contain variables +ERROR: argument of percentile function must not contain aggregates LINE 1: select percentile_cont(count(*)) within group (order by a) f... ^ select sum(percentile_cont(0.22) within group (order by a)) from perct; diff --git a/src/test/regress/sql/limit_gp.sql b/src/test/regress/sql/limit_gp.sql index 8e5c0ab222..9b84d9140b 100644 --- a/src/test/regress/sql/limit_gp.sql +++ b/src/test/regress/sql/limit_gp.sql @@ -27,3 +27,8 @@ SELECT dkey, substring(tval from 1 for 2) as str from (SELECT * from mksort_limi SELECT dkey, substring(tval from 1 for 2) as str from (SELECT * from mksort_limit_test_table ORDER BY dkey DESC LIMIT 5000) as temp ORDER BY jkey DESC LIMIT 3; DROP TABLE mksort_limit_test_table; + +-- Check invalid things in LIMIT + +select * from generate_series(1,10) g limit g; +select * from generate_series(1,10) g limit count(*); -- GitLab