Skip to content

Commit 49e0a42

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 dd616f2 commit 49e0a42

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
@@ -3628,11 +3628,11 @@ numeric_combine(PG_FUNCTION_ARGS)
36283628
PG_RETURN_POINTER(state1);
36293629
}
36303630

3631+
state1->N += state2->N;
3632+
state1->NaNcount += state2->NaNcount;
3633+
36313634
if (state2->N > 0)
36323635
{
3633-
state1->N += state2->N;
3634-
state1->NaNcount += state2->NaNcount;
3635-
36363636
/*
36373637
* These are currently only needed for moving aggregates, but let's do
36383638
* the right thing anyway...
@@ -3715,11 +3715,11 @@ numeric_avg_combine(PG_FUNCTION_ARGS)
37153715
PG_RETURN_POINTER(state1);
37163716
}
37173717

3718+
state1->N += state2->N;
3719+
state1->NaNcount += state2->NaNcount;
3720+
37183721
if (state2->N > 0)
37193722
{
3720-
state1->N += state2->N;
3721-
state1->NaNcount += state2->NaNcount;
3722-
37233723
/*
37243724
* These are currently only needed for moving aggregates, but let's do
37253725
* the right thing anyway...

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

Lines changed: 68 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2114,29 +2114,83 @@ SET parallel_setup_cost = 0;
21142114
SET parallel_tuple_cost = 0;
21152115
SET min_parallel_table_scan_size = 0;
21162116
SET max_parallel_workers_per_gather = 4;
2117+
SET parallel_leader_participation = off;
21172118
SET enable_indexonlyscan = off;
21182119
-- variance(int4) covers numeric_poly_combine
21192120
-- sum(int8) covers int8_avg_combine
21202121
-- regr_count(float8, float8) covers int8inc_float8_float8 and aggregates with > 1 arg
21212122
EXPLAIN (COSTS OFF, VERBOSE)
2122-
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
2123-
QUERY PLAN
2124-
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
2123+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
2124+
FROM (SELECT * FROM tenk1
2125+
UNION ALL SELECT * FROM tenk1
2126+
UNION ALL SELECT * FROM tenk1
2127+
UNION ALL SELECT * FROM tenk1) u;
2128+
QUERY PLAN
2129+
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
21252130
Finalize Aggregate
2126-
Output: variance(unique1), sum((unique1)::bigint), regr_count((unique1)::double precision, (unique1)::double precision)
2131+
Output: variance(tenk1.unique1), sum((tenk1.unique1)::bigint), regr_count((tenk1.unique1)::double precision, (tenk1.unique1)::double precision)
21272132
-> Gather
2128-
Output: (PARTIAL variance(unique1)), (PARTIAL sum((unique1)::bigint)), (PARTIAL regr_count((unique1)::double precision, (unique1)::double precision))
2133+
Output: (PARTIAL variance(tenk1.unique1)), (PARTIAL sum((tenk1.unique1)::bigint)), (PARTIAL regr_count((tenk1.unique1)::double precision, (tenk1.unique1)::double precision))
21292134
Workers Planned: 4
21302135
-> Partial Aggregate
2131-
Output: PARTIAL variance(unique1), PARTIAL sum((unique1)::bigint), PARTIAL regr_count((unique1)::double precision, (unique1)::double precision)
2132-
-> Parallel Seq Scan on public.tenk1
2133-
Output: unique1, unique2, two, four, ten, twenty, hundred, thousand, twothousand, fivethous, tenthous, odd, even, stringu1, stringu2, string4
2134-
(9 rows)
2135-
2136-
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
2137-
variance | sum | regr_count
2138-
----------------------+----------+------------
2139-
8334166.666666666667 | 49995000 | 10000
2136+
Output: PARTIAL variance(tenk1.unique1), PARTIAL sum((tenk1.unique1)::bigint), PARTIAL regr_count((tenk1.unique1)::double precision, (tenk1.unique1)::double precision)
2137+
-> Parallel Append
2138+
-> Parallel Seq Scan on public.tenk1
2139+
Output: tenk1.unique1
2140+
-> Parallel Seq Scan on public.tenk1 tenk1_1
2141+
Output: tenk1_1.unique1
2142+
-> Parallel Seq Scan on public.tenk1 tenk1_2
2143+
Output: tenk1_2.unique1
2144+
-> Parallel Seq Scan on public.tenk1 tenk1_3
2145+
Output: tenk1_3.unique1
2146+
(16 rows)
2147+
2148+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
2149+
FROM (SELECT * FROM tenk1
2150+
UNION ALL SELECT * FROM tenk1
2151+
UNION ALL SELECT * FROM tenk1
2152+
UNION ALL SELECT * FROM tenk1) u;
2153+
variance | sum | regr_count
2154+
----------------------+-----------+------------
2155+
8333541.588539713493 | 199980000 | 40000
2156+
(1 row)
2157+
2158+
-- variance(int8) covers numeric_combine
2159+
-- avg(numeric) covers numeric_avg_combine
2160+
EXPLAIN (COSTS OFF, VERBOSE)
2161+
SELECT variance(unique1::int8), avg(unique1::numeric)
2162+
FROM (SELECT * FROM tenk1
2163+
UNION ALL SELECT * FROM tenk1
2164+
UNION ALL SELECT * FROM tenk1
2165+
UNION ALL SELECT * FROM tenk1) u;
2166+
QUERY PLAN
2167+
--------------------------------------------------------------------------------------------------------
2168+
Finalize Aggregate
2169+
Output: variance((tenk1.unique1)::bigint), avg((tenk1.unique1)::numeric)
2170+
-> Gather
2171+
Output: (PARTIAL variance((tenk1.unique1)::bigint)), (PARTIAL avg((tenk1.unique1)::numeric))
2172+
Workers Planned: 4
2173+
-> Partial Aggregate
2174+
Output: PARTIAL variance((tenk1.unique1)::bigint), PARTIAL avg((tenk1.unique1)::numeric)
2175+
-> Parallel Append
2176+
-> Parallel Seq Scan on public.tenk1
2177+
Output: tenk1.unique1
2178+
-> Parallel Seq Scan on public.tenk1 tenk1_1
2179+
Output: tenk1_1.unique1
2180+
-> Parallel Seq Scan on public.tenk1 tenk1_2
2181+
Output: tenk1_2.unique1
2182+
-> Parallel Seq Scan on public.tenk1 tenk1_3
2183+
Output: tenk1_3.unique1
2184+
(16 rows)
2185+
2186+
SELECT variance(unique1::int8), avg(unique1::numeric)
2187+
FROM (SELECT * FROM tenk1
2188+
UNION ALL SELECT * FROM tenk1
2189+
UNION ALL SELECT * FROM tenk1
2190+
UNION ALL SELECT * FROM tenk1) u;
2191+
variance | avg
2192+
----------------------+-----------------------
2193+
8333541.588539713493 | 4999.5000000000000000
21402194
(1 row)
21412195

21422196
ROLLBACK;

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

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -937,15 +937,39 @@ SET parallel_setup_cost = 0;
937937
SET parallel_tuple_cost = 0;
938938
SET min_parallel_table_scan_size = 0;
939939
SET max_parallel_workers_per_gather = 4;
940+
SET parallel_leader_participation = off;
940941
SET enable_indexonlyscan = off;
941942

942943
-- variance(int4) covers numeric_poly_combine
943944
-- sum(int8) covers int8_avg_combine
944945
-- regr_count(float8, float8) covers int8inc_float8_float8 and aggregates with > 1 arg
945946
EXPLAIN (COSTS OFF, VERBOSE)
946-
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
947-
948-
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
947+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
948+
FROM (SELECT * FROM tenk1
949+
UNION ALL SELECT * FROM tenk1
950+
UNION ALL SELECT * FROM tenk1
951+
UNION ALL SELECT * FROM tenk1) u;
952+
953+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
954+
FROM (SELECT * FROM tenk1
955+
UNION ALL SELECT * FROM tenk1
956+
UNION ALL SELECT * FROM tenk1
957+
UNION ALL SELECT * FROM tenk1) u;
958+
959+
-- variance(int8) covers numeric_combine
960+
-- avg(numeric) covers numeric_avg_combine
961+
EXPLAIN (COSTS OFF, VERBOSE)
962+
SELECT variance(unique1::int8), avg(unique1::numeric)
963+
FROM (SELECT * FROM tenk1
964+
UNION ALL SELECT * FROM tenk1
965+
UNION ALL SELECT * FROM tenk1
966+
UNION ALL SELECT * FROM tenk1) u;
967+
968+
SELECT variance(unique1::int8), avg(unique1::numeric)
969+
FROM (SELECT * FROM tenk1
970+
UNION ALL SELECT * FROM tenk1
971+
UNION ALL SELECT * FROM tenk1
972+
UNION ALL SELECT * FROM tenk1) u;
949973

950974
ROLLBACK;
951975

0 commit comments

Comments
 (0)