Skip to content

Commit 411a462

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 19a6e1b commit 411a462

File tree

3 files changed

+103
-21
lines changed

3 files changed

+103
-21
lines changed

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3464,11 +3464,11 @@ numeric_combine(PG_FUNCTION_ARGS)
34643464
PG_RETURN_POINTER(state1);
34653465
}
34663466

3467+
state1->N += state2->N;
3468+
state1->NaNcount += state2->NaNcount;
3469+
34673470
if (state2->N > 0)
34683471
{
3469-
state1->N += state2->N;
3470-
state1->NaNcount += state2->NaNcount;
3471-
34723472
/*
34733473
* These are currently only needed for moving aggregates, but let's do
34743474
* the right thing anyway...
@@ -3551,11 +3551,11 @@ numeric_avg_combine(PG_FUNCTION_ARGS)
35513551
PG_RETURN_POINTER(state1);
35523552
}
35533553

3554+
state1->N += state2->N;
3555+
state1->NaNcount += state2->NaNcount;
3556+
35543557
if (state2->N > 0)
35553558
{
3556-
state1->N += state2->N;
3557-
state1->NaNcount += state2->NaNcount;
3558-
35593559
/*
35603560
* These are currently only needed for moving aggregates, but let's do
35613561
* the right thing anyway...

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

Lines changed: 69 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2085,21 +2085,79 @@ SET max_parallel_workers_per_gather = 4;
20852085
SET enable_indexonlyscan = off;
20862086
-- variance(int4) covers numeric_poly_combine
20872087
-- sum(int8) covers int8_avg_combine
2088-
EXPLAIN (COSTS OFF)
2089-
SELECT variance(unique1::int4), sum(unique1::int8) FROM tenk1;
2090-
QUERY PLAN
2091-
----------------------------------------------
2088+
-- regr_count(float8, float8) covers int8inc_float8_float8 and aggregates with > 1 arg
2089+
EXPLAIN (COSTS OFF, VERBOSE)
2090+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
2091+
FROM (SELECT * FROM tenk1
2092+
UNION ALL SELECT * FROM tenk1
2093+
UNION ALL SELECT * FROM tenk1
2094+
UNION ALL SELECT * FROM tenk1) u;
2095+
QUERY PLAN
2096+
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
20922097
Finalize Aggregate
2098+
Output: variance(tenk1.unique1), sum((tenk1.unique1)::bigint), regr_count((tenk1.unique1)::double precision, (tenk1.unique1)::double precision)
20932099
-> Gather
2100+
Output: (PARTIAL variance(tenk1.unique1)), (PARTIAL sum((tenk1.unique1)::bigint)), (PARTIAL regr_count((tenk1.unique1)::double precision, (tenk1.unique1)::double precision))
20942101
Workers Planned: 4
20952102
-> Partial Aggregate
2096-
-> Parallel Seq Scan on tenk1
2097-
(5 rows)
2098-
2099-
SELECT variance(unique1::int4), sum(unique1::int8) FROM tenk1;
2100-
variance | sum
2101-
----------------------+----------
2102-
8334166.666666666667 | 49995000
2103+
Output: PARTIAL variance(tenk1.unique1), PARTIAL sum((tenk1.unique1)::bigint), PARTIAL regr_count((tenk1.unique1)::double precision, (tenk1.unique1)::double precision)
2104+
-> Append
2105+
-> Parallel Seq Scan on public.tenk1
2106+
Output: tenk1.unique1
2107+
-> Parallel Seq Scan on public.tenk1 tenk1_1
2108+
Output: tenk1_1.unique1
2109+
-> Parallel Seq Scan on public.tenk1 tenk1_2
2110+
Output: tenk1_2.unique1
2111+
-> Parallel Seq Scan on public.tenk1 tenk1_3
2112+
Output: tenk1_3.unique1
2113+
(16 rows)
2114+
2115+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
2116+
FROM (SELECT * FROM tenk1
2117+
UNION ALL SELECT * FROM tenk1
2118+
UNION ALL SELECT * FROM tenk1
2119+
UNION ALL SELECT * FROM tenk1) u;
2120+
variance | sum | regr_count
2121+
----------------------+-----------+------------
2122+
8333541.588539713493 | 199980000 | 40000
2123+
(1 row)
2124+
2125+
-- variance(int8) covers numeric_combine
2126+
-- avg(numeric) covers numeric_avg_combine
2127+
EXPLAIN (COSTS OFF, VERBOSE)
2128+
SELECT variance(unique1::int8), avg(unique1::numeric)
2129+
FROM (SELECT * FROM tenk1
2130+
UNION ALL SELECT * FROM tenk1
2131+
UNION ALL SELECT * FROM tenk1
2132+
UNION ALL SELECT * FROM tenk1) u;
2133+
QUERY PLAN
2134+
--------------------------------------------------------------------------------------------------------
2135+
Finalize Aggregate
2136+
Output: variance((tenk1.unique1)::bigint), avg((tenk1.unique1)::numeric)
2137+
-> Gather
2138+
Output: (PARTIAL variance((tenk1.unique1)::bigint)), (PARTIAL avg((tenk1.unique1)::numeric))
2139+
Workers Planned: 4
2140+
-> Partial Aggregate
2141+
Output: PARTIAL variance((tenk1.unique1)::bigint), PARTIAL avg((tenk1.unique1)::numeric)
2142+
-> Append
2143+
-> Parallel Seq Scan on public.tenk1
2144+
Output: tenk1.unique1
2145+
-> Parallel Seq Scan on public.tenk1 tenk1_1
2146+
Output: tenk1_1.unique1
2147+
-> Parallel Seq Scan on public.tenk1 tenk1_2
2148+
Output: tenk1_2.unique1
2149+
-> Parallel Seq Scan on public.tenk1 tenk1_3
2150+
Output: tenk1_3.unique1
2151+
(16 rows)
2152+
2153+
SELECT variance(unique1::int8), avg(unique1::numeric)
2154+
FROM (SELECT * FROM tenk1
2155+
UNION ALL SELECT * FROM tenk1
2156+
UNION ALL SELECT * FROM tenk1
2157+
UNION ALL SELECT * FROM tenk1) u;
2158+
variance | avg
2159+
----------------------+-----------------------
2160+
8333541.588539713493 | 4999.5000000000000000
21032161
(1 row)
21042162

21052163
ROLLBACK;

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

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -920,10 +920,34 @@ SET enable_indexonlyscan = off;
920920

921921
-- variance(int4) covers numeric_poly_combine
922922
-- sum(int8) covers int8_avg_combine
923-
EXPLAIN (COSTS OFF)
924-
SELECT variance(unique1::int4), sum(unique1::int8) FROM tenk1;
925-
926-
SELECT variance(unique1::int4), sum(unique1::int8) FROM tenk1;
923+
-- regr_count(float8, float8) covers int8inc_float8_float8 and aggregates with > 1 arg
924+
EXPLAIN (COSTS OFF, VERBOSE)
925+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
926+
FROM (SELECT * FROM tenk1
927+
UNION ALL SELECT * FROM tenk1
928+
UNION ALL SELECT * FROM tenk1
929+
UNION ALL SELECT * FROM tenk1) u;
930+
931+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8)
932+
FROM (SELECT * FROM tenk1
933+
UNION ALL SELECT * FROM tenk1
934+
UNION ALL SELECT * FROM tenk1
935+
UNION ALL SELECT * FROM tenk1) u;
936+
937+
-- variance(int8) covers numeric_combine
938+
-- avg(numeric) covers numeric_avg_combine
939+
EXPLAIN (COSTS OFF, VERBOSE)
940+
SELECT variance(unique1::int8), avg(unique1::numeric)
941+
FROM (SELECT * FROM tenk1
942+
UNION ALL SELECT * FROM tenk1
943+
UNION ALL SELECT * FROM tenk1
944+
UNION ALL SELECT * FROM tenk1) u;
945+
946+
SELECT variance(unique1::int8), avg(unique1::numeric)
947+
FROM (SELECT * FROM tenk1
948+
UNION ALL SELECT * FROM tenk1
949+
UNION ALL SELECT * FROM tenk1
950+
UNION ALL SELECT * FROM tenk1) u;
927951

928952
ROLLBACK;
929953

0 commit comments

Comments
 (0)