Rethinking representation of partial-aggregate steps
I really dislike this aspect of the partial-aggregate patch:
typedef struct Agg
{
...
bool combineStates; /* input tuples contain transition states */
bool finalizeAggs; /* should we call the finalfn on agg states? */
bool serialStates; /* should agg states be (de)serialized? */
...
} Agg;
Representing the various partial-aggregate behaviors as three bools seems
like a bad idea to me. In the first place, this makes it look like there
are as many as eight possible behaviors, whereas only a subset of those
flag combinations are or ever will be supported (not that the comments
anywhere tell you that, much less which ones). In the second place,
it leads to unreadable code like this:
/* partial phase */
count_agg_clauses(root, (Node *) partial_grouping_target->exprs,
&agg_partial_costs, false, false, true);
/* final phase */
count_agg_clauses(root, (Node *) target->exprs, &agg_final_costs,
true, true, true);
count_agg_clauses(root, parse->havingQual, &agg_final_costs, true,
true, true);
Good luck telling whether those falses and trues actually do what they
should.
I'm inclined to think that we should aggressively simplify here, perhaps
replacing all three of these flags with a three-way enum, say
PARTIALAGG_NORMAL /* simple aggregation */
PARTIALAGG_PARTIAL /* lower phase of partial aggregation */
PARTIALAGG_FINAL /* upper phase of partial aggregation */
This embodies a couple of judgments that maybe someone would quibble with:
* we don't have any use for intermediate partial aggregation steps;
* we don't have any use for partial aggregation in which data transferred
up needn't be serialized.
But I can't really see how either of those would make sense for any
practical use-case, and I don't think we need to have a confusing and
bug-prone API in order to hold the door open to support them.
Comments?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 23, 2016 at 12:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I really dislike this aspect of the partial-aggregate patch:
typedef struct Agg
{
...
bool combineStates; /* input tuples contain transition states */
bool finalizeAggs; /* should we call the finalfn on agg states? */
bool serialStates; /* should agg states be (de)serialized? */
...
} Agg;Representing the various partial-aggregate behaviors as three bools seems
like a bad idea to me. In the first place, this makes it look like there
are as many as eight possible behaviors, whereas only a subset of those
flag combinations are or ever will be supported (not that the comments
anywhere tell you that, much less which ones). In the second place,
it leads to unreadable code like this:/* partial phase */
count_agg_clauses(root, (Node *) partial_grouping_target->exprs,
&agg_partial_costs, false, false, true);/* final phase */
count_agg_clauses(root, (Node *) target->exprs, &agg_final_costs,
true, true, true);
count_agg_clauses(root, parse->havingQual, &agg_final_costs, true,
true, true);Good luck telling whether those falses and trues actually do what they
should.I'm inclined to think that we should aggressively simplify here, perhaps
replacing all three of these flags with a three-way enum, sayPARTIALAGG_NORMAL /* simple aggregation */
PARTIALAGG_PARTIAL /* lower phase of partial aggregation */
PARTIALAGG_FINAL /* upper phase of partial aggregation */This embodies a couple of judgments that maybe someone would quibble with:
* we don't have any use for intermediate partial aggregation steps;
* we don't have any use for partial aggregation in which data transferred
up needn't be serialized.But I can't really see how either of those would make sense for any
practical use-case, and I don't think we need to have a confusing and
bug-prone API in order to hold the door open to support them.
Hmm, well I guess I would have to disagree with the idea that those
other modes aren't useful. I seem to recall that David had some
performance results showing that pushing partial aggregate steps below
an Append node resulted in a noticeable speed-up even in the absence
of any parallelism, presumably because it avoids whatever projection
the Append might do, and also improves icache and dcache locality. So
it seems quite possible that you might want to have something like
this:
FinalizeAggregate
-> Gather
-> IntermediateAggregate
-> Append
-> PartialAggregate
-> Parallel SeqScan
-> PartialAggregate
-> Parallel SeqScan
-> PartialAggregate
-> Parallel SeqScan
-> PartialAggregate
-> Parallel SeqScan
The partial aggregate plans need not serialize, because they are
passing data to the same process, and the intermediate aggregate is
there, too.
I do agree, however, that the three Boolean flags don't make the code
entirely easy to read. What I might suggest is that we replace the
three Boolean flags with integer flags, something like this:
#define AGGOP_COMBINESTATES 0x1
#define AGGOP_SERIALIZESTATES 0x2
#define AGGOP_FINALIZEAGGS 0x4
#define AGGTYPE_SIMPLE (AGGOP_FINALIZEAGGS)
#define AGGTYPE_PARTIAL_SERIAL (AGGOP_SERIALIZESTATES)
#define AGGTYPE_FINALIZE_SERIAL
(AGGOP_COMBINESTATES|AGGOP_FINALIZEAGGS|AGG_SERIALIZESTATES)
Code that requests behavior can do so with the AGGTYPE_* constants,
but code that implements behavior can test AGGOP_* constants. Then if
we decide we need new combinations in the future, we can just add
those --- e.g. AGGTYPE_PARTIAL = 0, AGGTYPE_FINALIZE =
AGGOP_COMBINESTATES|AGGOP_FINALIZEAGGS, AGGTYPE_INTERMEDIATE =
AGGOP_COMBINESTATES --- and have some hope that it will mostly work.
I actually think nearly all of the combinations are sensible, here,
although I think it may have been a mistake to overload the "serialize
states" flag to mean both "does this combining aggregate expect to
receive the serial type rather than the transition type?" and also
"does this partial aggregate output the serial type rather than the
transition type?". In the example above, you'd want the
IntermediateAggregate to expect the transition type but output the
serial type. Oops.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Thu, Jun 23, 2016 at 12:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'm inclined to think that we should aggressively simplify here, perhaps
replacing all three of these flags with a three-way enum, sayPARTIALAGG_NORMAL /* simple aggregation */
PARTIALAGG_PARTIAL /* lower phase of partial aggregation */
PARTIALAGG_FINAL /* upper phase of partial aggregation */This embodies a couple of judgments that maybe someone would quibble with:
* we don't have any use for intermediate partial aggregation steps;
* we don't have any use for partial aggregation in which data transferred
up needn't be serialized.
Hmm, well I guess I would have to disagree with the idea that those
other modes aren't useful. I seem to recall that David had some
performance results showing that pushing partial aggregate steps below
an Append node resulted in a noticeable speed-up even in the absence
of any parallelism, presumably because it avoids whatever projection
the Append might do, and also improves icache and dcache locality.
I don't believe that for one second, because introducing another layer of
intermediate aggregation implies another projection step, plus all the
other overhead of a Plan node.
Also, if we are going to support intermediate partial aggregation, the
current representation is broken anyway, since it lacks any ability
to distinguish serialization for the input states from serialization
for the output states, which is something you'd certainly need to
distinguish in this type of plan structure.
I do agree, however, that the three Boolean flags don't make the code
entirely easy to read. What I might suggest is that we replace the
three Boolean flags with integer flags, something like this:
Yeah, that's another way we could go. I had been considering a variant
of that, which was to assign specific code values to the enum constants
and then invent macros that did bit-anding tests on them. That ends up
being just about what you propose except that the compiler understands
the enum-ness of the behavioral alternatives, which seems like a good
thing.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 23, 2016 at 1:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Thu, Jun 23, 2016 at 12:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'm inclined to think that we should aggressively simplify here, perhaps
replacing all three of these flags with a three-way enum, sayPARTIALAGG_NORMAL /* simple aggregation */
PARTIALAGG_PARTIAL /* lower phase of partial aggregation */
PARTIALAGG_FINAL /* upper phase of partial aggregation */This embodies a couple of judgments that maybe someone would quibble with:
* we don't have any use for intermediate partial aggregation steps;
* we don't have any use for partial aggregation in which data transferred
up needn't be serialized.Hmm, well I guess I would have to disagree with the idea that those
other modes aren't useful. I seem to recall that David had some
performance results showing that pushing partial aggregate steps below
an Append node resulted in a noticeable speed-up even in the absence
of any parallelism, presumably because it avoids whatever projection
the Append might do, and also improves icache and dcache locality.I don't believe that for one second, because introducing another layer of
intermediate aggregation implies another projection step, plus all the
other overhead of a Plan node.
Sure, but aggregating as early as possible will often have the effect
of dramatically reducing the number of tuples that have to pass
through upper levels of the plan tree, which seems it would frequently
far outweigh those considerations. Anyway, you can go back and find
what he posted about this previously and look at it for yourself;
that's probably better than having an argument with me about his test
results.
Also, if we are going to support intermediate partial aggregation, the
current representation is broken anyway, since it lacks any ability
to distinguish serialization for the input states from serialization
for the output states, which is something you'd certainly need to
distinguish in this type of plan structure.
I mentioned that in the very same email to which you were replying
when you wrote this, which makes me wonder whether you bothered to
read the whole thing.
I do agree, however, that the three Boolean flags don't make the code
entirely easy to read. What I might suggest is that we replace the
three Boolean flags with integer flags, something like this:Yeah, that's another way we could go. I had been considering a variant
of that, which was to assign specific code values to the enum constants
and then invent macros that did bit-anding tests on them. That ends up
being just about what you propose except that the compiler understands
the enum-ness of the behavioral alternatives, which seems like a good
thing.
I don't object to that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
I do agree, however, that the three Boolean flags don't make the code
entirely easy to read. What I might suggest is that we replace the
three Boolean flags with integer flags, something like this:Yeah, that's another way we could go. I had been considering a variant
of that, which was to assign specific code values to the enum constants
and then invent macros that did bit-anding tests on them. That ends up
being just about what you propose except that the compiler understands
the enum-ness of the behavioral alternatives, which seems like a good
thing.
Isn't that what you said not to do in
/messages/by-id/13345.1462383078@sss.pgh.pa.us ?
ISTM that in Robert's proposal it is allowed for a "flags" value to
have an OR'ed combination of multiple individual flags. Unless you're
proposing to enumerate each allowed combination?
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 23 June 2016 at 18:31, Robert Haas <robertmhaas@gmail.com> wrote:
Sure, but aggregating as early as possible will often have the effect
of dramatically reducing the number of tuples that have to pass
through upper levels of the plan tree, which seems it would frequently
far outweigh those considerations.
Agreed
I can imagine plans where a useful aggregation occurs before every join, so
N > 2 is easily possible.
--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Tom Lane wrote:
Yeah, that's another way we could go. I had been considering a variant
of that, which was to assign specific code values to the enum constants
and then invent macros that did bit-anding tests on them. That ends up
being just about what you propose except that the compiler understands
the enum-ness of the behavioral alternatives, which seems like a good
thing.
Isn't that what you said not to do in
/messages/by-id/13345.1462383078@sss.pgh.pa.us ?
No. What I'm imagining is, say,
#define AGGOP_COMBINESTATES 0x1
#define AGGOP_SERIALIZESTATES 0x2
#define AGGOP_DESERIALIZESTATES 0x4
#define AGGOP_FINALIZEAGGS 0x8
typedef enum AggPartialMode
{
AGGPARTIAL_SIMPLE = AGGOP_FINALIZEAGGS,
AGGPARTIAL_PARTIAL = AGGOP_SERIALIZESTATES,
AGGPARTIAL_FINAL = AGGOP_COMBINESTATES | AGGOP_DESERIALIZESTATES | AGGOP_FINALIZEAGGS
} AggPartialMode;
#define DO_AGGPARTIAL_COMBINE(apm) (((apm) & AGGOP_COMBINESTATES) != 0)
#define DO_AGGPARTIAL_SERIALIZE(apm) (((apm) & AGGOP_SERIALIZESTATES) != 0)
#define DO_AGGPARTIAL_DESERIALIZE(apm) (((apm) & AGGOP_DESERIALIZESTATES) != 0)
#define DO_AGGPARTIAL_FINALIZE(apm) (((apm) & AGGOP_FINALIZEAGGS) != 0)
These enum constants satisfy the properties I mentioned before, but their
assigned values are chosen to make the macros cheap.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom Lane wrote:
What I'm imagining is, say,
#define AGGOP_COMBINESTATES 0x1
#define AGGOP_SERIALIZESTATES 0x2
#define AGGOP_DESERIALIZESTATES 0x4
#define AGGOP_FINALIZEAGGS 0x8typedef enum AggPartialMode
{
AGGPARTIAL_SIMPLE = AGGOP_FINALIZEAGGS,
AGGPARTIAL_PARTIAL = AGGOP_SERIALIZESTATES,
AGGPARTIAL_FINAL = AGGOP_COMBINESTATES | AGGOP_DESERIALIZESTATES | AGGOP_FINALIZEAGGS
} AggPartialMode;#define DO_AGGPARTIAL_COMBINE(apm) (((apm) & AGGOP_COMBINESTATES) != 0)
#define DO_AGGPARTIAL_SERIALIZE(apm) (((apm) & AGGOP_SERIALIZESTATES) != 0)
#define DO_AGGPARTIAL_DESERIALIZE(apm) (((apm) & AGGOP_DESERIALIZESTATES) != 0)
#define DO_AGGPARTIAL_FINALIZE(apm) (((apm) & AGGOP_FINALIZEAGGS) != 0)These enum constants satisfy the properties I mentioned before, but their
assigned values are chosen to make the macros cheap.
Ah, sure, that makes sense.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 24 June 2016 at 05:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
Hmm, well I guess I would have to disagree with the idea that those
other modes aren't useful. I seem to recall that David had some
performance results showing that pushing partial aggregate steps below
an Append node resulted in a noticeable speed-up even in the absence
of any parallelism, presumably because it avoids whatever projection
the Append might do, and also improves icache and dcache locality.I don't believe that for one second, because introducing another layer of
intermediate aggregation implies another projection step, plus all the
other overhead of a Plan node.
It's certainly not difficult to mock up a test to prove it is faster.
create table t1 (a int not null);
insert into t1 select generate_series(1,1000000);
create table t2 (a int not null);
insert into t2 select generate_series(1,1000000);
select sum(c) from (select count(*) c from t1 union all select
count(*) from t2) t;
sum
---------
2000000
(1 row)
Time: 82.038 ms
select count(*) from (select * from t1 union all select * from t2) t;
count
---------
2000000
(1 row)
Time: 180.540 ms
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 24 June 2016 at 05:12, Robert Haas <robertmhaas@gmail.com> wrote:
I do agree, however, that the three Boolean flags don't make the code
entirely easy to read. What I might suggest is that we replace the
three Boolean flags with integer flags, something like this:#define AGGOP_COMBINESTATES 0x1
#define AGGOP_SERIALIZESTATES 0x2
#define AGGOP_FINALIZEAGGS 0x4#define AGGTYPE_SIMPLE (AGGOP_FINALIZEAGGS)
#define AGGTYPE_PARTIAL_SERIAL (AGGOP_SERIALIZESTATES)
#define AGGTYPE_FINALIZE_SERIAL
(AGGOP_COMBINESTATES|AGGOP_FINALIZEAGGS|AGG_SERIALIZESTATES)Code that requests behavior can do so with the AGGTYPE_* constants,
but code that implements behavior can test AGGOP_* constants. Then if
we decide we need new combinations in the future, we can just add
those --- e.g. AGGTYPE_PARTIAL = 0, AGGTYPE_FINALIZE =
AGGOP_COMBINESTATES|AGGOP_FINALIZEAGGS, AGGTYPE_INTERMEDIATE =
AGGOP_COMBINESTATES --- and have some hope that it will mostly work.I actually think nearly all of the combinations are sensible, here,
although I think it may have been a mistake to overload the "serialize
states" flag to mean both "does this combining aggregate expect to
receive the serial type rather than the transition type?" and also
"does this partial aggregate output the serial type rather than the
transition type?". In the example above, you'd want the
IntermediateAggregate to expect the transition type but output the
serial type. Oops.
I did consider using bit flags for this before, and it's a little bit
of an accident that it didn't end up that way. Originally there were
just two bool flags to control finalize and combine. A later patch
added the serialisation stuff which required the 3rd flag. It then
became a little untidy, but I hesitated to change it as I really just
wanted to keep the patch as easy to review as possible. In hindsight,
if I should've probably used bit flags from day one.
As for the above proposal, I do agree that it'll be cleaner with bit
flags, I just really don't see the need for the AGGTYPE_* alias
macros. For me it's easier to read if each option is explicitly listed
similar to how pull_var_clause() is done, e.g:
List *vars = pull_var_clause((Node *) cur_em->em_expr,
PVC_RECURSE_AGGREGATES |
PVC_RECURSE_WINDOWFUNCS |
PVC_INCLUDE_PLACEHOLDERS);
I'll start by writing the code to do that much, and if the consensus
is to add the alias macros too, then it's a small addition.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
David Rowley <david.rowley@2ndquadrant.com> writes:
As for the above proposal, I do agree that it'll be cleaner with bit
flags, I just really don't see the need for the AGGTYPE_* alias
macros. For me it's easier to read if each option is explicitly listed
similar to how pull_var_clause() is done, e.g:
That does not sound to me like it does anything to address the issue of
documenting which combinations of flags are sensible/supported. I prefer
something like the enum approach I suggested further downthread.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 24 June 2016 at 05:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
#define AGGOP_COMBINESTATES 0x1
#define AGGOP_SERIALIZESTATES 0x2
#define AGGOP_DESERIALIZESTATES 0x4
#define AGGOP_FINALIZEAGGS 0x8typedef enum AggPartialMode
{
AGGPARTIAL_SIMPLE = AGGOP_FINALIZEAGGS,
AGGPARTIAL_PARTIAL = AGGOP_SERIALIZESTATES,
AGGPARTIAL_FINAL = AGGOP_COMBINESTATES | AGGOP_DESERIALIZESTATES | AGGOP_FINALIZEAGGS
} AggPartialMode;#define DO_AGGPARTIAL_COMBINE(apm) (((apm) & AGGOP_COMBINESTATES) != 0)
#define DO_AGGPARTIAL_SERIALIZE(apm) (((apm) & AGGOP_SERIALIZESTATES) != 0)
#define DO_AGGPARTIAL_DESERIALIZE(apm) (((apm) & AGGOP_DESERIALIZESTATES) != 0)
#define DO_AGGPARTIAL_FINALIZE(apm) (((apm) & AGGOP_FINALIZEAGGS) != 0)
The attached implements this, with the exception that I didn't really
think AggPartialMode was the best name for the enum. I've named this
AggregateMode instead, as the aggregate is only partial in some cases.
I also appended _SERIAL and _DESERIAL to the end of the partial and
final modes for now, as likely in 10.0 we'll be performing some
partial non-serialized aggregation and we'll likely want to keep those
other names for that instead.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
aggmode_bitflags.patchapplication/octet-stream; name=aggmode_bitflags.patchDownload
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 379fc5c..b894454 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -947,9 +947,9 @@ ExplainNode(PlanState *planstate, List *ancestors,
{
Agg *agg = (Agg *) plan;
- if (agg->finalizeAggs == false)
+ if (!DO_AGGMODE_FINALIZE(agg->aggmode))
operation = "Partial";
- else if (agg->combineStates == true)
+ else if (DO_AGGMODE_COMBINE(agg->aggmode))
operation = "Finalize";
switch (agg->aggstrategy)
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index 01e04d3..5b9b11a 100644
--- a/src/backend/executor/execQual.c
+++ b/src/backend/executor/execQual.c
@@ -4519,7 +4519,7 @@ ExecInitExpr(Expr *node, PlanState *parent)
/* planner messed up */
elog(ERROR, "Aggref found in non-Agg plan node");
}
- if (aggref->aggpartial == aggstate->finalizeAggs)
+ if (aggref->aggpartial == DO_AGGMODE_FINALIZE(aggstate->aggmode))
{
/* planner messed up */
if (aggref->aggpartial)
@@ -4528,7 +4528,7 @@ ExecInitExpr(Expr *node, PlanState *parent)
elog(ERROR, "non-partial Aggref found in non-finalize agg plan node");
}
- if (aggref->aggcombine != aggstate->combineStates)
+ if (aggref->aggcombine != DO_AGGMODE_COMBINE(aggstate->aggmode))
{
/* planner messed up */
if (aggref->aggcombine)
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index a447964..8d845f7 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -1577,7 +1577,7 @@ finalize_aggregates(AggState *aggstate,
pergroupstate);
}
- if (aggstate->finalizeAggs)
+ if (DO_AGGMODE_FINALIZE(aggstate->aggmode))
finalize_aggregate(aggstate, peragg, pergroupstate,
&aggvalues[aggno], &aggnulls[aggno]);
else
@@ -2114,7 +2114,7 @@ agg_retrieve_direct(AggState *aggstate)
*/
for (;;)
{
- if (!aggstate->combineStates)
+ if (!DO_AGGMODE_COMBINE(aggstate->aggmode))
advance_aggregates(aggstate, pergroup);
else
combine_aggregates(aggstate, pergroup);
@@ -2225,7 +2225,7 @@ agg_fill_hash_table(AggState *aggstate)
entry = lookup_hash_entry(aggstate, outerslot);
/* Advance the aggregates */
- if (!aggstate->combineStates)
+ if (!DO_AGGMODE_COMBINE(aggstate->aggmode))
advance_aggregates(aggstate, entry->pergroup);
else
combine_aggregates(aggstate, entry->pergroup);
@@ -2360,9 +2360,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
aggstate->pertrans = NULL;
aggstate->curpertrans = NULL;
aggstate->agg_done = false;
- aggstate->combineStates = node->combineStates;
- aggstate->finalizeAggs = node->finalizeAggs;
- aggstate->serialStates = node->serialStates;
+ aggstate->aggmode = node->aggmode;
aggstate->input_done = false;
aggstate->pergroup = NULL;
aggstate->grp_firstTuple = NULL;
@@ -2724,7 +2722,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
* If this aggregation is performing state combines, then instead of
* using the transition function, we'll use the combine function
*/
- if (aggstate->combineStates)
+ if (DO_AGGMODE_COMBINE(aggstate->aggmode))
{
transfn_oid = aggform->aggcombinefn;
@@ -2736,7 +2734,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
transfn_oid = aggform->aggtransfn;
/* Final function only required if we're finalizing the aggregates */
- if (aggstate->finalizeAggs)
+ if (DO_AGGMODE_FINALIZE(aggstate->aggmode))
peragg->finalfn_oid = finalfn_oid = aggform->aggfinalfn;
else
peragg->finalfn_oid = finalfn_oid = InvalidOid;
@@ -2745,30 +2743,38 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
deserialfn_oid = InvalidOid;
/*
- * Determine if we require serialization or deserialization of the
- * aggregate states. This is only required if the aggregate state is
- * internal.
+ * Check if serial/deserialization is required. We only do this
+ * when the transtype is INTERNAL.
*/
- if (aggstate->serialStates && aggtranstype == INTERNALOID)
+ if (aggtranstype == INTERNALOID)
{
/*
- * The planner should only have generated an agg node with
- * serialStates if every aggregate with an INTERNAL state has
- * serialization/deserialization functions. Verify that.
+ * The planner should only have generated a serialize agg node if
+ * every aggregate with an INTERNAL state has
+ * serialization function. Verify that.
*/
- if (!OidIsValid(aggform->aggserialfn))
- elog(ERROR, "serialfunc not set during serialStates aggregation step");
+ if (DO_AGGMODE_SERIALIZE(aggstate->aggmode))
+ {
+ if (!OidIsValid(aggform->aggserialfn))
+ elog(ERROR, "serialfunc not set during serialization aggregation");
- if (!OidIsValid(aggform->aggdeserialfn))
- elog(ERROR, "deserialfunc not set during serialStates aggregation step");
+ /* serialization only valid when not in finalize mode */
+ Assert(!DO_AGGMODE_FINALIZE(aggstate->aggmode));
- /* serialization func only required when not finalizing aggs */
- if (!aggstate->finalizeAggs)
serialfn_oid = aggform->aggserialfn;
+ }
+
+ /* Likewise for deserialization functions */
+ if (DO_AGGMODE_DESERIALIZE(aggstate->aggmode))
+ {
+ if (!OidIsValid(aggform->aggdeserialfn))
+ elog(ERROR, "deserialfunc not set during deserialization aggregation");
+
+ /* deserialization only valid when combining states */
+ Assert(DO_AGGMODE_COMBINE(aggstate->aggmode));
- /* deserialization func only required when combining states */
- if (aggstate->combineStates)
deserialfn_oid = aggform->aggdeserialfn;
+ }
}
/* Check that aggregate owner has permission to call component fns */
@@ -2972,7 +2978,7 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
* transfn and transfn_oid fields of pertrans refer to the combine
* function rather than the transition function.
*/
- if (aggstate->combineStates)
+ if (DO_AGGMODE_COMBINE(aggstate->aggmode))
{
Expr *combinefnexpr;
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 59add5b..f1319d7 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -870,9 +870,7 @@ _copyAgg(const Agg *from)
CopyPlanFields((const Plan *) from, (Plan *) newnode);
COPY_SCALAR_FIELD(aggstrategy);
- COPY_SCALAR_FIELD(combineStates);
- COPY_SCALAR_FIELD(finalizeAggs);
- COPY_SCALAR_FIELD(serialStates);
+ COPY_SCALAR_FIELD(aggmode);
COPY_SCALAR_FIELD(numCols);
if (from->numCols > 0)
{
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index b0a28f5..e81c69a 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -705,9 +705,7 @@ _outAgg(StringInfo str, const Agg *node)
_outPlanInfo(str, (const Plan *) node);
WRITE_ENUM_FIELD(aggstrategy, AggStrategy);
- WRITE_BOOL_FIELD(combineStates);
- WRITE_BOOL_FIELD(finalizeAggs);
- WRITE_BOOL_FIELD(serialStates);
+ WRITE_ENUM_FIELD(aggmode, AggregateMode);
WRITE_INT_FIELD(numCols);
appendStringInfoString(str, " :grpColIdx");
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index b1f9e3e..72a8804 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1989,9 +1989,7 @@ _readAgg(void)
ReadCommonPlan(&local_node->plan);
READ_ENUM_FIELD(aggstrategy, AggStrategy);
- READ_BOOL_FIELD(combineStates);
- READ_BOOL_FIELD(finalizeAggs);
- READ_BOOL_FIELD(serialStates);
+ READ_ENUM_FIELD(aggmode, AggregateMode);
READ_INT_FIELD(numCols);
READ_ATTRNUMBER_ARRAY(grpColIdx, local_node->numCols);
READ_OID_ARRAY(grpOperators, local_node->numCols);
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index b2db6e8..315d4ea 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -1304,9 +1304,7 @@ create_unique_plan(PlannerInfo *root, UniquePath *best_path, int flags)
plan = (Plan *) make_agg(build_path_tlist(root, &best_path->path),
NIL,
AGG_HASHED,
- false,
- true,
- false,
+ AGGMODE_SIMPLE,
numGroupCols,
groupColIdx,
groupOperators,
@@ -1610,9 +1608,7 @@ create_agg_plan(PlannerInfo *root, AggPath *best_path)
plan = make_agg(tlist, quals,
best_path->aggstrategy,
- best_path->combineStates,
- best_path->finalizeAggs,
- best_path->serialStates,
+ best_path->aggmode,
list_length(best_path->groupClause),
extract_grouping_cols(best_path->groupClause,
subplan->targetlist),
@@ -1765,9 +1761,7 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path)
agg_plan = (Plan *) make_agg(NIL,
NIL,
AGG_SORTED,
- false,
- true,
- false,
+ AGGMODE_SIMPLE,
list_length((List *) linitial(gsets)),
new_grpColIdx,
extract_grouping_ops(groupClause),
@@ -1802,9 +1796,7 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path)
plan = make_agg(build_path_tlist(root, &best_path->path),
best_path->qual,
(numGroupCols > 0) ? AGG_SORTED : AGG_PLAIN,
- false,
- true,
- false,
+ AGGMODE_SIMPLE,
numGroupCols,
top_grpColIdx,
extract_grouping_ops(groupClause),
@@ -5653,9 +5645,8 @@ materialize_finished_plan(Plan *subplan)
Agg *
make_agg(List *tlist, List *qual,
AggStrategy aggstrategy,
- bool combineStates, bool finalizeAggs, bool serialStates,
- int numGroupCols, AttrNumber *grpColIdx, Oid *grpOperators,
- List *groupingSets, List *chain,
+ AggregateMode aggmode, int numGroupCols, AttrNumber *grpColIdx,
+ Oid *grpOperators, List *groupingSets, List *chain,
double dNumGroups, Plan *lefttree)
{
Agg *node = makeNode(Agg);
@@ -5666,9 +5657,7 @@ make_agg(List *tlist, List *qual,
numGroups = (long) Min(dNumGroups, (double) LONG_MAX);
node->aggstrategy = aggstrategy;
- node->combineStates = combineStates;
- node->finalizeAggs = finalizeAggs;
- node->serialStates = serialStates;
+ node->aggmode = aggmode;
node->numCols = numGroupCols;
node->grpColIdx = grpColIdx;
node->grpOperators = grpOperators;
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 2372311..5f4a4b8 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -3390,10 +3390,10 @@ create_grouping_paths(PlannerInfo *root,
MemSet(&agg_costs, 0, sizeof(AggClauseCosts));
if (parse->hasAggs)
{
- count_agg_clauses(root, (Node *) target->exprs, &agg_costs, true,
- false, false);
- count_agg_clauses(root, parse->havingQual, &agg_costs, true, false,
- false);
+ count_agg_clauses(root, (Node *) target->exprs, &agg_costs,
+ AGGMODE_SIMPLE);
+ count_agg_clauses(root, parse->havingQual, &agg_costs,
+ AGGMODE_SIMPLE);
}
/*
@@ -3479,13 +3479,13 @@ create_grouping_paths(PlannerInfo *root,
{
/* partial phase */
count_agg_clauses(root, (Node *) partial_grouping_target->exprs,
- &agg_partial_costs, false, false, true);
+ &agg_partial_costs, AGGMODE_PARTIAL_SERIAL);
/* final phase */
count_agg_clauses(root, (Node *) target->exprs, &agg_final_costs,
- true, true, true);
- count_agg_clauses(root, parse->havingQual, &agg_final_costs, true,
- true, true);
+ AGGMODE_FINAL_DESERIAL);
+ count_agg_clauses(root, parse->havingQual, &agg_final_costs,
+ AGGMODE_FINAL_DESERIAL);
}
if (can_sort)
@@ -3525,9 +3525,7 @@ create_grouping_paths(PlannerInfo *root,
NIL,
&agg_partial_costs,
dNumPartialGroups,
- false,
- false,
- true));
+ AGGMODE_PARTIAL_SERIAL));
else
add_partial_path(grouped_rel, (Path *)
create_group_path(root,
@@ -3567,9 +3565,7 @@ create_grouping_paths(PlannerInfo *root,
NIL,
&agg_partial_costs,
dNumPartialGroups,
- false,
- false,
- true));
+ AGGMODE_PARTIAL_SERIAL));
}
}
}
@@ -3632,9 +3628,7 @@ create_grouping_paths(PlannerInfo *root,
(List *) parse->havingQual,
&agg_costs,
dNumGroups,
- false,
- true,
- false));
+ AGGMODE_SIMPLE));
}
else if (parse->groupClause)
{
@@ -3699,9 +3693,7 @@ create_grouping_paths(PlannerInfo *root,
(List *) parse->havingQual,
&agg_final_costs,
dNumGroups,
- true,
- true,
- true));
+ AGGMODE_FINAL_DESERIAL));
else
add_path(grouped_rel, (Path *)
create_group_path(root,
@@ -3742,9 +3734,7 @@ create_grouping_paths(PlannerInfo *root,
(List *) parse->havingQual,
&agg_costs,
dNumGroups,
- false,
- true,
- false));
+ AGGMODE_SIMPLE));
}
/*
@@ -3781,9 +3771,7 @@ create_grouping_paths(PlannerInfo *root,
(List *) parse->havingQual,
&agg_final_costs,
dNumGroups,
- true,
- true,
- true));
+ AGGMODE_FINAL_DESERIAL));
}
}
}
@@ -4125,9 +4113,7 @@ create_distinct_paths(PlannerInfo *root,
NIL,
NULL,
numDistinctRows,
- false,
- true,
- false));
+ AGGMODE_SIMPLE));
}
/* Give a helpful error if we failed to find any implementation */
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 17edc27..eb7f311 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -680,7 +680,7 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
{
Agg *aggplan = (Agg *) plan;
- if (aggplan->combineStates)
+ if (DO_AGGMODE_COMBINE(aggplan->aggmode))
set_combineagg_references(root, plan, rtoffset);
else
set_upper_references(root, plan, rtoffset);
diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
index 552b756..75b6f36 100644
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -865,9 +865,7 @@ make_union_unique(SetOperationStmt *op, Path *path, List *tlist,
NIL,
NULL,
dNumGroups,
- false,
- true,
- false);
+ AGGMODE_SIMPLE);
}
else
{
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 7138cad..22877d8 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -61,9 +61,7 @@ typedef struct
{
PlannerInfo *root;
AggClauseCosts *costs;
- bool finalizeAggs;
- bool combineStates;
- bool serialStates;
+ AggregateMode aggmode;
} count_agg_clauses_context;
typedef struct
@@ -543,15 +541,13 @@ contain_agg_clause_walker(Node *node, void *context)
*/
void
count_agg_clauses(PlannerInfo *root, Node *clause, AggClauseCosts *costs,
- bool finalizeAggs, bool combineStates, bool serialStates)
+ AggregateMode aggmode)
{
count_agg_clauses_context context;
context.root = root;
context.costs = costs;
- context.finalizeAggs = finalizeAggs;
- context.combineStates = combineStates;
- context.serialStates = serialStates;
+ context.aggmode = aggmode;
(void) count_agg_clauses_walker(clause, &context);
}
@@ -628,24 +624,25 @@ count_agg_clauses_walker(Node *node, count_agg_clauses_context *context)
* Add the appropriate component function execution costs to
* appropriate totals.
*/
- if (context->combineStates)
+ if (DO_AGGMODE_COMBINE(context->aggmode))
{
/* charge for combining previously aggregated states */
costs->transCost.per_tuple += get_func_cost(aggcombinefn) * cpu_operator_cost;
/* charge for deserialization, when appropriate */
- if (context->serialStates && OidIsValid(aggdeserialfn))
+ if (DO_AGGMODE_DESERIALIZE(context->aggmode) &&
+ OidIsValid(aggdeserialfn))
costs->transCost.per_tuple += get_func_cost(aggdeserialfn) * cpu_operator_cost;
}
else
costs->transCost.per_tuple += get_func_cost(aggtransfn) * cpu_operator_cost;
- if (context->finalizeAggs)
+ if (DO_AGGMODE_FINALIZE(context->aggmode))
{
if (OidIsValid(aggfinalfn))
costs->finalCost += get_func_cost(aggfinalfn) * cpu_operator_cost;
}
- else if (context->serialStates)
+ else if (DO_AGGMODE_SERIALIZE(context->aggmode))
{
if (OidIsValid(aggserialfn))
costs->finalCost += get_func_cost(aggserialfn) * cpu_operator_cost;
@@ -655,7 +652,7 @@ count_agg_clauses_walker(Node *node, count_agg_clauses_context *context)
* Some costs will already have been incurred by the initial aggregate
* node, so we mustn't include these again.
*/
- if (!context->combineStates)
+ if (!DO_AGGMODE_COMBINE(context->aggmode))
{
/* add the input expressions' cost to per-input-row costs */
cost_qual_eval_node(&argcosts, (Node *) aggref->args, context->root);
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 8fd933f..2dc9678 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -2483,9 +2483,7 @@ create_agg_path(PlannerInfo *root,
List *qual,
const AggClauseCosts *aggcosts,
double numGroups,
- bool combineStates,
- bool finalizeAggs,
- bool serialStates)
+ AggregateMode aggmode)
{
AggPath *pathnode = makeNode(AggPath);
@@ -2508,9 +2506,7 @@ create_agg_path(PlannerInfo *root,
pathnode->numGroups = numGroups;
pathnode->groupClause = groupClause;
pathnode->qual = qual;
- pathnode->finalizeAggs = finalizeAggs;
- pathnode->combineStates = combineStates;
- pathnode->serialStates = serialStates;
+ pathnode->aggmode = aggmode;
cost_agg(&pathnode->path, root,
aggstrategy, aggcosts,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 1ddf14a..4aba920 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1834,9 +1834,7 @@ typedef struct AggState
AggStatePerTrans curpertrans; /* currently active trans state */
bool input_done; /* indicates end of input */
bool agg_done; /* indicates completion of Agg scan */
- bool combineStates; /* input tuples contain transition states */
- bool finalizeAggs; /* should we call the finalfn on agg states? */
- bool serialStates; /* should agg states be (de)serialized? */
+ AggregateMode aggmode; /* enum to control type of agg operation */
int projected_set; /* The last projected grouping set */
int current_set; /* The current grouping set being evaluated */
Bitmapset *grouped_cols; /* grouped cols in current projection */
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 72f53fd..5ecd51b 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -711,9 +711,7 @@ typedef struct Agg
{
Plan plan;
AggStrategy aggstrategy; /* basic strategy, see nodes.h */
- bool combineStates; /* input tuples contain transition states */
- bool finalizeAggs; /* should we call the finalfn on agg states? */
- bool serialStates; /* should agg states be (de)serialized? */
+ AggregateMode aggmode; /* enum to control type of agg operation */
int numCols; /* number of grouping columns */
AttrNumber *grpColIdx; /* their indexes in the target list */
Oid *grpOperators; /* equality operators to compare with */
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 3de11f0..8d84898 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -301,6 +301,23 @@ typedef struct Aggref
int location; /* token location, or -1 if unknown */
} Aggref;
+#define AGGOP_COMBINESTATES 0x1
+#define AGGOP_SERIALIZESTATES 0x2
+#define AGGOP_DESERIALIZESTATES 0x4
+#define AGGOP_FINALIZEAGGS 0x8
+
+typedef enum AggregateMode
+{
+ AGGMODE_SIMPLE = AGGOP_FINALIZEAGGS,
+ AGGMODE_PARTIAL_SERIAL = AGGOP_SERIALIZESTATES,
+ AGGMODE_FINAL_DESERIAL = AGGOP_COMBINESTATES | AGGOP_DESERIALIZESTATES | AGGOP_FINALIZEAGGS
+} AggregateMode;
+
+#define DO_AGGMODE_COMBINE(am) (((am) & AGGOP_COMBINESTATES) != 0)
+#define DO_AGGMODE_SERIALIZE(am) (((am) & AGGOP_SERIALIZESTATES) != 0)
+#define DO_AGGMODE_DESERIALIZE(am) (((am) & AGGOP_DESERIALIZESTATES) != 0)
+#define DO_AGGMODE_FINALIZE(am) (((am) & AGGOP_FINALIZEAGGS) != 0)
+
/*
* GroupingFunc
*
diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h
index 9470df6..96bbe31 100644
--- a/src/include/nodes/relation.h
+++ b/src/include/nodes/relation.h
@@ -1350,9 +1350,7 @@ typedef struct AggPath
double numGroups; /* estimated number of groups in input */
List *groupClause; /* a list of SortGroupClause's */
List *qual; /* quals (HAVING quals), if any */
- bool combineStates; /* input is partially aggregated agg states */
- bool finalizeAggs; /* should the executor call the finalfn? */
- bool serialStates; /* should agg states be (de)serialized? */
+ AggregateMode aggmode; /* enum to control type of agg operation */
} AggPath;
/*
diff --git a/src/include/optimizer/clauses.h b/src/include/optimizer/clauses.h
index 53cf726..abec159 100644
--- a/src/include/optimizer/clauses.h
+++ b/src/include/optimizer/clauses.h
@@ -68,8 +68,7 @@ extern List *make_ands_implicit(Expr *clause);
extern PartialAggType aggregates_allow_partial(Node *clause);
extern bool contain_agg_clause(Node *clause);
extern void count_agg_clauses(PlannerInfo *root, Node *clause,
- AggClauseCosts *costs, bool finalizeAggs,
- bool combineStates, bool serialStates);
+ AggClauseCosts *costs, AggregateMode aggmode);
extern bool contain_window_function(Node *clause);
extern WindowFuncLists *find_window_functions(Node *clause, Index maxWinRef);
diff --git a/src/include/optimizer/pathnode.h b/src/include/optimizer/pathnode.h
index 5de4c34..73bb27c 100644
--- a/src/include/optimizer/pathnode.h
+++ b/src/include/optimizer/pathnode.h
@@ -170,9 +170,7 @@ extern AggPath *create_agg_path(PlannerInfo *root,
List *qual,
const AggClauseCosts *aggcosts,
double numGroups,
- bool combineStates,
- bool finalizeAggs,
- bool serialStates);
+ AggregateMode aggmode);
extern GroupingSetsPath *create_groupingsets_path(PlannerInfo *root,
RelOptInfo *rel,
Path *subpath,
diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h
index c529085..834c614 100644
--- a/src/include/optimizer/planmain.h
+++ b/src/include/optimizer/planmain.h
@@ -59,9 +59,8 @@ extern bool is_projection_capable_plan(Plan *plan);
/* External use of these functions is deprecated: */
extern Sort *make_sort_from_sortclauses(List *sortcls, Plan *lefttree);
extern Agg *make_agg(List *tlist, List *qual, AggStrategy aggstrategy,
- bool combineStates, bool finalizeAggs, bool serialStates,
- int numGroupCols, AttrNumber *grpColIdx, Oid *grpOperators,
- List *groupingSets, List *chain,
+ AggregateMode aggmode, int numGroupCols, AttrNumber *grpColIdx,
+ Oid *grpOperators, List *groupingSets, List *chain,
double dNumGroups, Plan *lefttree);
extern Limit *make_limit(Plan *lefttree, Node *limitOffset, Node *limitCount);
David Rowley <david.rowley@2ndquadrant.com> writes:
The attached implements this, with the exception that I didn't really
think AggPartialMode was the best name for the enum. I've named this
AggregateMode instead, as the aggregate is only partial in some cases.
Hm. We already have an AggStrategy (for hashed vs. grouped aggregation)
so adding AggregateMode beside it seems somewhere between confusing and
content-free. And it's needlessly inconsistent with the spelling of the
existing enum name. I'm not wedded to "AggPartialMode" but I think
we need some name that's a bit more specific than "AggregateMode".
Suggestions anyone?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
David Rowley <david.rowley@2ndquadrant.com> writes:
The attached implements this, with the exception that I didn't really
think AggPartialMode was the best name for the enum. I've named this
AggregateMode instead, as the aggregate is only partial in some cases.
Hm. We already have an AggStrategy (for hashed vs. grouped aggregation)
so adding AggregateMode beside it seems somewhere between confusing and
content-free. And it's needlessly inconsistent with the spelling of the
existing enum name. I'm not wedded to "AggPartialMode" but I think
we need some name that's a bit more specific than "AggregateMode".
Suggestions anyone?
After a bit of thought, maybe AggDivision or AggSplit or something
along those lines?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 26 June 2016 at 04:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
David Rowley <david.rowley@2ndquadrant.com> writes:
The attached implements this, with the exception that I didn't really
think AggPartialMode was the best name for the enum. I've named this
AggregateMode instead, as the aggregate is only partial in some cases.Hm. We already have an AggStrategy (for hashed vs. grouped aggregation)
so adding AggregateMode beside it seems somewhere between confusing and
content-free. And it's needlessly inconsistent with the spelling of the
existing enum name. I'm not wedded to "AggPartialMode" but I think
we need some name that's a bit more specific than "AggregateMode".
Suggestions anyone?After a bit of thought, maybe AggDivision or AggSplit or something
along those lines?
How about AggCompletion? It's seems to fit well in the sense of the
aggregation being partial or not, but less well when you consider
serialisation and combining states.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
David Rowley <david.rowley@2ndquadrant.com> writes:
On 26 June 2016 at 04:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
After a bit of thought, maybe AggDivision or AggSplit or something
along those lines?
How about AggCompletion? It's seems to fit well in the sense of the
aggregation being partial or not, but less well when you consider
serialisation and combining states.
After having worked on the patch some, I think that AggSplit is a pretty
good choice, because it is about how we split up the calculation of an
aggregate. And it's short, which is a good thing for something that's
going to be a component of assorted names.
What I've got at the moment looks like:
/* Primitive options supported by nodeAgg.c: */
#define AGGSPLITOP_COMBINE 0x1 /* substitute combinefn for transfn */
#define AGGSPLITOP_SERIALIZE 0x2 /* apply serializefn to output */
#define AGGSPLITOP_DESERIALIZE 0x4 /* apply deserializefn to input */
#define AGGSPLITOP_FINALIZE 0x8 /* run finalfn */
/* Supported operating modes (i.e., useful combinations of these options): */
typedef enum AggSplit
{
/* Basic, non-split aggregation: */
AGGSPLIT_SIMPLE = AGGSPLITOP_FINALIZE,
/* Initial phase of partial aggregation, with serialization: */
AGGSPLIT_PARTIAL_SERIAL = AGGSPLITOP_SERIALIZE,
/* Final phase of partial aggregation, with deserialization: */
AGGSPLIT_FINAL_DESERIAL = AGGSPLITOP_COMBINE | AGGSPLITOP_DESERIALIZE | AGGSPLITOP_FINALIZE
} AggSplit;
/* Test macros for the primitive options: */
#define DO_AGGSPLIT_COMBINE(as) (((as) & AGGSPLITOP_COMBINE) != 0)
#define DO_AGGSPLIT_SERIALIZE(as) (((as) & AGGSPLITOP_SERIALIZE) != 0)
#define DO_AGGSPLIT_DESERIALIZE(as) (((as) & AGGSPLITOP_DESERIALIZE) != 0)
#define DO_AGGSPLIT_FINALIZE(as) (((as) & AGGSPLITOP_FINALIZE) != 0)
Looking at this in the light of morning, I'm rather strongly tempted to
invert the sense of the FINALIZE option, so that "simple" mode works out
as zero, ie, select no options. Maybe call it SKIPFINAL instead of
FINALIZE?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 27 June 2016 at 03:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Looking at this in the light of morning, I'm rather strongly tempted to
invert the sense of the FINALIZE option, so that "simple" mode works out
as zero, ie, select no options. Maybe call it SKIPFINAL instead of
FINALIZE?
Aggref calls this aggpartial, and I was tempted to invert that many
times and make it aggfinalize, but in the end didn't.
It seems nicer to me to keep it as a list of things that are done,
rather than to make one exception to that just so we can have the
simple mode as 0.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
David Rowley <david.rowley@2ndquadrant.com> writes:
On 27 June 2016 at 03:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Looking at this in the light of morning, I'm rather strongly tempted to
invert the sense of the FINALIZE option, so that "simple" mode works out
as zero, ie, select no options. Maybe call it SKIPFINAL instead of
FINALIZE?
Aggref calls this aggpartial, and I was tempted to invert that many
times and make it aggfinalize, but in the end didn't.
It seems nicer to me to keep it as a list of things that are done,
rather than to make one exception to that just so we can have the
simple mode as 0.
[ shrug... ] I do not buy that argument, because it doesn't justify
the COMBINE option: why shouldn't that be inverted, ie USEFINALFUNC?
The only way to decide that except by fiat is to say that we're
enumerating the non-default or non-simple-mode behaviors.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
[ shrug... ] I do not buy that argument, because it doesn't justify
the COMBINE option: why shouldn't that be inverted, ie USEFINALFUNC?
Sorry, I meant USETRANSFUNC of course.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 27 June 2016 at 08:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Rowley <david.rowley@2ndquadrant.com> writes:
On 27 June 2016 at 03:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Looking at this in the light of morning, I'm rather strongly tempted to
invert the sense of the FINALIZE option, so that "simple" mode works out
as zero, ie, select no options. Maybe call it SKIPFINAL instead of
FINALIZE?Aggref calls this aggpartial, and I was tempted to invert that many
times and make it aggfinalize, but in the end didn't.
It seems nicer to me to keep it as a list of things that are done,
rather than to make one exception to that just so we can have the
simple mode as 0.[ shrug... ] I do not buy that argument, because it doesn't justify
the COMBINE option: why shouldn't that be inverted, ie USEFINALFUNC?
The only way to decide that except by fiat is to say that we're
enumerating the non-default or non-simple-mode behaviors.
hmm ok. I guess what exists today is there because I tried to make the
options "on" for additional tasks which aggregate must perform.
Whether to use transfunc or combine func is more of an alternative
than additional, so I have let what is standard tiebreak the decision
on which way around to have that flag. So perhaps you're right and we
should just normalise the flag to the standard aggregate behaviour to
keep it consistent.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 27 June 2016 at 03:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
After having worked on the patch some, I think that AggSplit is a pretty
good choice, because it is about how we split up the calculation of an
aggregate. And it's short, which is a good thing for something that's
going to be a component of assorted names.
I wish to thank you for working hard to improve this area of the code.
I had a read over your changes and I think there's quite a large
problem with your code which realy needs some more thinking:
I can't help wonder how plan to allow future expansions of
non-serialized partial aggregates giving that in setrefs.c you're
making a hard assumption that mark_partial_aggref() should always
receive the SERIAL versions of the aggsplit. This outright wrong as
the Agg node may not be a serialized aggregate node at all.
The attached very rough patch hacks together some code which has the
planner generate some partial aggregates which are not serialized.
Here's what I get:
CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
finalfunc = numeric_avg,
combinefunc = numeric_avg_combine
);
create table a (num numeric not null);
insert into a select generate_series(1,10);
select myavg(num) from a;
ERROR: invalid memory alloc request size 18446744072025250716
This is down to the aggtype being set to BYTEA regardless of the fact
that it's not a serialized aggregate.
Passing the non-serial versions of the enum to mark_partial_aggref()
makes this case work, but that's certainly not the fix.
David
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
aggtest.patchapplication/octet-stream; name=aggtest.patchDownload
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 070ad31..2a68920 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -3448,6 +3448,43 @@ create_grouping_paths(PlannerInfo *root,
grouping_is_hashable(parse->groupClause));
/*
+ * Build target list for partial aggregate paths. These paths cannot
+ * just emit the same tlist as regular aggregate paths, because (1) we
+ * must include Vars and Aggrefs needed in HAVING, which might not
+ * appear in the result tlist, and (2) the Aggrefs must be set in
+ * partial mode.
+ */
+ partial_grouping_target = make_partial_grouping_target(root, target);
+
+ /* Estimate number of partial groups. */
+ dNumPartialGroups = get_number_of_groups(root,
+ cheapest_path->rows,
+ NIL,
+ NIL);
+
+ /*
+ * Collect statistics about aggregates for estimating costs of
+ * performing aggregation in parallel.
+ */
+ MemSet(&agg_partial_costs, 0, sizeof(AggClauseCosts));
+ MemSet(&agg_final_costs, 0, sizeof(AggClauseCosts));
+ if (parse->hasAggs)
+ {
+ /* partial phase */
+ get_agg_clause_costs(root, (Node *) partial_grouping_target->exprs,
+ AGGSPLIT_INITIAL_SERIAL,
+ &agg_partial_costs);
+
+ /* final phase */
+ get_agg_clause_costs(root, (Node *) target->exprs,
+ AGGSPLIT_FINAL_DESERIAL,
+ &agg_final_costs);
+ get_agg_clause_costs(root, parse->havingQual,
+ AGGSPLIT_FINAL_DESERIAL,
+ &agg_final_costs);
+ }
+
+ /*
* Before generating paths for grouped_rel, we first generate any possible
* partial paths; that way, later code can easily consider both parallel
* and non-parallel approaches to grouping. Note that the partial paths
@@ -3459,43 +3496,6 @@ create_grouping_paths(PlannerInfo *root,
{
Path *cheapest_partial_path = linitial(input_rel->partial_pathlist);
- /*
- * Build target list for partial aggregate paths. These paths cannot
- * just emit the same tlist as regular aggregate paths, because (1) we
- * must include Vars and Aggrefs needed in HAVING, which might not
- * appear in the result tlist, and (2) the Aggrefs must be set in
- * partial mode.
- */
- partial_grouping_target = make_partial_grouping_target(root, target);
-
- /* Estimate number of partial groups. */
- dNumPartialGroups = get_number_of_groups(root,
- cheapest_partial_path->rows,
- NIL,
- NIL);
-
- /*
- * Collect statistics about aggregates for estimating costs of
- * performing aggregation in parallel.
- */
- MemSet(&agg_partial_costs, 0, sizeof(AggClauseCosts));
- MemSet(&agg_final_costs, 0, sizeof(AggClauseCosts));
- if (parse->hasAggs)
- {
- /* partial phase */
- get_agg_clause_costs(root, (Node *) partial_grouping_target->exprs,
- AGGSPLIT_INITIAL_SERIAL,
- &agg_partial_costs);
-
- /* final phase */
- get_agg_clause_costs(root, (Node *) target->exprs,
- AGGSPLIT_FINAL_DESERIAL,
- &agg_final_costs);
- get_agg_clause_costs(root, parse->havingQual,
- AGGSPLIT_FINAL_DESERIAL,
- &agg_final_costs);
- }
-
if (can_sort)
{
/* Checked in set_grouped_rel_consider_parallel() */
@@ -3626,17 +3626,47 @@ create_grouping_paths(PlannerInfo *root,
* We have aggregation, possibly with plain GROUP BY. Make
* an AggPath.
*/
- add_path(grouped_rel, (Path *)
- create_agg_path(root,
- grouped_rel,
- path,
- target,
- parse->groupClause ? AGG_SORTED : AGG_PLAIN,
- AGGSPLIT_SIMPLE,
- parse->groupClause,
- (List *) parse->havingQual,
- &agg_costs,
- dNumGroups));
+ if (!agg_costs.hasNonPartial)
+ {
+ Path *path2;
+
+ path2 = (Path *) create_agg_path(root,
+ grouped_rel,
+ path,
+ partial_grouping_target,
+ parse->groupClause ? AGG_SORTED : AGG_PLAIN,
+ AGGSPLIT_INITIAL,
+ parse->groupClause,
+ NIL,
+ &agg_costs,
+ dNumGroups);
+
+ add_path(grouped_rel, (Path *)
+ create_agg_path(root,
+ grouped_rel,
+ path2,
+ target,
+ parse->groupClause ? AGG_SORTED : AGG_PLAIN,
+ AGGSPLIT_FINAL,
+ parse->groupClause,
+ (List *) parse->havingQual,
+ &agg_final_costs,
+ dNumGroups));
+ }
+ else
+ {
+ add_path(grouped_rel, (Path *)
+ create_agg_path(root,
+ grouped_rel,
+ path,
+ target,
+ parse->groupClause ? AGG_SORTED : AGG_PLAIN,
+ AGGSPLIT_SIMPLE,
+ parse->groupClause,
+ (List *) parse->havingQual,
+ &agg_costs,
+ dNumGroups));
+ }
}
else if (parse->groupClause)
{
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 6b850e4..293a20c 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -715,7 +715,11 @@ typedef enum AggSplit
/* Initial phase of partial aggregation, with serialization: */
AGGSPLIT_INITIAL_SERIAL = AGGSPLITOP_SKIPFINAL | AGGSPLITOP_SERIALIZE,
/* Final phase of partial aggregation, with deserialization: */
- AGGSPLIT_FINAL_DESERIAL = AGGSPLITOP_COMBINE | AGGSPLITOP_DESERIALIZE
+ AGGSPLIT_FINAL_DESERIAL = AGGSPLITOP_COMBINE | AGGSPLITOP_DESERIALIZE,
+ /* Initial phase of partial aggregation: */
+ AGGSPLIT_INITIAL = AGGSPLITOP_SKIPFINAL,
+ /* Final phase of partial aggregation */
+ AGGSPLIT_FINAL = AGGSPLITOP_COMBINE
} AggSplit;
/* Test whether an AggSplit value selects each primitive option: */
David Rowley <david.rowley@2ndquadrant.com> writes:
I can't help wonder how plan to allow future expansions of
non-serialized partial aggregates giving that in setrefs.c you're
making a hard assumption that mark_partial_aggref() should always
receive the SERIAL versions of the aggsplit.
What I was imagining, but didn't bother to implement immediately, is
that we could pass down the appropriate AggSplit value from the plan
node (using the context argument for the mutator function). planner.c
likewise needs a bit more plumbing to generate such plan nodes in the
first place, so I didn't feel that setrefs.c had to be smarter right now.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers