From 2527110a6e7515b1cbe421db53ecfaa7972ddcc5 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Thu, 7 Feb 2019 09:05:19 +0200 Subject: [PATCH] Fix outer query's target list, when creating a synthetic subquery. When the planner creates a subquery as part of grouping planning, fix the target list of the outer query to correctly refer to the outputs of the subquery. Previously, the parse tree's target list was sometimes left unchanged, so that it still contained Vars referring to the original relations, which have been pushed down to the subquery. After adding the subquery, the outer query only contains a single RTE for the subquery. We usually got away with a bogus target list in the outer query, because we had already decided how to construct the plan, and usually didn't look at the original parse tree anymore. In some cases, though, when the caller asked for it by passing 'use_root=true', we were already doing the right thing, but otherwise we didn't bother. There's little downside to fixing the target list, though, so let's do it unconditionally. Fixes https://github.com/greenplum-db/gpdb/issues/6754 Reviewed-by: Melanie Plageman --- src/backend/cdb/cdbgroup.c | 31 ++++++++++------------- src/backend/optimizer/plan/plangroupext.c | 3 +-- src/include/cdb/cdbgroup.h | 1 - src/test/regress/expected/bfv_olap.out | 18 +++++++++++++ src/test/regress/sql/bfv_olap.sql | 10 ++++++++ 5 files changed, 43 insertions(+), 20 deletions(-) diff --git a/src/backend/cdb/cdbgroup.c b/src/backend/cdb/cdbgroup.c index 9974f3a76d..68c52c59c7 100644 --- a/src/backend/cdb/cdbgroup.c +++ b/src/backend/cdb/cdbgroup.c @@ -1545,7 +1545,6 @@ make_two_stage_agg_plan(PlannerInfo *root, "partial_aggregation", ¤t_pathkeys, result_plan, - !ctx->is_grpext, true); if (ctx->is_grpext) @@ -2247,7 +2246,7 @@ make_plan_for_one_dqa(PlannerInfo *root, MppGroupContext *ctx, int dqa_index, "partial_aggregation", ¤t_pathkeys, result_plan, - true, false); + false); } /* @@ -2324,7 +2323,6 @@ make_plan_for_one_dqa(PlannerInfo *root, MppGroupContext *ctx, int dqa_index, "partial_aggregation", ¤t_pathkeys, result_plan, - true, false); /* Final sort */ @@ -3621,13 +3619,18 @@ deconstruct_agg_info(MppGroupContext *ctx) { TargetEntry *sub_tle, *prelim_tle; + char *resname; sub_tle = get_tle_by_resno(ctx->sub_tlist, ctx->groupColIdx[i]); + + if (sub_tle->resname) + resname = pstrdup(sub_tle->resname); + else + resname = psprintf("prelim_aggref_%d", sub_tle->ressortgroupref); + prelim_tle = makeTargetEntry(copyObject(sub_tle->expr), list_length(ctx->grps_tlist) + 1, - (sub_tle->resname == NULL) ? - NULL : - pstrdup(sub_tle->resname), + resname, false); prelim_tle->ressortgroupref = sub_tle->ressortgroupref; prelim_tle->resjunk = false; @@ -4477,7 +4480,6 @@ add_second_stage_agg(PlannerInfo *root, const char *alias, List **p_current_pathkeys, Plan *result_plan, - bool use_root, bool adjust_scatter) { Query *parse = root->parse; @@ -4539,7 +4541,7 @@ add_second_stage_agg(PlannerInfo *root, tle->resjunk = false; if (tle->resname == NULL) { - if (use_root && IsA(tle->expr, Var)) + if (IsA(tle->expr, Var)) { Var *var = (Var *) tle->expr; RangeTblEntry *rte = rt_fetch(var->varno, root->parse->rtable); @@ -4586,18 +4588,13 @@ add_second_stage_agg(PlannerInfo *root, parse->rowMarks = NIL; /* - * uses parse->targetList to derive the portal's tupDesc, so - * when use_root is true, the caller owns the responsibility to make sure - * it ends up in an appropriate form at the end of planning. + * Set the target list of outer Query tree to match the new range table. */ - if (use_root) + if (adjust_scatter) { - if (adjust_scatter) - { - UpdateScatterClause(parse, upper_tlist); - } - parse->targetList = copyObject(upper_tlist); /* Match range. */ + UpdateScatterClause(parse, upper_tlist); } + parse->targetList = copyObject(upper_tlist); /* Match range. */ result_plan = add_subqueryscan(root, p_current_pathkeys, 1, subquery, result_plan); diff --git a/src/backend/optimizer/plan/plangroupext.c b/src/backend/optimizer/plan/plangroupext.c index c90e4f84d5..3a624c949e 100644 --- a/src/backend/optimizer/plan/plangroupext.c +++ b/src/backend/optimizer/plan/plangroupext.c @@ -891,7 +891,7 @@ make_list_aggs_for_rollup(PlannerInfo *root, context->agg_costs, "rollup", &context->current_pathkeys, - agg_node, false, false); + agg_node, false); /* Set inputHasGrouping */ ((Agg *)agg_node)->inputHasGrouping = true; @@ -1001,7 +1001,6 @@ make_list_aggs_for_rollup(PlannerInfo *root, context->agg_costs, "rollup", &context->current_pathkeys, agg_node, - false, false); if (context->aggstrategy == AGG_HASHED) diff --git a/src/include/cdb/cdbgroup.h b/src/include/cdb/cdbgroup.h index 4db65b2951..317b88160a 100644 --- a/src/include/cdb/cdbgroup.h +++ b/src/include/cdb/cdbgroup.h @@ -64,7 +64,6 @@ extern Plan *add_second_stage_agg(PlannerInfo *root, const char *alias, List **p_current_pathkeys, Plan *result_plan, - bool use_root, bool adjust_scatter); extern List *generate_subquery_tlist(Index varno, List *input_tlist, bool keep_resjunk, int **p_resno_map); diff --git a/src/test/regress/expected/bfv_olap.out b/src/test/regress/expected/bfv_olap.out index 7e67708b52..41b61c39b1 100644 --- a/src/test/regress/expected/bfv_olap.out +++ b/src/test/regress/expected/bfv_olap.out @@ -512,6 +512,24 @@ GROUP BY ROLLUP( (sale.dt,sale.cn),(sale.pn),(sale.vn)); (6 rows) +-- +-- Another ROLLUP query, that hit a bug in setting up the planner-generated +-- subquery's targetlist. (https://github.com/greenplum-db/gpdb/issues/6754) +-- +SELECT sale.vn, rank() over (partition by sale.vn) +FROM vendor, sale +WHERE sale.vn=vendor.vn +GROUP BY ROLLUP( sale.vn); + vn | rank +----+------ + | 1 + 10 | 1 + 20 | 1 + 30 | 1 + 40 | 1 + 50 | 1 +(6 rows) + -- -- Test window function with constant PARTITION BY -- diff --git a/src/test/regress/sql/bfv_olap.sql b/src/test/regress/sql/bfv_olap.sql index 7c2a7f6a80..c52ac5dd35 100644 --- a/src/test/regress/sql/bfv_olap.sql +++ b/src/test/regress/sql/bfv_olap.sql @@ -347,6 +347,16 @@ WHERE sale.vn=vendor.vn GROUP BY ROLLUP( (sale.dt,sale.cn),(sale.pn),(sale.vn)); +-- +-- Another ROLLUP query, that hit a bug in setting up the planner-generated +-- subquery's targetlist. (https://github.com/greenplum-db/gpdb/issues/6754) +-- +SELECT sale.vn, rank() over (partition by sale.vn) +FROM vendor, sale +WHERE sale.vn=vendor.vn +GROUP BY ROLLUP( sale.vn); + + -- -- Test window function with constant PARTITION BY -- -- GitLab