From 72daabc7a3e75788df862104b8f723513c2471ae Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Tue, 27 Sep 2016 18:43:36 -0400
Subject: [PATCH] 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>
---
src/backend/optimizer/path/allpaths.c | 12 ++-
src/test/regress/expected/subselect.out | 100 ++++++++++++++++++++++++
src/test/regress/sql/subselect.sql | 44 +++++++++++
3 files changed, 154 insertions(+), 2 deletions(-)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 99b6bc8f5a1..e42ef98d8d7 100644
--- 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 0fc93d9d726..eda319d24b5 100644
--- 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 29912230891..08eb825c542 100644
--- 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);
--
GitLab