Skip to content

Commit 4284e11

Browse files
tglsfdctglsfdc
committed
Fix mishandling of NaN counts in numeric_[avg_]combine.
When merging two NumericAggStates, the code missed adding the new state's NaNcount unless its N was also nonzero; since those counts are independent, this is wrong. This would only have visible effect if some partial aggregate scans found only NaNs while earlier ones found only non-NaNs; then we could end up falsely deciding that there were no NaNs and fail to return a NaN final result as expected. That's pretty improbable, so it's no surprise this hasn't been reported from the field. Still, it's a bug. I didn't try to produce a regression test that would show the bug, but I did notice that these functions weren't being reached at all in our regression tests, so I improved the tests to at least exercise them. With these additions, I see pretty complete code coverage on the aggregation-related functions in numeric.c. Back-patch to 9.6 where this code was introduced. (I only added the improved test case as far back as v10, though, since the relevant part of aggregates.sql isn't there at all in 9.6.)
1 parent e620a38 commit 4284e11

File tree

3 files changed

+101
-23
lines changed

3 files changed

+101
-23
lines changed

‎src/backend/utils/adt/numeric.c

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3772,11 +3772,11 @@ numeric_combine(PG_FUNCTION_ARGS)
37723772
PG_RETURN_POINTER(state1);
37733773
}
37743774

