On Mon, Apr 25, 2016 at 1:03 PM, Robert Haas <robertmh...@gmail.com> wrote:
> Yeah, I'd be happy to have more people chime in. I think your example
> is interesting, but it doesn't persuade me, because the rule has
> always been that EXPLAIN shows the output *columns*, not the output
> *rows*. The disappearance of some rows doesn't change the list of
> output columns. For scans and joins this rule is easy to apply; for
> aggregates, where many rows become one, less so. Some of the existing
> choices there are certainly arguable, like the fact that FILTER is
> shown anywhere at all, which seems like an artifact to me. But I
> think that now is not the time to rethink those decisions.
My proposed fix for this issue is attached. Review is welcome;
otherwise, I'll just commit this. The output looks like what I
suggested upthread:
rhaas=# explain verbose select count(*), corr(aid, bid) from pgbench_accounts ;
QUERY PLAN
--------------------------------------------------------------------------------------------------------------
Finalize Aggregate (cost=742804.22..742804.23 rows=1 width=16)
Output: count(*), corr((aid)::double precision, (bid)::double precision)
-> Gather (cost=742804.00..742804.21 rows=2 width=40)
Output: (PARTIAL count(*)), (PARTIAL corr((aid)::double
precision, (bid)::double precision))
Workers Planned: 2
-> Partial Aggregate (cost=741804.00..741804.01 rows=1 width=40)
Output: PARTIAL count(*), PARTIAL corr((aid)::double
precision, (bid)::double precision)
-> Parallel Seq Scan on public.pgbench_accounts
(cost=0.00..616804.00 rows=12500000 width=8)
Output: aid, bid, abalance, filler
(9 rows)
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index a21928b..20e38f0 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 3c6c567..c5ccc42 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 5ac7446..c2f0e0f 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 8059594..6f28047 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 5537c14..93fe3a3 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,8 @@ 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;
+ newaggref->aggpartial = false;
return (Node *) newaggref;
}
diff --git a/src/backend/optimizer/util/tlist.c b/src/backend/optimizer/util/tlist.c
index 4c8c83d..aa2c2f8 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 1b8f0ae..5a5d4a6 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -392,6 +392,9 @@ 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);
+static void resolve_special_varno(Node *node, deparse_context *context,
+ void (*callback)(Node *, deparse_context *));
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 +410,9 @@ 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,
+ bool allow_partial_decoration);
+static void get_agg_combine_expr(Node *node, deparse_context *context);
static void get_windowfunc_expr(WindowFunc *wfunc, deparse_context *context);
static void get_coercion_expr(Node *arg, deparse_context *context,
Oid resulttype, int32 resulttypmod,
@@ -5864,7 +5869,6 @@ get_utility_query_def(Query *query, deparse_context *context)
}
}
-
/*
* Display a Var appropriately.
*
@@ -5917,82 +5921,10 @@ 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, get_special_variable);
+ return NULL;
}
/*
@@ -6105,6 +6037,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)
+{
+ 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 (*callback)(Node *, deparse_context *))
+{
+ Var *var;
+ deparse_namespace *dpns;
+
+ /* If it's not a Var, invoke the callback. */
+ if (!IsA(node, Var))
+ {
+ callback(node, context);
+ 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, 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, 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, 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);
+}
/*
* Get the name of a field of an expression of composite type. The
@@ -7067,7 +7094,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, true);
break;
case T_GroupingFunc:
@@ -8223,13 +8250,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,
+ bool allow_partial_decoration)
{
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,
+ get_agg_combine_expr);
+ return;
+ }
+
+ /* Decorate partial aggregates, unless suppressed. */
+ if (aggref->aggpartial && allow_partial_decoration)
+ appendStringInfoString(buf, "PARTIAL ");
+
/* Extract the argument types as seen by the parser */
nargs = get_aggregate_argtypes(aggref, argtypes);
@@ -8299,6 +8349,25 @@ get_agg_expr(Aggref *aggref, deparse_context *context)
}
/*
+ * 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)
+{
+ Aggref *aggref;
+
+ if (!IsA(node, Aggref))
+ elog(ERROR, "combining Aggref does not point to an Aggref");
+ aggref = (Aggref *) node;
+ if (!aggref->aggpartial)
+ elog(ERROR, "referenced Aggref is not partial");
+
+ get_agg_expr(aggref, context, false);
+}
+
+/*
* get_windowfunc_expr - Parse back a WindowFunc node
*/
static void
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 1ffc0a1..a4bc751 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 */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
