Fix IGNORE NULLS nullness cache for volatile window arguments.

The IGNORE NULLS implementation caches whether a window function argument
evaluated to NULL or NOT NULL for a given partition row.  That is safe for
ordinary expressions, but not for volatile expressions, where evaluating the
same argument on the same row can produce a different NULL/NOT NULL result
later.

This could produce wrong results in two ways.  A row previously cached as
NULL could be skipped even though a later evaluation would return NOT NULL.
Conversely, a row cached as NOT NULL could be chosen as the target row, then
re-evaluated to fetch the actual value and return NULL.

Make the nullness cache conditional per argument.  Do not use it for
arguments containing volatile functions or subplans, following the same
conservative approach used for moving window aggregates.  Also avoid
re-evaluating non-cacheable partition arguments after the scan has already
found the target row.

Add regression tests covering volatile arguments and subplan arguments with
IGNORE NULLS.

Author: Chao Li <lic@highgo.com>
Reviewed-by: Tatsuo Ishii <ishii@postgresql.org>
Discussion: https://postgr.es/m/42B42506-6972-4266-8422-FB73E61D9DA7@gmail.com

diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 5cc39bd90861ca5b5bd658b76f61fb337217ecad..f1c524d00df9292161f2e0ab481d4df56bad0c7e 100644 (file)
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -76,6 +76,7 @@ typedef struct WindowObjectData
        int64      *num_notnull_info;   /* track size (number of tuples in
                                                                         * partition) of the notnull_info array
                                                                         * for each func args */
+       bool       *notnull_info_cacheable; /* can we cache notnull_info? */
 
        /*
         * Null treatment options. One of: NO_NULLTREATMENT, PARSER_IGNORE_NULLS,
@@ -3518,8 +3519,23 @@ init_notnull_info(WindowObject winobj, WindowStatePerFunc perfuncstate)
 
        if (winobj->ignore_nulls == PARSER_IGNORE_NULLS)
        {
+               int                     argno = 0;
+               ListCell   *lc;
+
                winobj->notnull_info = palloc0_array(uint8 *, numargs);
                winobj->num_notnull_info = palloc0_array(int64, numargs);
+               winobj->notnull_info_cacheable = palloc_array(bool, numargs);
+
+               foreach(lc, perfuncstate->wfunc->args)
+               {
+                       Node       *arg = (Node *) lfirst(lc);
+
+                       winobj->notnull_info_cacheable[argno] =
+                               !contain_volatile_functions(arg) &&
+                               !contain_subplans(arg);
+
+                       argno++;
+               }
        }
 }
 
@@ -3580,6 +3596,9 @@ get_notnull_info(WindowObject winobj, int64 pos, int argno)
        uint8           mb;
        int64           bpos;
 
+       if (!winobj->notnull_info_cacheable[argno])
+               return NN_UNKNOWN;
+
        grow_notnull_info(winobj, pos, argno);
        bpos = NN_POS_TO_BYTES(pos);
        mbp = winobj->notnull_info[argno];
@@ -3603,6 +3622,9 @@ put_notnull_info(WindowObject winobj, int64 pos, int argno, bool isnull)
        uint8           val = isnull ? NN_NULL : NN_NOTNULL;
        int                     shift;
 
+       if (!winobj->notnull_info_cacheable[argno])
+               return;
+
        grow_notnull_info(winobj, pos, argno);
        bpos = NN_POS_TO_BYTES(pos);
        mbp = winobj->notnull_info[argno];
@@ -3812,6 +3834,7 @@ WinGetFuncArgInPartition(WindowObject winobj, int argno,
        int                     notnull_relpos;
        int                     forward;
        bool            myisout;
+       bool            got_datum;
 
        Assert(WindowObjectIsValid(winobj));
        winstate = winobj->winstate;
@@ -3860,6 +3883,7 @@ WinGetFuncArgInPartition(WindowObject winobj, int argno,
        notnull_relpos = abs(relpos);
        forward = relpos > 0 ? 1 : -1;
        myisout = false;
+       got_datum = false;
        datum = 0;
 
        /*
@@ -3905,25 +3929,29 @@ WinGetFuncArgInPartition(WindowObject winobj, int argno,
                {
                        /*
                         * NOT NULL info does not exist yet.  Get tuple and evaluate func
-                        * arg in partition. We ignore the return value from
-                        * gettuple_eval_partition because we are just interested in
-                        * whether we are inside or outside of partition, NULL or NOT
-                        * NULL.
+                        * arg in partition. Keep the return value in case this row is the
+                        * target; re-evaluating a volatile argument could give a
+                        * different nullness status.
                         */
-                       (void) gettuple_eval_partition(winobj, argno,
-                                                                                  abs_pos, isnull, &myisout);
+                       datum = gettuple_eval_partition(winobj, argno,
+                                                                                       abs_pos, isnull, &myisout);
                        if (myisout)            /* out of partition? */
                                break;
                        if (!*isnull)
+                       {
                                notnull_offset++;
+                               if (notnull_offset >= notnull_relpos)
+                                       got_datum = true;
+                       }
                        /* record the row status */
                        put_notnull_info(winobj, abs_pos, argno, *isnull);
                }
        } while (notnull_offset < notnull_relpos);
 
        /* get tuple and evaluate func arg in partition */
