Skip to content

Commit 77a3be3

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 92c58fd commit 77a3be3

File tree

3 files changed

+101
-23
lines changed

3 files changed

+101
-23
lines changed

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

+6-6
Original file line numberDiff line numberDiff line change
@@ -3967,11 +3967,11 @@ numeric_combine(PG_FUNCTION_ARGS)
39673967
PG_RETURN_POINTER(state1);
39683968
}
39693969

3970+
state1->N += state2->N;
3971+
state1->NaNcount += state2->NaNcount;
3972+
39703973
if (state2->N > 0)
39713974
{
3972-
state1->N += state2->N;
3973-
state1->NaNcount += state2->NaNcount;
3974-
39753975
/*
39763976
* These are currently only needed for moving aggregates, but let's do
39773977
* the right thing anyway...
@@ -4054,11 +4054,11 @@ numeric_avg_combine(PG_FUNCTION_ARGS)
40544054
PG_RETURN_POINTER(state1);
40554055
}
40564056

4057+
state1->N += state2->N;
4058+
state1->NaNcount += state2->NaNcount;
4059+
40574060
if (state2->N > 0)
40584061
{
4059-
state1->N += state2->N;
4060-
state1->NaNcount += state2->NaNcount;
4061-
40624062
/*
40634063
* These are currently only needed for moving aggregates, but let's do
40644064
* the right thing anyway...

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

+68-14
Original file line numberDiff line numberDiff line change
@@ -2294,29 +2294,83 @@ SET parallel_setup_cost = 0;
22942294
SET parallel_tuple_cost = 0;
22952295
SET min_parallel_table_scan_size = 0;
22962296
SET max_parallel_workers_per_gather = 4;
2297+
SET parallel_leader_participation = off;
22972298
SET enable_indexonlyscan = off;
22982299
-- variance(int4) covers numeric_poly_combine
22992300
-- sum(int8) covers int8_avg_combine
23002301
-- regr_count(float8, float8) covers int8inc_float8_float8 and aggregates with > 1 arg
23012302
EXPLAIN (COSTS OFF, VERBOSE)
2302-
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
2303-
QUERY PLAN
2304-
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
2303+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
2304+
FROM (SELECT * FROM tenk1
2305+
UNION ALL SELECT * FROM tenk1
2306+
UNION ALL SELECT * FROM tenk1
2307+
UNION ALL SELECT * FROM tenk1) u;
2308+
QUERY PLAN
2309+
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
23052310
Finalize Aggregate
2306-
Output: variance(unique1), sum((unique1)::bigint), regr_count((unique1)::double precision, (unique1)::double precision)
2311+
Output: variance(tenk1.unique1), sum((tenk1.unique1)::bigint), regr_count((tenk1.unique1)::double precision, (tenk1.unique1)::double precision)
23072312
-> Gather
2308-
Output: (PARTIAL variance(unique1)), (PARTIAL sum((unique1)::bigint)), (PARTIAL regr_count((unique1)::double precision, (unique1)::double precision))
2313+
Output: (PARTIAL variance(tenk1.unique1)), (PARTIAL sum((tenk1.unique1)::bigint)), (PARTIAL regr_count((tenk1.unique1)::double precision, (tenk1.unique1)::double precision))
23092314
Workers Planned: 4
23102315
-> Partial Aggregate
2311-
Output: PARTIAL variance(unique1), PARTIAL sum((unique1)::bigint), PARTIAL regr_count((unique1)::double precision, (unique1)::double precision)
2312-
-> Parallel Seq Scan on public.tenk1
2313-
Output: unique1, unique2, two, four, ten, twenty, hundred, thousand, twothousand, fivethous, tenthous, odd, even, stringu1, stringu2, string4
2314-
(9 rows)
2315-
2316-
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
2317-
variance | sum | regr_count
2318-
----------------------+----------+------------
2319-
8334166.666666666667 | 49995000 | 10000
2316+
Output: PARTIAL variance(tenk1.unique1), PARTIAL sum((tenk1.unique1)::bigint), PARTIAL regr_count((tenk1.unique1)::double precision, (tenk1.unique1)::double precision)
2317+
-> Parallel Append
2318+
-> Parallel Seq Scan on public.tenk1
2319+
Output: tenk1.unique1
2320+
-> Parallel Seq Scan on public.tenk1 tenk1_1
2321+
Output: tenk1_1.unique1
2322+
-> Parallel Seq Scan on public.tenk1 tenk1_2
2323+
Output: tenk1_2.unique1
2324+
-> Parallel Seq Scan on public.tenk1 tenk1_3
2325+
Output: tenk1_3.unique1
2326+
(16 rows)
2327+
2328+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
2329+
FROM (SELECT * FROM tenk1
2330+
UNION ALL SELECT * FROM tenk1
2331+
UNION ALL SELECT * FROM tenk1
2332+
UNION ALL SELECT * FROM tenk1) u;
2333+
variance | sum | regr_count
2334+
----------------------+-----------+------------
2335+
8333541.588539713493 | 199980000 | 40000
2336+
(1 row)
2337+
2338+
-- variance(int8) covers numeric_combine
2339+
-- avg(numeric) covers numeric_avg_combine
2340+
EXPLAIN (COSTS OFF, VERBOSE)
2341+
SELECT variance(unique1::int8), avg(unique1::numeric)
2342+
FROM (SELECT * FROM tenk1
2343+
UNION ALL SELECT * FROM tenk1
2344+
UNION ALL SELECT * FROM tenk1
2345+
UNION ALL SELECT * FROM tenk1) u;
2346+
QUERY PLAN
2347+
--------------------------------------------------------------------------------------------------------
2348+
Finalize Aggregate
2349+
Output: variance((tenk1.unique1)::bigint), avg((tenk1.unique1)::numeric)
2350+
-> Gather
2351+
Output: (PARTIAL variance((tenk1.unique1)::bigint)), (PARTIAL avg((tenk1.unique1)::numeric))
2352+
Workers Planned: 4
2353+
-> Partial Aggregate
2354+
Output: PARTIAL variance((tenk1.unique1)::bigint), PARTIAL avg((tenk1.unique1)::numeric)
2355+
-> Parallel Append
2356+
-> Parallel Seq Scan on public.tenk1
2357+
Output: tenk1.unique1
2358+
-> Parallel Seq Scan on public.tenk1 tenk1_1
2359+
Output: tenk1_1.unique1
2360+
-> Parallel Seq Scan on public.tenk1 tenk1_2
2361+
Output: tenk1_2.unique1
2362+
-> Parallel Seq Scan on public.tenk1 tenk1_3
2363+
Output: tenk1_3.unique1
2364+
(16 rows)
2365+
2366+
SELECT variance(unique1::int8), avg(unique1::numeric)
2367+
FROM (SELECT * FROM tenk1
2368+
UNION ALL SELECT * FROM tenk1
2369+
UNION ALL SELECT * FROM tenk1
2370+
UNION ALL SELECT * FROM tenk1) u;
2371+
variance | avg
2372+
----------------------+-----------------------
2373+
8333541.588539713493 | 4999.5000000000000000
23202374
(1 row)
23212375

23222376
ROLLBACK;

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

+27-3
Original file line numberDiff line numberDiff line change
@@ -1000,15 +1000,39 @@ SET parallel_setup_cost = 0;
10001000
SET parallel_tuple_cost = 0;
10011001
SET min_parallel_table_scan_size = 0;
10021002
SET max_parallel_workers_per_gather = 4;
1003+
SET parallel_leader_participation = off;
10031004
SET enable_indexonlyscan = off;
10041005

10051006
-- variance(int4) covers numeric_poly_combine
10061007
-- sum(int8) covers int8_avg_combine
10071008
-- regr_count(float8, float8) covers int8inc_float8_float8 and aggregates with > 1 arg
10081009
EXPLAIN (COSTS OFF, VERBOSE)
1009-
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
1010-
1011-
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
1010+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
1011+
FROM (SELECT * FROM tenk1
1012+
UNION ALL SELECT * FROM tenk1
1013+
UNION ALL SELECT * FROM tenk1
1014+
UNION ALL SELECT * FROM tenk1) u;
1015+
1016+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
1017+
FROM (SELECT * FROM tenk1
1018+
UNION ALL SELECT * FROM tenk1
1019+
UNION ALL SELECT * FROM tenk1
1020+
UNION ALL SELECT * FROM tenk1) u;
1021+
1022+
-- variance(int8) covers numeric_combine
1023+
-- avg(numeric) covers numeric_avg_combine
1024+
EXPLAIN (COSTS OFF, VERBOSE)
1025+
SELECT variance(unique1::int8), avg(unique1::numeric)
1026+
FROM (SELECT * FROM tenk1
1027+
UNION ALL SELECT * FROM tenk1
1028+
UNION ALL SELECT * FROM tenk1
1029+
UNION ALL SELECT * FROM tenk1) u;
1030+
1031+
SELECT variance(unique1::int8), avg(unique1::numeric)
1032+
FROM (SELECT * FROM tenk1
1033+
UNION ALL SELECT * FROM tenk1
1034+
UNION ALL SELECT * FROM tenk1
1035+
UNION ALL SELECT * FROM tenk1) u;
10121036

10131037
ROLLBACK;
10141038

0 commit comments

Comments
 (0)