Fix incorrect presorted DISTINCT aggregate if condition

Here we fix a faulty "if" condition which failed to correctly handle two
or more consecutive NULL transition values when checking if the new value
is DISTINCT from the old value for presorted aggregates.  Given a suitably
non-strict aggregate transition function, a byref aggregate could cause a
crash due to calling the type's equality function and passing along a
(Datum) 0 value to test for equality, the equality function would then try
to dereference that 0 Datum and segfault.  For byval types, there'd have
been no crash and the equality function would have seen that the two 0
Datums matched, which (only by chance) meant the calling code would have
worked correctly.

Here we ensure that we only call the equality function when neither of
the input values are NULL.

This code is all new as of 1349d2790, so no backpatch needed.

Reported-by: Fujii Masao
Discussion: https://postgr.es/m/860c6d6f-a3c5-3ae9-9da2-827177bede06@oss.nttdata.com

diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 1470edc0aba6da04215c9a0bce62dbc03a00e973..827c65cc852a2d300baa22b516c2d71abb53fd9e 100644 (file)
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -4250,9 +4250,9 @@ ExecEvalPreOrderedDistinctSingle(AggState *aggstate, AggStatePerTrans pertrans)
 
        if (!pertrans->haslast ||
                pertrans->lastisnull != isnull ||
-               !DatumGetBool(FunctionCall2Coll(&pertrans->equalfnOne,
-                                                                               pertrans->aggCollation,
-                                                                               pertrans->lastdatum, value)))
+               (!isnull && !DatumGetBool(FunctionCall2Coll(&pertrans->equalfnOne,
+                                                                                                       pertrans->aggCollation,
+                                                                                                       pertrans->lastdatum, value))))
        {
                if (pertrans->haslast && !pertrans->inputtypeByVal)
                        pfree(DatumGetPointer(pertrans->lastdatum));

diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 82d096152484ede70001c0cef358c5e1cdea5510..52046c33dc8d479a880840a5735fe6b838ab26f5 100644 (file)
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -1506,6 +1506,14 @@ group by ten;
          ->  Seq Scan on tenk1
 (5 rows)
 
+-- Ensure consecutive NULLs are properly treated as distinct from each other
+select array_agg(distinct val)
+from (select null as val from generate_series(1, 2));
+ array_agg 
+-----------
+ {NULL}
+(1 row)
+
 -- Ensure no ordering is requested when enable_presorted_aggregate is off
 set enable_presorted_aggregate to off;
 explain (costs off)

diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index e81a22465b24db18fb98476d81976f3c48f33d9a..e7970983c3677d45c6feeb238864876cdf9358fb 100644 (file)
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -567,6 +567,10 @@ select
 from tenk1
 group by ten;
 
+-- Ensure consecutive NULLs are properly treated as distinct from each other
+select array_agg(distinct val)
+from (select null as val from generate_series(1, 2));
+
 -- Ensure no ordering is requested when enable_presorted_aggregate is off
 set enable_presorted_aggregate to off;
 explain (costs off)