From ec282bb7fb963325a30a3e94375289aa5457004b Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Fri, 12 Sep 2025 13:11:47 +0900 Subject: [PATCH v22 2/2] Allow negative aggtransspace to indicate unbounded state size This patch reuses the existing aggtransspace in pg_aggregate to signal that an aggregate's transition state can grow unboundedly. If aggtransspace is set to a negative value, it now indicates that the transition state may consume unpredictable or large amounts of memory, such as in aggregates like array_agg or string_agg that accumulate input rows. This information can be used by the planner to avoid applying memory-sensitive optimizations (e.g., eager aggregation) when there is a risk of excessive memory usage during partial aggregation. Bump catalog version. --- doc/src/sgml/catalogs.sgml | 5 ++++- doc/src/sgml/ref/create_aggregate.sgml | 11 ++++++++--- src/backend/optimizer/plan/initsplan.c | 23 +++++++---------------- src/include/catalog/catversion.h | 2 +- src/include/catalog/pg_aggregate.dat | 10 ++++++---- src/test/regress/expected/opr_sanity.out | 2 +- src/test/regress/sql/opr_sanity.sql | 2 +- 7 files changed, 28 insertions(+), 27 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index e9095bedf21..3acc2222a87 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -596,7 +596,10 @@ Approximate average size (in bytes) of the transition state - data, or zero to use a default estimate + data. A positive value provides an estimate; zero means to + use a default estimate. A negative value indicates the state + data can grow unboundedly in size, such as when the aggregate + accumulates input rows (e.g., array_agg, string_agg). diff --git a/doc/src/sgml/ref/create_aggregate.sgml b/doc/src/sgml/ref/create_aggregate.sgml index 222e0aa5c9d..0472ac2e874 100644 --- a/doc/src/sgml/ref/create_aggregate.sgml +++ b/doc/src/sgml/ref/create_aggregate.sgml @@ -384,9 +384,13 @@ SELECT col FROM tab ORDER BY col USING sortop LIMIT 1; The approximate average size (in bytes) of the aggregate's state value. If this parameter is omitted or is zero, a default estimate is used - based on the state_data_type. + based on the state_data_type. If set to a + negative value, it indicates the state data can grow unboundedly in + size, such as when the aggregate accumulates input rows (e.g., + array_agg, string_agg). The planner uses this value to estimate the memory required for a - grouped aggregate query. + grouped aggregate query and to avoid optimizations that may cause + excessive memory usage. @@ -568,7 +572,8 @@ SELECT col FROM tab ORDER BY col USING sortop LIMIT 1; The approximate average size (in bytes) of the aggregate's state value, when using moving-aggregate mode. This works the same as - state_data_size. + state_data_size, except that negative + values are not used to indicate unbounded state size. diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index 1b778f692d4..cb29c72c96c 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -716,19 +716,14 @@ setup_eager_aggregation(PlannerInfo *root) /* * is_partial_agg_memory_risky - * Checks if any aggregate poses a risk of excessive memory usage during + * Check if any aggregate poses a risk of excessive memory usage during * partial aggregation. * - * We check if any aggregate uses INTERNAL transition type. Although INTERNAL - * is marked as pass-by-value, it usually points to a large internal data - * structure (like those used by string_agg or array_agg). These transition - * states can grow large and their size is hard to estimate. Applying eager - * aggregation in such cases risks high memory usage since partial aggregation - * results might be stored in join hash tables or materialized nodes. - * - * We explicitly exclude aggregates with AVG_ACCUM transition function from - * this check, based on the assumption that avg() and sum() are safe in this - * context. + * We check if any aggregate has a negative aggtransspace value, which + * indicates that its transition state data can grow unboundedly in size. + * Applying eager aggregation in such cases risks high memory usage since + * partial aggregation results might be stored in join hash tables or + * materialized nodes. */ static bool is_partial_agg_memory_risky(PlannerInfo *root) @@ -739,11 +734,7 @@ is_partial_agg_memory_risky(PlannerInfo *root) { AggTransInfo *transinfo = lfirst_node(AggTransInfo, lc); - if (transinfo->transfn_oid == F_NUMERIC_AVG_ACCUM || - transinfo->transfn_oid == F_INT8_AVG_ACCUM) - continue; - - if (transinfo->aggtranstype == INTERNALOID) + if (transinfo->aggtransspace < 0) return true; } diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h index ef0d0f92165..62b0af3e0c3 100644 --- a/src/include/catalog/catversion.h +++ b/src/include/catalog/catversion.h @@ -57,6 +57,6 @@ */ /* yyyymmddN */ -#define CATALOG_VERSION_NO 202509091 +#define CATALOG_VERSION_NO 202509121 #endif diff --git a/src/include/catalog/pg_aggregate.dat b/src/include/catalog/pg_aggregate.dat index d6aa1f6ec47..870769e8f14 100644 --- a/src/include/catalog/pg_aggregate.dat +++ b/src/include/catalog/pg_aggregate.dat @@ -558,26 +558,28 @@ aggfinalfn => 'array_agg_finalfn', aggcombinefn => 'array_agg_combine', aggserialfn => 'array_agg_serialize', aggdeserialfn => 'array_agg_deserialize', aggfinalextra => 't', - aggtranstype => 'internal' }, + aggtranstype => 'internal', aggtransspace => '-1' }, { aggfnoid => 'array_agg(anyarray)', aggtransfn => 'array_agg_array_transfn', aggfinalfn => 'array_agg_array_finalfn', aggcombinefn => 'array_agg_array_combine', aggserialfn => 'array_agg_array_serialize', aggdeserialfn => 'array_agg_array_deserialize', aggfinalextra => 't', - aggtranstype => 'internal' }, + aggtranstype => 'internal', aggtransspace => '-1' }, # text { aggfnoid => 'string_agg(text,text)', aggtransfn => 'string_agg_transfn', aggfinalfn => 'string_agg_finalfn', aggcombinefn => 'string_agg_combine', aggserialfn => 'string_agg_serialize', - aggdeserialfn => 'string_agg_deserialize', aggtranstype => 'internal' }, + aggdeserialfn => 'string_agg_deserialize', + aggtranstype => 'internal', aggtransspace => '-1' }, # bytea { aggfnoid => 'string_agg(bytea,bytea)', aggtransfn => 'bytea_string_agg_transfn', aggfinalfn => 'bytea_string_agg_finalfn', aggcombinefn => 'string_agg_combine', aggserialfn => 'string_agg_serialize', - aggdeserialfn => 'string_agg_deserialize', aggtranstype => 'internal' }, + aggdeserialfn => 'string_agg_deserialize', + aggtranstype => 'internal', aggtransspace => '-1' }, # range { aggfnoid => 'range_intersect_agg(anyrange)', diff --git a/src/test/regress/expected/opr_sanity.out b/src/test/regress/expected/opr_sanity.out index 20bf9ea9cdf..a357e1d0c0e 100644 --- a/src/test/regress/expected/opr_sanity.out +++ b/src/test/regress/expected/opr_sanity.out @@ -1470,7 +1470,7 @@ WHERE aggfnoid = 0 OR aggtransfn = 0 OR (aggkind = 'n' AND aggnumdirectargs > 0) OR aggfinalmodify NOT IN ('r', 's', 'w') OR aggmfinalmodify NOT IN ('r', 's', 'w') OR - aggtranstype = 0 OR aggtransspace < 0 OR aggmtransspace < 0; + aggtranstype = 0 OR aggmtransspace < 0; ctid | aggfnoid ------+---------- (0 rows) diff --git a/src/test/regress/sql/opr_sanity.sql b/src/test/regress/sql/opr_sanity.sql index 2fb3a852878..cd674d7dbca 100644 --- a/src/test/regress/sql/opr_sanity.sql +++ b/src/test/regress/sql/opr_sanity.sql @@ -847,7 +847,7 @@ WHERE aggfnoid = 0 OR aggtransfn = 0 OR (aggkind = 'n' AND aggnumdirectargs > 0) OR aggfinalmodify NOT IN ('r', 's', 'w') OR aggmfinalmodify NOT IN ('r', 's', 'w') OR - aggtranstype = 0 OR aggtransspace < 0 OR aggmtransspace < 0; + aggtranstype = 0 OR aggmtransspace < 0; -- Make sure the matching pg_proc entry is sensible, too. -- 2.39.5 (Apple Git-154)