From 59eb55127906b943ff155240eebc161df8edb62f Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Wed, 27 Apr 2016 07:33:33 -0400 Subject: [PATCH] Fix EXPLAIN VERBOSE output for parallel aggregate. The way that PartialAggregate and FinalizeAggregate plan nodes were displaying output columns before was bogus. Now, FinalizeAggregate produces the same outputs as an Aggregate would have produced, while PartialAggregate produces each of those outputs prefixed by the word PARTIAL. Discussion: 12585.1460737650@sss.pgh.pa.us Patch by me, reviewed by David Rowley. --- src/backend/nodes/copyfuncs.c | 2 + src/backend/nodes/equalfuncs.c | 2 + src/backend/nodes/outfuncs.c | 2 + src/backend/nodes/readfuncs.c | 2 + src/backend/optimizer/plan/setrefs.c | 5 + src/backend/optimizer/util/tlist.c | 3 + src/backend/utils/adt/ruleutils.c | 228 ++++++++++++++++++--------- src/include/nodes/primnodes.h | 2 + 8 files changed, 168 insertions(+), 78 deletions(-) diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index a21928bebe..20e38f09fb 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -1244,6 +1244,8 @@ _copyAggref(const Aggref *from) COPY_NODE_FIELD(aggfilter); COPY_SCALAR_FIELD(aggstar); COPY_SCALAR_FIELD(aggvariadic); + COPY_SCALAR_FIELD(aggpartial); + COPY_SCALAR_FIELD(aggcombine); COPY_SCALAR_FIELD(aggkind); COPY_SCALAR_FIELD(agglevelsup); COPY_LOCATION_FIELD(location); diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c index 3c6c5679b1..c5ccc42dfc 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -202,6 +202,8 @@ _equalAggref(const Aggref *a, const Aggref *b) COMPARE_NODE_FIELD(aggfilter); COMPARE_SCALAR_FIELD(aggstar); COMPARE_SCALAR_FIELD(aggvariadic); + COMPARE_SCALAR_FIELD(aggcombine); + COMPARE_SCALAR_FIELD(aggpartial); COMPARE_SCALAR_FIELD(aggkind); COMPARE_SCALAR_FIELD(agglevelsup); COMPARE_LOCATION_FIELD(location); diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index 5ac74460f2..c2f0e0f8ee 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -1040,6 +1040,8 @@ _outAggref(StringInfo str, const Aggref *node) WRITE_NODE_FIELD(aggfilter); WRITE_BOOL_FIELD(aggstar); WRITE_BOOL_FIELD(aggvariadic); + WRITE_BOOL_FIELD(aggcombine); + WRITE_BOOL_FIELD(aggpartial); WRITE_CHAR_FIELD(aggkind); WRITE_UINT_FIELD(agglevelsup); WRITE_LOCATION_FIELD(location); diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index 8059594d3b..6f28047d84 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -556,6 +556,8 @@ _readAggref(void) READ_NODE_FIELD(aggfilter); READ_BOOL_FIELD(aggstar); READ_BOOL_FIELD(aggvariadic); + READ_BOOL_FIELD(aggcombine); + READ_BOOL_FIELD(aggpartial); READ_CHAR_FIELD(aggkind); READ_UINT_FIELD(agglevelsup); READ_LOCATION_FIELD(location); diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 5537c147ad..266e83055b 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -2100,6 +2100,10 @@ search_indexed_tlist_for_partial_aggref(Aggref *aggref, indexed_tlist *itlist, continue; if (aggref->aggvariadic != tlistaggref->aggvariadic) continue; + /* + * it would be harmless to compare aggcombine and aggpartial, but + * it's also unnecessary + */ if (aggref->aggkind != tlistaggref->aggkind) continue; if (aggref->agglevelsup != tlistaggref->agglevelsup) @@ -2463,6 +2467,7 @@ fix_combine_agg_expr_mutator(Node *node, fix_upper_expr_context *context) newtle = makeTargetEntry((Expr *) newvar, 1, NULL, false); newaggref = (Aggref *) copyObject(aggref); newaggref->args = list_make1(newtle); + newaggref->aggcombine = true; return (Node *) newaggref; } diff --git a/src/backend/optimizer/util/tlist.c b/src/backend/optimizer/util/tlist.c index 4c8c83da80..aa2c2f890c 100644 --- a/src/backend/optimizer/util/tlist.c +++ b/src/backend/optimizer/util/tlist.c @@ -792,6 +792,9 @@ apply_partialaggref_adjustment(PathTarget *target) else newaggref->aggoutputtype = aggform->aggtranstype; + /* flag it as partial */ + newaggref->aggpartial = true; + lfirst(lc) = newaggref; ReleaseSysCache(aggTuple); diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index c1ba3197b2..6dfa6b9a31 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -392,6 +392,11 @@ static void get_rule_windowspec(WindowClause *wc, List *targetList, deparse_context *context); static char *get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context); +static void get_special_variable(Node *node, deparse_context *context, + void *private); +static void resolve_special_varno(Node *node, deparse_context *context, + void *private, + void (*callback) (Node *, deparse_context *, void *)); static Node *find_param_referent(Param *param, deparse_context *context, deparse_namespace **dpns_p, ListCell **ancestor_cell_p); static void get_parameter(Param *param, deparse_context *context); @@ -407,7 +412,10 @@ static void get_rule_expr_toplevel(Node *node, deparse_context *context, static void get_oper_expr(OpExpr *expr, deparse_context *context); static void get_func_expr(FuncExpr *expr, deparse_context *context, bool showimplicit); -static void get_agg_expr(Aggref *aggref, deparse_context *context); +static void get_agg_expr(Aggref *aggref, deparse_context *context, + Aggref *original_aggref); +static void get_agg_combine_expr(Node *node, deparse_context *context, + void *private); static void get_windowfunc_expr(WindowFunc *wfunc, deparse_context *context); static void get_coercion_expr(Node *arg, deparse_context *context, Oid resulttype, int32 resulttypmod, @@ -5877,7 +5885,6 @@ get_utility_query_def(Query *query, deparse_context *context) } } - /* * Display a Var appropriately. * @@ -5930,82 +5937,11 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context) colinfo = deparse_columns_fetch(var->varno, dpns); attnum = var->varattno; } - else if (var->varno == OUTER_VAR && dpns->outer_tlist) - { - TargetEntry *tle; - deparse_namespace save_dpns; - - tle = get_tle_by_resno(dpns->outer_tlist, var->varattno); - if (!tle) - elog(ERROR, "bogus varattno for OUTER_VAR var: %d", var->varattno); - - Assert(netlevelsup == 0); - push_child_plan(dpns, dpns->outer_planstate, &save_dpns); - - /* - * Force parentheses because our caller probably assumed a Var is a - * simple expression. - */ - if (!IsA(tle->expr, Var)) - appendStringInfoChar(buf, '('); - get_rule_expr((Node *) tle->expr, context, true); - if (!IsA(tle->expr, Var)) - appendStringInfoChar(buf, ')'); - - pop_child_plan(dpns, &save_dpns); - return NULL; - } - else if (var->varno == INNER_VAR && dpns->inner_tlist) - { - TargetEntry *tle; - deparse_namespace save_dpns; - - tle = get_tle_by_resno(dpns->inner_tlist, var->varattno); - if (!tle) - elog(ERROR, "bogus varattno for INNER_VAR var: %d", var->varattno); - - Assert(netlevelsup == 0); - push_child_plan(dpns, dpns->inner_planstate, &save_dpns); - - /* - * Force parentheses because our caller probably assumed a Var is a - * simple expression. - */ - if (!IsA(tle->expr, Var)) - appendStringInfoChar(buf, '('); - get_rule_expr((Node *) tle->expr, context, true); - if (!IsA(tle->expr, Var)) - appendStringInfoChar(buf, ')'); - - pop_child_plan(dpns, &save_dpns); - return NULL; - } - else if (var->varno == INDEX_VAR && dpns->index_tlist) - { - TargetEntry *tle; - - tle = get_tle_by_resno(dpns->index_tlist, var->varattno); - if (!tle) - elog(ERROR, "bogus varattno for INDEX_VAR var: %d", var->varattno); - - Assert(netlevelsup == 0); - - /* - * Force parentheses because our caller probably assumed a Var is a - * simple expression. - */ - if (!IsA(tle->expr, Var)) - appendStringInfoChar(buf, '('); - get_rule_expr((Node *) tle->expr, context, true); - if (!IsA(tle->expr, Var)) - appendStringInfoChar(buf, ')'); - - return NULL; - } else { - elog(ERROR, "bogus varno: %d", var->varno); - return NULL; /* keep compiler quiet */ + resolve_special_varno((Node *) var, context, NULL, + get_special_variable); + return NULL; } /* @@ -6118,6 +6054,101 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context) return attname; } +/* + * Deparse a Var which references OUTER_VAR, INNER_VAR, or INDEX_VAR. This + * routine is actually a callback for get_special_varno, which handles finding + * the correct TargetEntry. We get the expression contained in that + * TargetEntry and just need to deparse it, a job we can throw back on + * get_rule_expr. + */ +static void +get_special_variable(Node *node, deparse_context *context, void *private) +{ + StringInfo buf = context->buf; + + /* + * Force parentheses because our caller probably assumed a Var is a simple + * expression. + */ + if (!IsA(node, Var)) + appendStringInfoChar(buf, '('); + get_rule_expr(node, context, true); + if (!IsA(node, Var)) + appendStringInfoChar(buf, ')'); +} + +/* + * Chase through plan references to special varnos (OUTER_VAR, INNER_VAR, + * INDEX_VAR) until we find a real Var or some kind of non-Var node; then, + * invoke the callback provided. + */ +static void +resolve_special_varno(Node *node, deparse_context *context, void *private, + void (*callback) (Node *, deparse_context *, void *)) +{ + Var *var; + deparse_namespace *dpns; + + /* If it's not a Var, invoke the callback. */ + if (!IsA(node, Var)) + { + callback(node, context, private); + return; + } + + /* Find appropriate nesting depth */ + var = (Var *) node; + dpns = (deparse_namespace *) list_nth(context->namespaces, + var->varlevelsup); + + /* + * It's a special RTE, so recurse. + */ + if (var->varno == OUTER_VAR && dpns->outer_tlist) + { + TargetEntry *tle; + deparse_namespace save_dpns; + + tle = get_tle_by_resno(dpns->outer_tlist, var->varattno); + if (!tle) + elog(ERROR, "bogus varattno for OUTER_VAR var: %d", var->varattno); + + push_child_plan(dpns, dpns->outer_planstate, &save_dpns); + resolve_special_varno((Node *) tle->expr, context, private, callback); + pop_child_plan(dpns, &save_dpns); + return; + } + else if (var->varno == INNER_VAR && dpns->inner_tlist) + { + TargetEntry *tle; + deparse_namespace save_dpns; + + tle = get_tle_by_resno(dpns->inner_tlist, var->varattno); + if (!tle) + elog(ERROR, "bogus varattno for INNER_VAR var: %d", var->varattno); + + push_child_plan(dpns, dpns->inner_planstate, &save_dpns); + resolve_special_varno((Node *) tle->expr, context, private, callback); + pop_child_plan(dpns, &save_dpns); + return; + } + else if (var->varno == INDEX_VAR && dpns->index_tlist) + { + TargetEntry *tle; + + tle = get_tle_by_resno(dpns->index_tlist, var->varattno); + if (!tle) + elog(ERROR, "bogus varattno for INDEX_VAR var: %d", var->varattno); + + resolve_special_varno((Node *) tle->expr, context, private, callback); + return; + } + else if (var->varno < 1 || var->varno > list_length(dpns->rtable)) + elog(ERROR, "bogus varno: %d", var->varno); + + /* Not special. Just invoke the callback. */ + callback(node, context, private); +} /* * Get the name of a field of an expression of composite type. The @@ -7080,7 +7111,7 @@ get_rule_expr(Node *node, deparse_context *context, break; case T_Aggref: - get_agg_expr((Aggref *) node, context); + get_agg_expr((Aggref *) node, context, (Aggref *) node); break; case T_GroupingFunc: @@ -8236,13 +8267,36 @@ get_func_expr(FuncExpr *expr, deparse_context *context, * get_agg_expr - Parse back an Aggref node */ static void -get_agg_expr(Aggref *aggref, deparse_context *context) +get_agg_expr(Aggref *aggref, deparse_context *context, + Aggref *original_aggref) { StringInfo buf = context->buf; Oid argtypes[FUNC_MAX_ARGS]; int nargs; bool use_variadic; + /* + * For a combining aggregate, we look up and deparse the corresponding + * partial aggregate instead. This is necessary because our input + * argument list has been replaced; the new argument list always has just + * one element, which will point to a partial Aggref that supplies us with + * transition states to combine. + */ + if (aggref->aggcombine) + { + TargetEntry *tle = linitial(aggref->args); + + Assert(list_length(aggref->args) == 1); + Assert(IsA(tle, TargetEntry)); + resolve_special_varno((Node *) tle->expr, context, original_aggref, + get_agg_combine_expr); + return; + } + + /* Mark as PARTIAL, if appropriate. */ + if (original_aggref->aggpartial) + appendStringInfoString(buf, "PARTIAL "); + /* Extract the argument types as seen by the parser */ nargs = get_aggregate_argtypes(aggref, argtypes); @@ -8311,6 +8365,24 @@ get_agg_expr(Aggref *aggref, deparse_context *context) appendStringInfoChar(buf, ')'); } +/* + * This is a helper function for get_agg_expr(). It's used when we deparse + * a combining Aggref; resolve_special_varno locates the corresponding partial + * Aggref and then calls this. + */ +static void +get_agg_combine_expr(Node *node, deparse_context *context, void *private) +{ + Aggref *aggref; + Aggref *original_aggref = private; + + if (!IsA(node, Aggref)) + elog(ERROR, "combining Aggref does not point to an Aggref"); + + aggref = (Aggref *) node; + get_agg_expr(aggref, context, original_aggref); +} + /* * get_windowfunc_expr - Parse back a WindowFunc node */ diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index 1ffc0a1e5e..a4bc751177 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -280,6 +280,8 @@ typedef struct Aggref bool aggstar; /* TRUE if argument list was really '*' */ bool aggvariadic; /* true if variadic arguments have been * combined into an array last argument */ + bool aggcombine; /* combining agg; input is a transvalue */ + bool aggpartial; /* partial agg; output is a transvalue */ char aggkind; /* aggregate kind (see pg_aggregate.h) */ Index agglevelsup; /* > 0 if agg belongs to outer query */ int location; /* token location, or -1 if unknown */