Re: Make the transition state of avg(int2)/avg(int4)/sum(int2)/sum(int4) internal

Hi,

On 2026-09-04 16:06:02 +0200, Andrei Lepikhov wrote:
> In the thread [1], there we couple of opinions [2,3] to make transition states
> internal. Since it is a separate topic from the overflow bug, so here is a
> separate thread.
>
> Attached is a draft implementation for the two families that still keep their
> transition state in an int8[]: avg(int2), avg(int4). These aggregates'
> transition type is now declared as INTERNAL that doesn't correspond to any SQL
> data type and can't be called outside.

> This is kinda of the continuation of 69c8fbac201, which did the same thing for
> the numeric aggregates.

> After this patch only float8 built-in aggregates still keep their transition
> state in an array. Does it make sense to fold float8 into this patch, or keep it
> separate?

Seems like if we do this - and I think we should - we should go broader than
just doing this for int8[]. So yea, let's do it for float8 too.

I think it might make sense to have an opr_sanity.sql check that verifies that
we don't add new builtin aggregates that have an array transition state. Or
perhaps even more strictly, test that aggregates either have an internal
transition state, or the argument type's (for stuff like min/max).

> From 5993ce7e528cee80fe598936ef1137fb934314e1 Mon Sep 17 00:00:00 2001
> From: Andrei Lepikhov <andrei(dot)lepikhov(at)pgedge(dot)com>
> Date: Fri, 4 Sep 2026 15:33:47 +0200
> Subject: [PATCH v0] Make the transition state of
> avg(int2)/avg(int4)/sum(int2)/sum(int4) internal

> Commit 69c8fbac201 declared the transition state of the numeric aggregates
> INTERNAL, on the grounds that it does not correspond to any SQL data type.
> avg(int2), avg(int4) and the moving-aggregate mode of sum(int2)/sum(int4)
> were left behind: they still keep count and sum in a two-element int8[],
> which is a value anybody can construct and pass in.
>
> Supporting that costs something in every transition call: the argument
> may be toasted, the array may contain NULLs, its length has to be checked,
> and the state cannot be modified in place unless we first establish that we
> really are inside an aggregate and buys nothing, since no caller has any
> reason to build such a state by hand.
>
> So declare the transition type INTERNAL and keep count and sum in a plain
> struct. Add int4_avg_serialize()/int4_avg_deserialize() so that two-phase
> aggregation, and with it parallel and partitionwise aggregation, keeps
> working.
>
> Note that this makes int2_avg_accum, int4_avg_accum, their inverses,
> int4_avg_combine, int8_avg and int2int4_sum unusable in a user-defined
> aggregate declared with stype = int8[]. The two in-tree examples that did
> so now declare stype = internal instead.

FWIW, I find it pretty useful to reference both threads here. If we, in a
couple years, look at some aspect of this change, the other thread will be
harder to find otherwise.

> +/*
> + * Prepare state data for an aggregate function that needs to compute the sum
> + * and count of int2 or int4 inputs.
> + */
> +static Int8TransTypeData *
> +makeInt8TransTypeData(FunctionCallInfo fcinfo)
> +{
> + Int8TransTypeData *state;
> + MemoryContext agg_context;
> + MemoryContext old_context;
> +
> + if (!AggCheckCallContext(fcinfo, &agg_context))
> + elog(ERROR, "aggregate function called in non-aggregate context");

That shouldn't be reachable, right? One could imo validly argue for making
this an assert instead...

> + old_context = MemoryContextSwitchTo(agg_context);
> +
> + state = palloc0_object(Int8TransTypeData);
> +
> + MemoryContextSwitchTo(old_context);
> +
> + return state;

I'd make that a MemoryContextAllocZero(). Just switching the context back and
forth for one allocation is more verbose and slower.

> +/*
> + * Transition function for int2 input.
> + */
> Datum
> int2_avg_accum(PG_FUNCTION_ARGS)
> {
> - ArrayType *transarray;
> - int16 newval = PG_GETARG_INT16(1);
> - Int8TransTypeData *transdata;
> + Int8TransTypeData *state;
>
> - /*
> - * If we're invoked as an aggregate, we can cheat and modify our first
> - * parameter in-place to reduce palloc overhead. Otherwise we need to make
> - * a copy of it before scribbling on it.
> - */
> - if (AggCheckCallContext(fcinfo, NULL))
> - transarray = PG_GETARG_ARRAYTYPE_P(0);
> - else
> - transarray = PG_GETARG_ARRAYTYPE_P_COPY(0);
> + state = PG_ARGISNULL(0) ? NULL : (Int8TransTypeData *) PG_GETARG_POINTER(0);
>
> - if (ARR_HASNULL(transarray) ||
> - ARR_SIZE(transarray) != ARR_OVERHEAD_NONULLS(1) + sizeof(Int8TransTypeData))
> - elog(ERROR, "expected 2-element int8 array");
> + /* Create the state data on the first call */
> + if (state == NULL)
> + state = makeInt8TransTypeData(fcinfo);

Probably the compiler can optimize it away, but it seems weird to me to have
state = PG_ARGISNULL(0) ? NULL : ...;
followed by an
if (state == NULL)
Why not just have one if?

> - transdata = (Int8TransTypeData *) ARR_DATA_PTR(transarray);
> - transdata->count++;
> - transdata->sum += newval;
> + if (!PG_ARGISNULL(1))
> + {
> + state->count++;
> + state->sum += PG_GETARG_INT16(1);
> + }

Hm. Previously these functions were declared strict, so the runtime check here
wasn't needed. But that doesn't work with internal transition states. I kinda
wonder whether we should - separately - add support for allocating the
transition space needed for all aggregates that support it, within one
group by "step", in one go. The fragmented allocations for transition state
space does not help us, performance wise, if you look at profiles of queries
with many aggregates (e.g. TPCH Q01).

Is there a reason to create the transition state in the PG_ARGISNULL case? If
not, we could just return immediately if (PG_ARGISNULL(1)), before even
creating the transition state.

Seems like int2_avg_accum() & int4_avg_accum(); int2_avg_accum_inv() &
int2_avg_accum_inv(); etc perhaps should share their implementation, with the
sole difference - PG_GETARG_INT16() vs PG_GETARG_INT32() - done in the caller
and the rest in wrappers?

> diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
> index f11e244899e..82a53884656 100644
> --- a/src/include/catalog/catversion.h
> +++ b/src/include/catalog/catversion.h
> @@ -57,6 +57,6 @@
> */
>
> /* yyyymmddN */
> -#define CATALOG_VERSION_NO 202608271
> +#define CATALOG_VERSION_NO 202609031

Don't include catversion bump in patches, just mention that it's needed in the
commit message. Otherwise it's guaranteed that patch does not apply anymore
very very soon.

Greetings,

Andres Freund