Disallow pushing volatile quals past set-returning functions.

Pushing an upper-level restriction clause into an unflattened
subquery-in-FROM is okay when the subquery contains no SRFs in its
targetlist, or when it does but the SRFs are unreferenced by the clause
*and the clause is not volatile*.  Otherwise, we're changing the number
of times the clause is evaluated, which is bad for volatile quals, and
possibly changing the result, since a volatile qual might succeed for some
SRF output rows and not others despite not referencing any of the changing
columns.  (Indeed, if the clause is something like "random() > 0.5", the
user is probably expecting exactly that behavior.)

We had most of these restrictions down, but not the one about the upper
clause not being volatile.  Fix that, and add a regression test to
illustrate the expected behavior.

Although this is definitely a bug, it doesn't seem like back-patch
material, since possibly some users don't realize that the broken
behavior is broken and are relying on what happens now.  Also, while
the added test is quite cheap in the wake of commit a4c35ea1c, it would
be much more expensive (or else messier) in older branches.

Per report from Tom van Tilburg.

Discussion: <CAP3PPDiucxYCNev52=YPVkrQAPVF1C5PFWnrQPT7iMzO1fiKFQ@mail.gmail.com>

diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 99b6bc8f5a19781cc62202736bfb5ec309f013f1..e42ef98d8d75023dcf52a758c8b32c810621680d 100644 (file)
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -2254,6 +2254,12 @@ standard_join_search(PlannerInfo *root, int levels_needed, List *initial_rels)
  * thereby changing the partition contents and thus the window functions'
  * results for rows that remain.
  *
+ * 5. If the subquery contains any set-returning functions in its targetlist,
+ * we cannot push volatile quals into it.  That would push them below the SRFs
+ * and thereby change the number of times they are evaluated.  Also, a
+ * volatile qual could succeed for some SRF output rows and fail for others,
+ * a behavior that cannot occur if it's evaluated before SRF expansion.
+ *
  * In addition, we make several checks on the subquery's output columns to see
  * if it is safe to reference them in pushed-down quals.  If output column k
  * is found to be unsafe to reference, we set safetyInfo->unsafeColumns[k]
@@ -2298,8 +2304,10 @@ subquery_is_pushdown_safe(Query *subquery, Query *topquery,
        if (subquery->limitOffset != NULL || subquery->limitCount != NULL)
                return false;
 
-       /* Check points 3 and 4 */
-       if (subquery->distinctClause || subquery->hasWindowFuncs)
+       /* Check points 3, 4, and 5 */
+       if (subquery->distinctClause ||
+               subquery->hasWindowFuncs ||
+               subquery->hasTargetSRFs)
                safetyInfo->unsafeVolatile = true;
 
        /*

diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out
index 0fc93d9d7267e40ceef81da6a0d657649a8a6782..eda319d24b56c3d337ab4773c150cd39c5459bbe 100644 (file)
--- a/src/test/regress/expected/subselect.out
+++ b/src/test/regress/expected/subselect.out
@@ -880,3 +880,103 @@ select nextval('ts1');
       11
 (1 row)
 
+--
+-- Check that volatile quals aren't pushed down past a set-returning function;
+-- while a nonvolatile qual can be, if it doesn't reference the SRF.
+--
+create function tattle(x int, y int) returns bool
+volatile language plpgsql as $$
+begin
+  raise notice 'x = %, y = %', x, y;
+  return x > y;
+end$$;
+explain (verbose, costs off)
+select * from
+  (select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
+  where tattle(x, 8);
+                        QUERY PLAN                        
+----------------------------------------------------------
+ Subquery Scan on ss
+   Output: x, u
+   Filter: tattle(ss.x, 8)
+   ->  Result
+         Output: 9, unnest('{1,2,3,11,12,13}'::integer[])
+(5 rows)
+
+select * from
+  (select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
+  where tattle(x, 8);
+NOTICE:  x = 9, y = 8
+NOTICE:  x = 9, y = 8
+NOTICE:  x = 9, y = 8
+NOTICE:  x = 9, y = 8
+NOTICE:  x = 9, y = 8
+NOTICE:  x = 9, y = 8
+ x | u  
+---+----
+ 9 |  1
+ 9 |  2
+ 9 |  3
+ 9 | 11
+ 9 | 12
+ 9 | 13
+(6 rows)
+
+-- if we pretend it's stable, we get different results:
+alter function tattle(x int, y int) stable;
+explain (verbose, costs off)
+select * from
+  (select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
+  where tattle(x, 8);
+                     QUERY PLAN                     
+----------------------------------------------------
+ Result
+   Output: 9, unnest('{1,2,3,11,12,13}'::integer[])
+   One-Time Filter: tattle(9, 8)
+(3 rows)
+
+select * from
+  (select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
+  where tattle(x, 8);
+NOTICE:  x = 9, y = 8
+ x | u  
+---+----
+ 9 |  1
+ 9 |  2
+ 9 |  3
+ 9 | 11
+ 9 | 12
+ 9 | 13
+(6 rows)
+
+-- although even a stable qual should not be pushed down if it references SRF
+explain (verbose, costs off)
+select * from
+  (select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
+  where tattle(x, u);
+                        QUERY PLAN                        
+----------------------------------------------------------
+ Subquery Scan on ss
+   Output: x, u
+   Filter: tattle(ss.x, ss.u)
+   ->  Result
+         Output: 9, unnest('{1,2,3,11,12,13}'::integer[])
+(5 rows)
+
+select * from
+  (select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
+  where tattle(x, u);
+NOTICE:  x = 9, y = 1
+NOTICE:  x = 9, y = 2
+NOTICE:  x = 9, y = 3
+NOTICE:  x = 9, y = 11
+NOTICE:  x = 9, y = 12
+NOTICE:  x = 9, y = 13
+ x | u 
+---+---
+ 9 | 1
+ 9 | 2
+ 9 | 3
+(3 rows)
+
+drop function tattle(x int, y int);

diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql
index 2991223089115546c6882ddf3d1712b22f676a33..08eb825c54296d3e23bb1ecb092c62c34c6cf65f 100644 (file)
--- a/src/test/regress/sql/subselect.sql
+++ b/src/test/regress/sql/subselect.sql
@@ -481,3 +481,47 @@ select * from
   order by 1;
 
 select nextval('ts1');
+
+--
+-- Check that volatile quals aren't pushed down past a set-returning function;
+-- while a nonvolatile qual can be, if it doesn't reference the SRF.
+--
+create function tattle(x int, y int) returns bool
+volatile language plpgsql as $$
+begin
+  raise notice 'x = %, y = %', x, y;
+  return x > y;
+end$$;
+
+explain (verbose, costs off)
+select * from
+  (select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
+  where tattle(x, 8);
+
+select * from
+  (select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
+  where tattle(x, 8);
+
+-- if we pretend it's stable, we get different results:
+alter function tattle(x int, y int) stable;
+
+explain (verbose, costs off)
+select * from
+  (select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
+  where tattle(x, 8);
+
+select * from
+  (select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
+  where tattle(x, 8);
+
+-- although even a stable qual should not be pushed down if it references SRF
+explain (verbose, costs off)
+select * from
+  (select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
+  where tattle(x, u);
+
+select * from
+  (select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
+  where tattle(x, u);
+
+drop function tattle(x int, y int);