-       datum = gettuple_eval_partition(winobj, argno,
-                                                                       abs_pos, isnull, &myisout);
+       if (!got_datum)
+               datum = gettuple_eval_partition(winobj, argno,
+                                                                               abs_pos, isnull, &myisout);
        if (!myisout && set_mark)
                WinSetMarkPosition(winobj, mark_pos);
        if (isout)

diff --git a/src/test/regress/expected/window.out b/src/test/regress/expected/window.out
index e6aac27a2a93e16d75b0374322e675b780cd2c7e..de0e14a686e326969231f9b30c6b980875aded30 100644 (file)
--- a/src/test/regress/expected/window.out
+++ b/src/test/regress/expected/window.out
@@ -5964,6 +5964,72 @@ WINDOW w AS (ORDER BY x ROWS BETWEEN 2 PRECEDING AND 2 FOLLOWING);
  5 |         4
 (5 rows)
 
+-- volatile arguments cannot use the IGNORE NULLS nullness cache
+CREATE TEMPORARY SEQUENCE null_treatment_seq;
+CREATE FUNCTION pg_temp.volatile_null(i int) RETURNS int
+LANGUAGE sql VOLATILE AS
+$$
+  SELECT CASE WHEN nextval('null_treatment_seq') % 2 = 0 THEN i ELSE NULL END;
+$$;
+SELECT x,
+       first_value(pg_temp.volatile_null(x)) IGNORE NULLS
+         OVER (ORDER BY x ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)
+FROM generate_series(1,5) g(x);
+ x | first_value 
+---+-------------
+ 1 |            
+ 2 |           1
+ 3 |           2
+ 4 |           2
+ 5 |           2
+(5 rows)
+
+SELECT last_value FROM null_treatment_seq;
+ last_value 
+------------
+          8
+(1 row)
+
+ALTER SEQUENCE null_treatment_seq RESTART WITH 1;
+SELECT x,
+       lead(pg_temp.volatile_null(x), 1) IGNORE NULLS OVER (ORDER BY x)
+FROM generate_series(1,5) g(x);
+ x | lead 
+---+------
+ 1 |    3
+ 2 |    4
+ 3 |    5
+ 4 |     
+ 5 |     
+(5 rows)
+
+SELECT last_value FROM null_treatment_seq;
+ last_value 
+------------
+          7
+(1 row)
+
+ALTER SEQUENCE null_treatment_seq RESTART WITH 1;
+SELECT x,
+       first_value((SELECT CASE WHEN nextval('null_treatment_seq') % 2 = 0
+                                THEN x ELSE NULL END)) IGNORE NULLS
+         OVER (ORDER BY x ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)
+FROM generate_series(1,5) g(x);
+ x | first_value 
+---+-------------
+ 1 |            
+ 2 |           1
+ 3 |           2
+ 4 |           2
+ 5 |           2
+(5 rows)
+
+SELECT last_value FROM null_treatment_seq;
+ last_value 
+------------
+          8
+(1 row)
+
 --cleanup
 DROP TABLE planets CASCADE;
 NOTICE:  drop cascades to view planets_view

diff --git a/src/test/regress/sql/window.sql b/src/test/regress/sql/window.sql
index 305549b104d202bc76eb0f957e04c829199845f0..17261135dc379352670ee7722a68f6994ed95894 100644 (file)
--- a/src/test/regress/sql/window.sql
+++ b/src/test/regress/sql/window.sql
@@ -2157,5 +2157,33 @@ SELECT x,
 FROM generate_series(1,5) g(x)
 WINDOW w AS (ORDER BY x ROWS BETWEEN 2 PRECEDING AND 2 FOLLOWING);
 
+-- volatile arguments cannot use the IGNORE NULLS nullness cache
+CREATE TEMPORARY SEQUENCE null_treatment_seq;
+CREATE FUNCTION pg_temp.volatile_null(i int) RETURNS int
+LANGUAGE sql VOLATILE AS
+$$
+  SELECT CASE WHEN nextval('null_treatment_seq') % 2 = 0 THEN i ELSE NULL END;
+$$;
+
+SELECT x,
+       first_value(pg_temp.volatile_null(x)) IGNORE NULLS
+         OVER (ORDER BY x ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)
+FROM generate_series(1,5) g(x);
+SELECT last_value FROM null_treatment_seq;
+
+ALTER SEQUENCE null_treatment_seq RESTART WITH 1;
+SELECT x,
+       lead(pg_temp.volatile_null(x), 1) IGNORE NULLS OVER (ORDER BY x)
+FROM generate_series(1,5) g(x);
+SELECT last_value FROM null_treatment_seq;
+
+ALTER SEQUENCE null_treatment_seq RESTART WITH 1;
+SELECT x,
+       first_value((SELECT CASE WHEN nextval('null_treatment_seq') % 2 = 0
+                                THEN x ELSE NULL END)) IGNORE NULLS
+         OVER (ORDER BY x ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)
+FROM generate_series(1,5) g(x);
+SELECT last_value FROM null_treatment_seq;
+
 --cleanup
 DROP TABLE planets CASCADE;