From 001df21bc29374b191981333f31ee639dc848952 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 11 Nov 2026 15:08:56 -0500 Subject: [PATCH v3] Make float8_regr_combine() handle edge cases like float8_regr_accum(). Commit 649828769 taught float8_regr_accum() to keep Sxx, Syy and Sxy exactly zero while all the inputs for X (or Y) are the same, so that corr() and the regr_*() functions return NULL for a constant input. Then 7774249d0 made it force Sxy to NaN when one input is constant and the other is Inf or NaN. float8_regr_combine() got neither change: it still added the correction terms computed from the rounded sums Sx1 and Sx2, which are usually slightly nonzero even when both states saw the same single value. Thus, the edge cases those commits intended to fix occurred anyway under parallel aggregation. Skip each correction term when the corresponding input(s) have a common value, and force Sxy to NaN when the Sxy correction is skipped and the other input has seen an Inf or NaN. Also, make the overflow handling in float8_regr_combine() look more like float8_regr_accum(). It might have worked out the same in terms of when we throw error or don't, but even if that's true, proving it and then keeping it true in the future requires brain cells readers shouldn't have to expend. Bug: #19826 Reported-by: Venu Kummari Author: Venu Kummari Reviewed-by: Tom Lane Discussion: https://postgr.es/m/19826-4b769cb5342fdfe7@postgresql.org Backpatch-through: 19 --- src/backend/utils/adt/float.c | 72 +++++++++++++++++++----- src/test/regress/expected/aggregates.out | 64 ++++++++++++++++++++- src/test/regress/sql/aggregates.sql | 26 +++++++++ 3 files changed, 148 insertions(+), 14 deletions(-) diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c index 38ef6daf290..f2fe0f5b495 100644 --- a/src/backend/utils/adt/float.c +++ b/src/backend/utils/adt/float.c @@ -3649,19 +3649,10 @@ float8_regr_combine(PG_FUNCTION_ARGS) else { N = N1 + N2; - Sx = float8_pl(Sx1, Sx2); - tmp1 = Sx1 / N1 - Sx2 / N2; - Sxx = Sxx1 + Sxx2 + N1 * N2 * tmp1 * tmp1 / N; - if (unlikely(isinf(Sxx)) && !isinf(Sxx1) && !isinf(Sxx2)) - float_overflow_error(); - Sy = float8_pl(Sy1, Sy2); - tmp2 = Sy1 / N1 - Sy2 / N2; - Syy = Syy1 + Syy2 + N1 * N2 * tmp2 * tmp2 / N; - if (unlikely(isinf(Syy)) && !isinf(Syy1) && !isinf(Syy2)) - float_overflow_error(); - Sxy = Sxy1 + Sxy2 + N1 * N2 * tmp1 * tmp2 / N; - if (unlikely(isinf(Sxy)) && !isinf(Sxy1) && !isinf(Sxy2)) - float_overflow_error(); + Sx = Sx1 + Sx2; + Sy = Sy1 + Sy2; + + /* Check to see if we have seen distinct inputs */ if (float8_eq(Cx1, Cx2)) Cx = Cx1; else @@ -3670,6 +3661,61 @@ float8_regr_combine(PG_FUNCTION_ARGS) Cy = Cy1; else Cy = get_float8_nan(); + + tmp1 = Sx1 / N1 - Sx2 / N2; + tmp2 = Sy1 / N1 - Sy2 / N2; + + /* + * As in float8_regr_accum, if we have not seen distinct inputs for X + * (or Y) in either state, the corresponding sums should remain + * exactly zero: both means are then exactly Cx, and computing the + * correction terms from the rounded sums Sx1 and Sx2 would just + * inject roundoff error. We need exact zero results so that the + * final functions will return NULL in the right cases. + */ + Sxx = Sxx1 + Sxx2; + if (isnan(Cx)) + Sxx += N1 * N2 * tmp1 * tmp1 / N; + Syy = Syy1 + Syy2; + if (isnan(Cy)) + Syy += N1 * N2 * tmp2 * tmp2 / N; + Sxy = Sxy1 + Sxy2; + if (isnan(Cx) && isnan(Cy)) + Sxy += N1 * N2 * tmp1 * tmp2 / N; + else if (isnan(Sx) || isinf(Sx) || + isnan(Sy) || isinf(Sy)) + { + /* + * If one input has been constant so far, but the other one has + * been Inf or NaN, we must force Sxy to NaN to avoid falsely + * reporting variance zero. (We can't see individual X/Y values + * from here, but if any one has been Inf or NaN, the sum Sx/Sy + * must now be Inf or NaN.) Sxx and Syy don't have this issue. + */ + Sxy = get_float8_nan(); + } + + /* + * Overflow check, same logic as in float8_regr_accum. + */ + if (isinf(Sx) || isinf(Sxx) || isinf(Sy) || isinf(Syy) || isinf(Sxy)) + { + if (((isinf(Sx) || isinf(Sxx)) && + !isinf(Sx1) && !isinf(Sx2)) || + ((isinf(Sy) || isinf(Syy)) && + !isinf(Sy1) && !isinf(Sy2)) || + (isinf(Sxy) && + !isinf(Sx1) && !isinf(Sx2) && + !isinf(Sy1) && !isinf(Sy2))) + float_overflow_error(); + + if (isinf(Sxx)) + Sxx = get_float8_nan(); + if (isinf(Syy)) + Syy = get_float8_nan(); + if (isinf(Sxy)) + Sxy = get_float8_nan(); + } } /* diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out index ce555a31693..4f57b616db5 100644 --- a/src/test/regress/expected/aggregates.out +++ b/src/test/regress/expected/aggregates.out @@ -637,6 +637,68 @@ SELECT corr('NaN', 'NaN'), regr_r2('NaN', 'NaN') FROM generate_series(1, 30) g; NaN | NaN (1 row) +-- float8_regr_combine must treat constant inputs, and Inf/NaN with a +-- constant other input, the same way float8_regr_accum does, so that +-- partial aggregation gives the same results as a serial plan +CREATE TABLE regr_part (k int, x float8, y float8) PARTITION BY LIST (k); +CREATE TABLE regr_part_1 PARTITION OF regr_part FOR VALUES IN (1); +CREATE TABLE regr_part_2 PARTITION OF regr_part FOR VALUES IN (2); +CREATE TABLE regr_part_3 PARTITION OF regr_part FOR VALUES IN (3); +INSERT INTO regr_part SELECT 1 + g % 3, 1.0 / 3, g FROM generate_series(1, 80) g; +ANALYZE regr_part; +SET enable_partitionwise_aggregate = on; +EXPLAIN (COSTS OFF) +SELECT corr(x, x) FROM regr_part; + QUERY PLAN +------------------------------------------------------- + Finalize Aggregate + -> Append + -> Partial Aggregate + -> Seq Scan on regr_part_1 regr_part + -> Partial Aggregate + -> Seq Scan on regr_part_2 regr_part_1 + -> Partial Aggregate + -> Seq Scan on regr_part_3 regr_part_2 +(8 rows) + +SELECT corr(x, x), regr_r2(x, x), regr_slope(y, x), regr_sxx(y, x), covar_pop(x, y) +FROM regr_part; + corr | regr_r2 | regr_slope | regr_sxx | covar_pop +------+---------+------------+----------+----------- + | | | 0 | 0 +(1 row) + +-- an Inf or NaN in one partition, the other input constant +INSERT INTO regr_part VALUES (2, 'Infinity', 1); +SELECT covar_pop(x, 0::float8), covar_pop(0::float8, x) FROM regr_part; + covar_pop | covar_pop +-----------+----------- + NaN | NaN +(1 row) + +UPDATE regr_part SET x = 'NaN' WHERE x = 'Infinity'; +SELECT covar_pop(x, 0::float8), covar_pop(0::float8, x) FROM regr_part; + covar_pop | covar_pop +-----------+----------- + NaN | NaN +(1 row) + +RESET enable_partitionwise_aggregate; +SELECT covar_pop(x, 0::float8), covar_pop(0::float8, x) FROM regr_part; + covar_pop | covar_pop +-----------+----------- + NaN | NaN +(1 row) + +DELETE FROM regr_part WHERE x = 'NaN'; +SELECT corr(x, x), regr_r2(x, x), regr_slope(y, x), regr_sxx(y, x), covar_pop(x, y) +FROM regr_part; + corr | regr_r2 | regr_slope | regr_sxx | covar_pop +------+---------+------------+----------+----------- + | | | 0 | 0 +(1 row) + +DROP TABLE regr_part; -- test accum and combine functions directly CREATE TABLE regr_test (x float8, y float8); INSERT INTO regr_test VALUES (10,150),(20,250),(30,350),(80,540),(100,200); @@ -716,7 +778,7 @@ SELECT float8_regr_combine('{3,60,200,750,20000,2000,7,8}'::float8[], '{2,180,200,740,57800,-3400,7,9}'::float8[]); float8_regr_combine ------------------------------------ - {5,240,6280,1490,95080,8680,7,NaN} + {5,240,400,1490,95080,-1400,7,NaN} (1 row) DROP TABLE regr_test; diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql index 0936088b074..9fd7a4e0ea2 100644 --- a/src/test/regress/sql/aggregates.sql +++ b/src/test/regress/sql/aggregates.sql @@ -181,6 +181,32 @@ SELECT corr(0.1, 'NaN'), regr_r2(0.1, 'NaN') FROM generate_series(1, 30) g; SELECT corr('NaN', 0.1), regr_r2('NaN', 0.1) FROM generate_series(1, 30) g; SELECT corr('NaN', 'NaN'), regr_r2('NaN', 'NaN') FROM generate_series(1, 30) g; +-- float8_regr_combine must treat constant inputs, and Inf/NaN with a +-- constant other input, the same way float8_regr_accum does, so that +-- partial aggregation gives the same results as a serial plan +CREATE TABLE regr_part (k int, x float8, y float8) PARTITION BY LIST (k); +CREATE TABLE regr_part_1 PARTITION OF regr_part FOR VALUES IN (1); +CREATE TABLE regr_part_2 PARTITION OF regr_part FOR VALUES IN (2); +CREATE TABLE regr_part_3 PARTITION OF regr_part FOR VALUES IN (3); +INSERT INTO regr_part SELECT 1 + g % 3, 1.0 / 3, g FROM generate_series(1, 80) g; +ANALYZE regr_part; +SET enable_partitionwise_aggregate = on; +EXPLAIN (COSTS OFF) +SELECT corr(x, x) FROM regr_part; +SELECT corr(x, x), regr_r2(x, x), regr_slope(y, x), regr_sxx(y, x), covar_pop(x, y) +FROM regr_part; +-- an Inf or NaN in one partition, the other input constant +INSERT INTO regr_part VALUES (2, 'Infinity', 1); +SELECT covar_pop(x, 0::float8), covar_pop(0::float8, x) FROM regr_part; +UPDATE regr_part SET x = 'NaN' WHERE x = 'Infinity'; +SELECT covar_pop(x, 0::float8), covar_pop(0::float8, x) FROM regr_part; +RESET enable_partitionwise_aggregate; +SELECT covar_pop(x, 0::float8), covar_pop(0::float8, x) FROM regr_part; +DELETE FROM regr_part WHERE x = 'NaN'; +SELECT corr(x, x), regr_r2(x, x), regr_slope(y, x), regr_sxx(y, x), covar_pop(x, y) +FROM regr_part; +DROP TABLE regr_part; + -- test accum and combine functions directly CREATE TABLE regr_test (x float8, y float8); INSERT INTO regr_test VALUES (10,150),(20,250),(30,350),(80,540),(100,200); -- 2.52.0