3775+
state1->N += state2->N;
3776+
state1->NaNcount += state2->NaNcount;
3777+
37753778
if (state2->N > 0)
37763779
{
3777-
state1->N += state2->N;
3778-
state1->NaNcount += state2->NaNcount;
3779-
37803780
/*
37813781
* These are currently only needed for moving aggregates, but let's do
37823782
* the right thing anyway...
@@ -3859,11 +3859,11 @@ numeric_avg_combine(PG_FUNCTION_ARGS)
38593859
PG_RETURN_POINTER(state1);
38603860
}
38613861

3862+
state1->N += state2->N;
3863+
state1->NaNcount += state2->NaNcount;
3864+
38623865
if (state2->N > 0)
38633866
{
3864-
state1->N += state2->N;
3865-
state1->NaNcount += state2->NaNcount;
3866-
38673867
/*
38683868
* These are currently only needed for moving aggregates, but let's do
38693869
* the right thing anyway...

‎src/test/regress/expected/aggregates.out

Lines changed: 68 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2244,29 +2244,83 @@ SET parallel_setup_cost = 0;
22442244
SET parallel_tuple_cost = 0;
22452245
SET min_parallel_table_scan_size = 0;
22462246
SET max_parallel_workers_per_gather = 4;
2247+
SET parallel_leader_participation = off;
22472248
SET enable_indexonlyscan = off;
22482249
-- variance(int4) covers numeric_poly_combine
22492250
-- sum(int8) covers int8_avg_combine
22502251
-- regr_count(float8, float8) covers int8inc_float8_float8 and aggregates with > 1 arg
22512252
EXPLAIN (COSTS OFF, VERBOSE)
2252-
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
2253-
QUERY PLAN
2254-
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
2253+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
2254+
FROM (SELECT * FROM tenk1
2255+
UNION ALL SELECT * FROM tenk1
2256+
UNION ALL SELECT * FROM tenk1
2257+
UNION ALL SELECT * FROM tenk1) u;
2258+
QUERY PLAN
2259+
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
22552260
Finalize Aggregate
2256-
Output: variance(unique1), sum((unique1)::bigint), regr_count((unique1)::double precision, (unique1)::double precision)
2261+
Output: variance(tenk1.unique1), sum((tenk1.unique1)::bigint), regr_count((tenk1.unique1)::double precision, (tenk1.unique1)::double precision)
22572262
-> Gather
2258-
Output: (PARTIAL variance(unique1)), (PARTIAL sum((unique1)::bigint)), (PARTIAL regr_count((unique1)::double precision, (unique1)::double precision))
2263+
Output: (PARTIAL variance(tenk1.unique1)), (PARTIAL sum((tenk1.unique1)::bigint)), (PARTIAL regr_count((tenk1.unique1)::double precision, (tenk1.unique1)::double precision))
22592264
Workers Planned: 4
22602265
-> Partial Aggregate
2261-
Output: PARTIAL variance(unique1), PARTIAL sum((unique1)::bigint), PARTIAL regr_count((unique1)::double precision, (unique1)::double precision)
2262-
-> Parallel Seq Scan on public.tenk1
2263-
Output: unique1, unique2, two, four, ten, twenty, hundred, thousand, twothousand, fivethous, tenthous, odd, even, stringu1, stringu2, string4
2264-
(9 rows)
2265-
2266-
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
2267-
variance | sum | regr_count
2268-
----------------------+----------+------------
2269-
8334166.666666666667 | 49995000 | 10000
2266+
Output: PARTIAL variance(tenk1.unique1), PARTIAL sum((tenk1.unique1)::bigint), PARTIAL regr_count((tenk1.unique1)::double precision, (tenk1.unique1)::double precision)
2267+
-> Parallel Append
2268+
-> Parallel Seq Scan on public.tenk1
2269+
Output: tenk1.unique1
2270+
-> Parallel Seq Scan on public.tenk1 tenk1_1
2271+
Output: tenk1_1.unique1
2272+
-> Parallel Seq Scan on public.tenk1 tenk1_2
2273+
Output: tenk1_2.unique1
2274+
-> Parallel Seq Scan on public.tenk1 tenk1_3
2275+
Output: tenk1_3.unique1
2276+
(16 rows)
2277+
2278+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
2279+
FROM (SELECT * FROM tenk1
2280+
UNION ALL SELECT * FROM tenk1
2281+
UNION ALL SELECT * FROM tenk1
2282+
UNION ALL SELECT * FROM tenk1) u;
2283+
variance | sum | regr_count
2284+
----------------------+-----------+------------
2285+
8333541.588539713493 | 199980000 | 40000
2286+
(1 row)
2287+
2288+
-- variance(int8) covers numeric_combine
2289+
-- avg(numeric) covers numeric_avg_combine
2290+
EXPLAIN (COSTS OFF, VERBOSE)
2291+
SELECT variance(unique1::int8), avg(unique1::numeric)
2292+
FROM (SELECT * FROM tenk1
2293+
UNION ALL SELECT * FROM tenk1
2294+
UNION ALL SELECT * FROM tenk1
2295+
UNION ALL SELECT * FROM tenk1) u;
2296+
QUERY PLAN
2297+
--------------------------------------------------------------------------------------------------------
2298+
Finalize Aggregate
2299+
Output: variance((tenk1.unique1)::bigint), avg((tenk1.unique1)::numeric)
2300+
-> Gather
2301+
Output: (PARTIAL variance((tenk1.unique1)::bigint)), (PARTIAL avg((tenk1.unique1)::numeric))
2302+
Workers Planned: 4
2303+
-> Partial Aggregate
2304+
Output: PARTIAL variance((tenk1.unique1)::bigint), PARTIAL avg((tenk1.unique1)::numeric)
2305+
-> Parallel Append
2306+
-> Parallel Seq Scan on public.tenk1
2307+
Output: tenk1.unique1
2308+
-> Parallel Seq Scan on public.tenk1 tenk1_1
2309+
Output: tenk1_1.unique1
2310+
-> Parallel Seq Scan on public.tenk1 tenk1_2
2311+
Output: tenk1_2.unique1
2312+
-> Parallel Seq Scan on public.tenk1 tenk1_3
2313+
Output: tenk1_3.unique1
2314+
(16 rows)
2315+
2316+
SELECT variance(unique1::int8), avg(unique1::numeric)
2317+
FROM (SELECT * FROM tenk1
2318+
UNION ALL SELECT * FROM tenk1
2319+
UNION ALL SELECT * FROM tenk1
2320+
UNION ALL SELECT * FROM tenk1) u;
2321+
variance | avg
2322+
----------------------+-----------------------
2323+
8333541.588539713493 | 4999.5000000000000000
22702324
(1 row)
22712325

22722326
ROLLBACK;

‎src/test/regress/sql/aggregates.sql

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -981,15 +981,39 @@ SET parallel_setup_cost = 0;
981981
SET parallel_tuple_cost = 0;
982982
SET min_parallel_table_scan_size = 0;
983983
SET max_parallel_workers_per_gather = 4;
984+
SET parallel_leader_participation = off;
984985
SET enable_indexonlyscan = off;
985986

986987
-- variance(int4) covers numeric_poly_combine
987988
-- sum(int8) covers int8_avg_combine
988989
-- regr_count(float8, float8) covers int8inc_float8_float8 and aggregates with > 1 arg
989990
EXPLAIN (COSTS OFF, VERBOSE)
990-
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
991-
992-
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
991+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
992+
FROM (SELECT * FROM tenk1
993+
UNION ALL SELECT * FROM tenk1
994+
UNION ALL SELECT * FROM tenk1
995+
UNION ALL SELECT * FROM tenk1) u;
996+
997+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
998+
FROM (SELECT * FROM tenk1
999+
UNION ALL SELECT * FROM tenk1
1000+
UNION ALL SELECT * FROM tenk1
1001+
UNION ALL SELECT * FROM tenk1) u;
1002+
1003+
-- variance(int8) covers numeric_combine
1004+
-- avg(numeric) covers numeric_avg_combine
1005+
EXPLAIN (COSTS OFF, VERBOSE)
1006+
SELECT variance(unique1::int8), avg(unique1::numeric)
1007+
FROM (SELECT * FROM tenk1
1008+
UNION ALL SELECT * FROM tenk1
1009+
UNION ALL SELECT * FROM tenk1
1010+
UNION ALL SELECT * FROM tenk1) u;
1011+
1012+
SELECT variance(unique1::int8), avg(unique1::numeric)
1013+
FROM (SELECT * FROM tenk1
1014+
UNION ALL SELECT * FROM tenk1
1015+
UNION ALL SELECT * FROM tenk1
1016+
UNION ALL SELECT * FROM tenk1) u;
9931017

9941018
ROLLBACK;
9951019

0 commit comments

Comments
 (0)