Re: Deduplicate aggregates and transition functions in planner - Mailing list pgsql-hackers

Hi,

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.


> Attached is a patch to do that. It adds two new fields to Aggref, 'aggno'
> and 'aggtransno', to identify the unique aggregate and transition states.
> The duplicates are detected, and those filled in, early in the planning.
> Aside from those fields, the planner doesn't pass any other new information
> to to the executor, so the the executor still has to do syscache lookups to
> get the transition, combine etc. functions.

> I tried a bigger refactoring at first, to pass more information from the
> planner to the executor, but the patch grew really large before I got very
> far with it. So as the first step, I think we should apply the attached
> patch, and further refactoring can be done after that, if it seems
> worthwhile.

Working incrementally makes sense.



> @@ -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.

Since AggRef now knows its aggno, the state for EEOP_AGGREF should be
changed to be just an int, instead of a pointer to AggrefExprState..


> @@ -3432,8 +3426,18 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
>      /*
>       * We should now have found all Aggrefs in the targetlist and quals.
>       */
> -    numaggs = aggstate->numaggs;
> -    Assert(numaggs == list_length(aggstate->aggs));
> +    numaggrefs = list_length(aggstate->aggs);
> +    max_aggno = -1;
> +    max_transno = -1;
> +    foreach(l, aggstate->aggs)
> +    {
> +        Aggref       *aggref = (Aggref *) lfirst(l);
> +
> +        max_aggno = Max(max_aggno, aggref->aggno);
> +        max_transno = Max(max_transno, aggref->aggtransno);
> +    }
> +    numaggs = max_aggno + 1;
> +    numtrans = max_transno + 1;

We must have previously determined this, why don't we stash it in struct
Agg?

> --- a/src/backend/jit/llvm/llvmjit_expr.c
> +++ b/src/backend/jit/llvm/llvmjit_expr.c
> @@ -1850,19 +1850,11 @@ llvm_compile_expr(ExprState *state)
>              case EEOP_AGGREF:
>                  {
>                      AggrefExprState *aggref = op->d.aggref.astate;
> -                    LLVMValueRef v_aggnop;
>                      LLVMValueRef v_aggno;
>                      LLVMValueRef value,
>                                  isnull;
>  
> -                    /*
> -                     * At this point aggref->aggno is not yet set (it's set up
> -                     * in ExecInitAgg() after initializing the expression). So
> -                     * load it from memory each time round.
> -                     */
> -                    v_aggnop = l_ptr_const(&aggref->aggno,
> -                                           l_ptr(LLVMInt32Type()));
> -                    v_aggno = LLVMBuildLoad(b, v_aggnop, "v_aggno");
> +                    v_aggno = l_int32_const(aggref->aggno);

Yay!


> +/*
> + * get_agg_clause_costs
> + *      Recursively find the Aggref nodes in an expression tree, and
> + *      accumulate cost information about them.

Think this comment is out of date now.


Greetings,

Andres Freund