Hi,
On 2020-10-29 10:17:20 +0200, Heikki Linnakangas wrote:
> On 28/10/2020 21:59, Andres Freund wrote:
> > 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:
I was actually thinking about a different angle - that the evaluation of
an Aggref can be faster, because we need less indirection to find the
aggno. As you have already implemented for the JITed code, but removing
it for the expression code looks easy enough too. You'd need a lot of
groups and presumably a fair number of Aggrefs to see it.
Attached is a quick version of what I am thinking wrt AggrefExprState.
> > > @@ -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.
Hold on a second: To me the question is why is it the right design that
the Agg node doesn't have the information about "aggregates and
transition functions"? Agg e.g. already does directly contains the group
keys...
My concern wouldn't really be addressed if we replace the lappend() in
ExecInitExprRec() with setting something "directly in the right
per-trans...". I think it's structurally wrong to have to discover
Aggrefs at execution time at all.
Perhaps the easiest incremental step would be to have something like a
CookedAggref { int aggno; } and then just store the Aggref nodes in
Agg->aggs, with aggno referencing that...
> 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.
Fair.
Greetings,
Andres Freund
diff --git i/src/include/executor/execExpr.h w/src/include/executor/execExpr.h
index b792de1bc95..abb489e2062 100644
--- i/src/include/executor/execExpr.h
+++ w/src/include/executor/execExpr.h
@@ -564,8 +564,7 @@ typedef struct ExprEvalStep
/* for EEOP_AGGREF */
struct
{
- /* out-of-line state, modified by nodeAgg.c */
- AggrefExprState *astate;
+ int aggno;
} aggref;
/* for EEOP_GROUPING_FUNC */
diff --git i/src/include/nodes/execnodes.h w/src/include/nodes/execnodes.h
index fc5698cff20..0ff19256e13 100644
--- i/src/include/nodes/execnodes.h
+++ w/src/include/nodes/execnodes.h
@@ -746,16 +746,6 @@ typedef tuplehash_iterator TupleHashIterator;
* ----------------------------------------------------------------
*/
-/* ----------------
- * AggrefExprState node
- * ----------------
- */
-typedef struct AggrefExprState
-{
- NodeTag type;
- int aggno; /* ID number for agg within its plan node */
-} AggrefExprState;
-
/* ----------------
* WindowFuncExprState node
* ----------------
diff --git i/src/include/nodes/nodes.h w/src/include/nodes/nodes.h
index 7ddd8c011bf..3684f87a883 100644
--- i/src/include/nodes/nodes.h
+++ w/src/include/nodes/nodes.h
@@ -206,10 +206,9 @@ typedef enum NodeTag
* Most Expr-based plan nodes do not have a corresponding expression state
* node, they're fully handled within execExpr* - but sometimes the state
* needs to be shared with other parts of the executor, as for example
- * with AggrefExprState, which nodeAgg.c has to modify.
+ * with SubPlanState, which nodeSubplan.c has to modify.
*/
T_ExprState,
- T_AggrefExprState,
T_WindowFuncExprState,
T_SetExprState,
T_SubPlanState,
diff --git i/src/backend/executor/execExpr.c w/src/backend/executor/execExpr.c
index 2a4dea2b052..7f1aba20d10 100644
--- i/src/backend/executor/execExpr.c
+++ w/src/backend/executor/execExpr.c
@@ -99,8 +99,7 @@ static void ExecBuildAggTransCall(ExprState *state, AggState *aggstate,
* the same as the per-query context of the associated ExprContext.
*
* Any Aggref, WindowFunc, or SubPlan nodes found in the tree are added to
- * the lists of such nodes held by the parent PlanState (or more accurately,
- * the AggrefExprState etc. nodes created for them are added).
+ * the lists of such nodes held by the parent PlanState.
*
* Note: there is no ExecEndExpr function; we assume that any resource
* cleanup needed will be handled by just releasing the memory context
@@ -779,11 +778,9 @@ ExecInitExprRec(Expr *node, ExprState *state,
case T_Aggref:
{
Aggref *aggref = (Aggref *) node;
- AggrefExprState *astate = makeNode(AggrefExprState);
scratch.opcode = EEOP_AGGREF;
- scratch.d.aggref.astate = astate;
- astate->aggno = aggref->aggno;
+ scratch.d.aggref.aggno = aggref->aggno;
if (state->parent && IsA(state->parent, AggState))
{
diff --git i/src/backend/executor/execExprInterp.c w/src/backend/executor/execExprInterp.c
index 26c2b496321..bef5fe22573 100644
--- i/src/backend/executor/execExprInterp.c
+++ w/src/backend/executor/execExprInterp.c
@@ -1494,12 +1494,12 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
* Returns a Datum whose value is the precomputed aggregate value
* found in the given expression context.
*/
- AggrefExprState *aggref = op->d.aggref.astate;
+ int aggno = op->d.aggref.aggno;
Assert(econtext->ecxt_aggvalues != NULL);
- *op->resvalue = econtext->ecxt_aggvalues[aggref->aggno];
- *op->resnull = econtext->ecxt_aggnulls[aggref->aggno];
+ *op->resvalue = econtext->ecxt_aggvalues[aggno];
+ *op->resnull = econtext->ecxt_aggnulls[aggno];
EEO_NEXT();
}
diff --git i/src/backend/jit/llvm/llvmjit_expr.c w/src/backend/jit/llvm/llvmjit_expr.c
index 4085064dad7..f232397cabf 100644
--- i/src/backend/jit/llvm/llvmjit_expr.c
+++ w/src/backend/jit/llvm/llvmjit_expr.c
@@ -1849,12 +1849,11 @@ llvm_compile_expr(ExprState *state)
case EEOP_AGGREF:
{
- AggrefExprState *aggref = op->d.aggref.astate;
LLVMValueRef v_aggno;
LLVMValueRef value,
isnull;
- v_aggno = l_int32_const(aggref->aggno);
+ v_aggno = l_int32_const(op->d.aggref.aggno);
/* load agg value / null */
value = l_load_gep1(b, v_aggvalues, v_aggno, "aggvalue");
diff --git i/src/tools/pgindent/typedefs.list w/src/tools/pgindent/typedefs.list
index b6acade6c67..dba3d992ae3 100644
--- i/src/tools/pgindent/typedefs.list
+++ w/src/tools/pgindent/typedefs.list
@@ -50,7 +50,6 @@ AggStatePerPhase
AggStatePerTrans
AggStrategy
Aggref
-AggrefExprState
AlenState
Alias
AllocBlock
