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

Andres Freund <andres@anarazel.de> writes:

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

On reflection, I'm not sure that such a policy is a win. If you go
with an internal-type transition state, then (if you want parallel
aggregation support) you need serialize/deserialize functions, and
I think also some other stuff that comes for free if the transition
state is a real SQL type. So this is not so much a clear win as
a tradeoff of which code you want to write.

regards, tom lane

On 04/09/2026 17:50, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

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

On reflection, I'm not sure that such a policy is a win. If you go
with an internal-type transition state, then (if you want parallel
aggregation support) you need serialize/deserialize functions, and
I think also some other stuff that comes for free if the transition
state is a real SQL type. So this is not so much a clear win as
a tradeoff of which code you want to write.

As I see it, with an internal state, some aggregates gain, while others don't.

Plain sum(int2/int4/float8) already has a trivial 8-byte transition state:
minimal memory, passed by value, no detoast. The moving-aggregate mode of
sum(int2/int4), on the other hand, reuses avg()'s int2/int4_avg_accum_inv
functions, which pay for array overhead and a detoast check on every row.
Internal state might reduce memory consumption there.

float8 has no moving-aggregate mode. Also note that the regression tests
currently call float8_regr_combine() directly, with literal array values.

Not sure how much it matters, but introducing an internal state also means
giving up initcond. Hence, sum(float8) with an internal state would lose its
STRICT marking, pushing NULL-checking into the function body.

Quick tests show sum() losing 1-3% in serial mode and a more significant ~7% in
a parallel GROUP BY, due to serialize/deserialize across workers. This was run
on my laptop, not a dedicated benchmarking box, but the overhead looks clearly
detectable.

Overall, in my opinion, only AVG() and other statistical aggregates, such as
STDDEV(), benefit from moving to internal state. Basic aggregates like sum()
should probably stay as they are, if we want to avoid a performance regression.

--
regards, Andrei Lepikhov,
pgEdge