Skip to content

Commit 72daabc

Browse files
tglsfdctglsfdc
committed
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 a4c35ea, it would be much more expensive (or else messier) in older branches. Per report from Tom van Tilburg. Discussion: <CAP3PPDiucxYCNev52=YPVkrQAPVF1C5PFWnrQPT7iMzO1fiKFQ@mail.gmail.com>
1 parent 0109ab2 commit 72daabc

File tree

3 files changed

+154
-2
lines changed

3 files changed

+154
-2
lines changed

‎src/backend/optimizer/path/allpaths.c

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2254,6 +2254,12 @@ standard_join_search(PlannerInfo *root, int levels_needed, List *initial_rels)
22542254
* thereby changing the partition contents and thus the window functions'
22552255
* results for rows that remain.
22562256
*
2257+
* 5. If the subquery contains any set-returning functions in its targetlist,
2258+
* we cannot push volatile quals into it. That would push them below the SRFs
2259+
* and thereby change the number of times they are evaluated. Also, a
2260+
* volatile qual could succeed for some SRF output rows and fail for others,
2261+
* a behavior that cannot occur if it's evaluated before SRF expansion.
2262+
*
22572263
* In addition, we make several checks on the subquery's output columns to see
22582264
* if it is safe to reference them in pushed-down quals. If output column k
22592265
* is found to be unsafe to reference, we set safetyInfo->unsafeColumns[k]
@@ -2298,8 +2304,10 @@ subquery_is_pushdown_safe(Query *subquery, Query *topquery,
22982304
if (subquery->limitOffset != NULL || subquery->limitCount != NULL)
22992305
return false;
23002306

2301-
/* Check points 3 and 4 */
2302-
if (subquery->distinctClause || subquery->hasWindowFuncs)
2307+
/* Check points 3, 4, and 5 */
2308+
if (subquery->distinctClause ||
2309+
subquery->hasWindowFuncs ||
2310+
subquery->hasTargetSRFs)
23032311
safetyInfo->unsafeVolatile = true;
23042312

23052313
/*

‎src/test/regress/expected/subselect.out

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -880,3 +880,103 @@ select nextval('ts1');
880880
11
881881
(1 row)
882882

883+
--
884+
-- Check that volatile quals aren't pushed down past a set-returning function;
885+
-- while a nonvolatile qual can be, if it doesn't reference the SRF.
886+
--
887+
create function tattle(x int, y int) returns bool
888+
volatile language plpgsql as $$
889+
begin
890+
raise notice 'x = %, y = %', x, y;
891+
return x > y;
892+
end$$;
893+
explain (verbose, costs off)
894+
select * from
895+
(select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
896+
where tattle(x, 8);
897+
QUERY PLAN
898+
----------------------------------------------------------
899+
Subquery Scan on ss
900+
Output: x, u
901+
Filter: tattle(ss.x, 8)
902+
-> Result
903+
Output: 9, unnest('{1,2,3,11,12,13}'::integer[])
904+
(5 rows)
905+
906+
select * from
907+
(select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
908+
where tattle(x, 8);
909+
NOTICE: x = 9, y = 8
910+
NOTICE: x = 9, y = 8
911+
NOTICE: x = 9, y = 8
912+
NOTICE: x = 9, y = 8
913+
NOTICE: x = 9, y = 8
914+
NOTICE: x = 9, y = 8
915+
x | u
916+
---+----
917+
9 | 1
918+
9 | 2
919+
9 | 3
920+
9 | 11
921+
9 | 12
922+
9 | 13
923+
(6 rows)
924+
925+
-- if we pretend it's stable, we get different results:
926+
alter function tattle(x int, y int) stable;
927+
explain (verbose, costs off)
928+
select * from
929+
(select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
930+
where tattle(x, 8);
931+
QUERY PLAN
932+
----------------------------------------------------
933+
Result
934+
Output: 9, unnest('{1,2,3,11,12,13}'::integer[])
935+
One-Time Filter: tattle(9, 8)
936+
(3 rows)
937+
938+
select * from
939+
(select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
940+
where tattle(x, 8);
941+
NOTICE: x = 9, y = 8
942+
x | u
943+
---+----
944+
9 | 1
945+
9 | 2
946+
9 | 3
947+
9 | 11
948+
9 | 12
949+
9 | 13
950+
(6 rows)
951+
952+
-- although even a stable qual should not be pushed down if it references SRF
953+
explain (verbose, costs off)
954+
select * from
955+
(select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
956+
where tattle(x, u);
957+
QUERY PLAN
958+
----------------------------------------------------------
959+
Subquery Scan on ss
960+
Output: x, u
961+
Filter: tattle(ss.x, ss.u)
962+
-> Result
963+
Output: 9, unnest('{1,2,3,11,12,13}'::integer[])
964+
(5 rows)
965+
966+
select * from
967+
(select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
968+
where tattle(x, u);
969+
NOTICE: x = 9, y = 1
970+
NOTICE: x = 9, y = 2
971+
NOTICE: x = 9, y = 3
972+
NOTICE: x = 9, y = 11
973+
NOTICE: x = 9, y = 12
974+
NOTICE: x = 9, y = 13
975+
x | u
976+
---+---
977+
9 | 1
978+
9 | 2
979+
9 | 3
980+
(3 rows)
981+
982+
drop function tattle(x int, y int);

‎src/test/regress/sql/subselect.sql

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -481,3 +481,47 @@ select * from
481481
order by 1;
482482

483483
select nextval('ts1');
484+
485+
--
486+
-- Check that volatile quals aren't pushed down past a set-returning function;
487+
-- while a nonvolatile qual can be, if it doesn't reference the SRF.
488+
--
489+
create function tattle(x int, y int) returns bool
490+
volatile language plpgsql as $$
491+
begin
492+
raise notice 'x = %, y = %', x, y;
493+
return x > y;
494+
end$$;
495+
496+
explain (verbose, costs off)
497+
select * from
498+
(select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
499+
where tattle(x, 8);
500+
501+
select * from
502+
(select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
503+
where tattle(x, 8);
504+
505+
-- if we pretend it's stable, we get different results:
506+
alter function tattle(x int, y int) stable;
507+
508+
explain (verbose, costs off)
509+
select * from
510+
(select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
511+
where tattle(x, 8);
512+
513+
select * from
514+
(select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
515+
where tattle(x, 8);
516+
517+
-- although even a stable qual should not be pushed down if it references SRF
518+
explain (verbose, costs off)
519+
select * from
520+
(select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
521+
where tattle(x, u);
522+
523+
select * from
524+
(select 9 as x, unnest(array[1,2,3,11,12,13]) as u) ss
525+
where tattle(x, u);
526+
527+
drop function tattle(x int, y int);

0 commit comments

Comments
 (0)