Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit c60b898

Browse files
committed
Fix handling of NULLs returned by aggregate combine functions.
When strict aggregate combine functions, used in multi-stage/parallel aggregation, returned NULL, we didn't check for that, invoking the combine function with NULL the next round, despite it being strict. The equivalent code invoking normal transition functions has a check for that situation, which did not get copied in a7de3dc. Fix the bug by adding the equivalent check. Based on a quick look I could not find any strict combine functions in core actually returning NULL, and it doesn't seem very likely external users have done so. So this isn't likely to have caused issues in practice. Add tests verifying transition / combine functions returning NULL is tested. Reported-By: Andres Freund Author: Andres Freund Discussion: https://postgr.es/m/20171121033642.7xvmjqrl4jdaaat3@alap3.anarazel.de Backpatch: 9.6, where parallel aggregation was introduced
1 parent 1881405 commit c60b898

File tree

3 files changed

+145
-0
lines changed

3 files changed

+145
-0
lines changed

src/backend/executor/nodeAgg.c

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1238,6 +1238,17 @@ advance_combine_function(AggState *aggstate,
12381238
pergroupstate->noTransValue = false;
12391239
return;
12401240
}
1241+
1242+
if (pergroupstate->transValueIsNull)
1243+
{
1244+
/*
1245+
* Don't call a strict function with NULL inputs. Note it is
1246+
* possible to get here despite the above tests, if the combinefn
1247+
* is strict *and* returned a NULL on a prior cycle. If that
1248+
* happens we will propagate the NULL all the way to the end.
1249+
*/
1250+
return;
1251+
}
12411252
}
12421253

12431254
/* We run the combine functions in per-input-tuple memory context */

src/test/regress/expected/aggregates.out

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1983,3 +1983,74 @@ NOTICE: sum_transfn called with 4
19831983
(1 row)
19841984

19851985
rollback;
1986+
-- test that the aggregate transition logic correctly handles
1987+
-- transition / combine functions returning NULL
1988+
-- First test the case of a normal transition function returning NULL
1989+
BEGIN;
1990+
CREATE FUNCTION balkifnull(int8, int4)
1991+
RETURNS int8
1992+
STRICT
1993+
LANGUAGE plpgsql AS $$
1994+
BEGIN
1995+
IF $1 IS NULL THEN
1996+
RAISE 'erroneously called with NULL argument';
1997+
END IF;
1998+
RETURN NULL;
1999+
END$$;
2000+
CREATE AGGREGATE balk(
2001+
BASETYPE = int4,
2002+
SFUNC = balkifnull(int8, int4),
2003+
STYPE = int8,
2004+
"PARALLEL" = SAFE,
2005+
INITCOND = '0');
2006+
SELECT balk(1) FROM tenk1;
2007+
balk
2008+
------
2009+
2010+
(1 row)
2011+
2012+
ROLLBACK;
2013+
-- Secondly test the case of a parallel aggregate combiner function
2014+
-- returning NULL. For that use normal transition function, but a
2015+
-- combiner function returning NULL.
2016+
BEGIN ISOLATION LEVEL REPEATABLE READ;
2017+
CREATE FUNCTION balkifnull(int8, int8)
2018+
RETURNS int8
2019+
PARALLEL SAFE
2020+
STRICT
2021+
LANGUAGE plpgsql AS $$
2022+
BEGIN
2023+
IF $1 IS NULL THEN
2024+
RAISE 'erroneously called with NULL argument';
2025+
END IF;
2026+
RETURN NULL;
2027+
END$$;
2028+
CREATE AGGREGATE balk(
2029+
BASETYPE = int4,
2030+
SFUNC = int4_sum(int8, int4),
2031+
STYPE = int8,
2032+
COMBINEFUNC = balkifnull(int8, int8),
2033+
"PARALLEL" = SAFE,
2034+
INITCOND = '0'
2035+
);
2036+
-- force use of parallelism
2037+
ALTER TABLE tenk1 set (parallel_workers = 4);
2038+
SET LOCAL parallel_setup_cost=0;
2039+
SET LOCAL max_parallel_workers_per_gather=4;
2040+
EXPLAIN (COSTS OFF) SELECT balk(1) FROM tenk1;
2041+
QUERY PLAN
2042+
--------------------------------------------------------------------------------
2043+
Finalize Aggregate
2044+
-> Gather
2045+
Workers Planned: 4
2046+
-> Partial Aggregate
2047+
-> Parallel Index Only Scan using tenk1_thous_tenthous on tenk1
2048+
(5 rows)
2049+
2050+
SELECT balk(1) FROM tenk1;
2051+
balk
2052+
------
2053+
2054+
(1 row)
2055+
2056+
ROLLBACK;

src/test/regress/sql/aggregates.sql

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -837,3 +837,66 @@ create aggregate my_half_sum(int4)
837837
select my_sum(one),my_half_sum(one) from (values(1),(2),(3),(4)) t(one);
838838

839839
rollback;
840+
841+
842+
-- test that the aggregate transition logic correctly handles
843+
-- transition / combine functions returning NULL
844+
845+
-- First test the case of a normal transition function returning NULL
846+
BEGIN;
847+
CREATE FUNCTION balkifnull(int8, int4)
848+
RETURNS int8
849+
STRICT
850+
LANGUAGE plpgsql AS $$
851+
BEGIN
852+
IF $1 IS NULL THEN
853+
RAISE 'erroneously called with NULL argument';
854+
END IF;
855+
RETURN NULL;
856+
END$$;
857+
858+
CREATE AGGREGATE balk(
859+
BASETYPE = int4,
860+
SFUNC = balkifnull(int8, int4),
861+
STYPE = int8,
862+
"PARALLEL" = SAFE,
863+
INITCOND = '0');
864+
865+
SELECT balk(1) FROM tenk1;
866+
867+
ROLLBACK;
868+
869+
-- Secondly test the case of a parallel aggregate combiner function
870+
-- returning NULL. For that use normal transition function, but a
871+
-- combiner function returning NULL.
872+
BEGIN ISOLATION LEVEL REPEATABLE READ;
873+
CREATE FUNCTION balkifnull(int8, int8)
874+
RETURNS int8
875+
PARALLEL SAFE
876+
STRICT
877+
LANGUAGE plpgsql AS $$
878+
BEGIN
879+
IF $1 IS NULL THEN
880+
RAISE 'erroneously called with NULL argument';
881+
END IF;
882+
RETURN NULL;
883+
END$$;
884+
885+
CREATE AGGREGATE balk(
886+
BASETYPE = int4,
887+
SFUNC = int4_sum(int8, int4),
888+
STYPE = int8,
889+
COMBINEFUNC = balkifnull(int8, int8),
890+
"PARALLEL" = SAFE,
891+
INITCOND = '0'
892+
);
893+
894+
-- force use of parallelism
895+
ALTER TABLE tenk1 set (parallel_workers = 4);
896+
SET LOCAL parallel_setup_cost=0;
897+
SET LOCAL max_parallel_workers_per_gather=4;
898+
899+
EXPLAIN (COSTS OFF) SELECT balk(1) FROM tenk1;
900+
SELECT balk(1) FROM tenk1;
901+
902+
ROLLBACK;

0 commit comments

Comments
 (0)