Re: Deduplicate aggregates and transition functions in planner

On 28/10/2020 21:59, Andres Freund wrote:
> On 2020-10-28 21:10:41 +0200, Heikki Linnakangas wrote:
>> Currently, ExecInitAgg() performs quite a lot of work, to deduplicate
>> identical Aggrefs, as well as Aggrefs that can share the same transition
>> state. That doesn't really belong in the executor, we should perform that
>> work in the planner. It doesn't change from one invocation of the plan to
>> another, and it would be nice to reflect the state-sharing in the plan
>> costs.
>
> Woo! Very glad to see this tackled.
>
> It wouldn't surprise me to see a small execution time speedup here -
> I've seen the load of the aggno show up in profiles.

I think you'd be hard-pressed to find a real-life query where it
matters. But if you don't care about real life:

regression=# do $$
begin
for i in 1..100000 loop
perform sum(g), sum(g+0), sum(g+1), sum(g+2), sum(g+3), sum(g+4),
sum(g+5), sum(g+6), sum(g+7), sum(g+8), sum(g+9), sum(g+10) from
generate_series(1,1) g;
end loop;
end;
$$;
DO
Time: 1282.701 ms (00:01.283)

vs.

Time: 860.323 ms

with the patch.

>> @@ -783,14 +783,13 @@ ExecInitExprRec(Expr *node, ExprState *state,
>>
>> scratch.opcode = EEOP_AGGREF;
>> scratch.d.aggref.astate = astate;
>> - astate->aggref = aggref;
>> + astate->aggno = aggref->aggno;
>>
>> if (state->parent && IsA(state->parent, AggState))
>> {
>> AggState *aggstate = (AggState *) state->parent;
>>
>> - aggstate->aggs = lappend(aggstate->aggs, astate);
>> - aggstate->numaggs++;
>> + aggstate->aggs = lappend(aggstate->aggs, aggref);
>
> Hm. Why is aggstate->aggs still built during expression initialization?
> Imo that's a pretty huge wart that also introduces more
> order-of-operation brittleness to executor startup.

The Agg node itself doesn't include any information about the aggregates
and transition functions. Because of that, ExecInitAgg needs a
"representive" Aggref for each transition state and agg, to initialize
the per-trans and per-agg structs. The expression initialization makes
those Aggrefs available for ExecInitAgg.

Instead of collecting all the Aggrefs in a list, ExecInitExprRec() could
set each representative Aggref directly in the right per-trans and
per-agg struct, based on the 'aggno' and 'aggtransno' fields. That
requires initializing the per-trans and per-agg arrays earlier, and for
that, we would need to store the # of aggs and transition states in the
Agg node, like you also suggested. Certainly doable, but on the whole,
it didn't really seem better to me. Attached is a patch, to demonstrate
what that looks like, on top of the main patch. It's not complete,
there's at least one case with hash-DISTINCT for queries like "SELECT
DISTINCT aggregate(x) ..." where the planner creates an Agg for the
DISTINCT without aggregates, but the code currently passes numAggs=1 to
the executor. Some further changes would be needed in the planner, to
mark the AggPath generated for deduplication differently from the
AggPaths created for aggregation. Again that's doable, but on the whole
I prefer the approach to scan the Aggrefs in executor startup, for now.

I'd like to get rid of the "representative Aggrefs" altogether, and pass
information about the transition and final functions from planner to
executor in some other form. But that's exactly what got me into the
refactoring that was ballooning out of hand that I mentioned.

- Heikki