Skip to content

Commit af38d14

Browse files
tglsfdctglsfdc
committed
Make contain_volatile_functions/contain_mutable_functions look into SubLinks.
This change prevents us from doing inappropriate subquery flattening in cases such as dangerous functions hidden inside a sub-SELECT in the targetlist of another sub-SELECT. That could result in unexpected behavior due to multiple evaluations of a volatile function, as in a recent complaint from Etienne Dube. It's been questionable from the very beginning whether these functions should look into subqueries (as noted in their comments), and this case seems to provide proof that they should. Because the new code only descends into SubLinks, not SubPlans or InitPlans, the change only affects the planner's behavior during prepjointree processing and not later on --- for example, you can still get it to use a volatile function in an indexqual if you wrap the function in (SELECT ...). That's a historical behavior, for sure, but it's reasonable given that the executor's evaluation rules for subplans don't depend on whether there are volatile functions inside them. In any case, we need to constrain the behavioral change as narrowly as we can to make this reasonable to back-patch.
1 parent 8cfd4c6 commit af38d14

File tree

3 files changed

+107
-6
lines changed

3 files changed

+107
-6
lines changed

‎src/backend/optimizer/util/clauses.c

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -786,8 +786,8 @@ contain_subplans_walker(Node *node, void *context)
786786
* mistakenly think that something like "WHERE random() < 0.5" can be treated
787787
* as a constant qualification.
788788
*
789-
* XXX we do not examine sub-selects to see if they contain uses of
790-
* mutable functions. It's not real clear if that is correct or not...
789+
* We will recursively look into Query nodes (i.e., SubLink sub-selects)
790+
* but not into SubPlans. See comments for contain_volatile_functions().
791791
*/
792792
bool
793793
contain_mutable_functions(Node *clause)
@@ -884,6 +884,13 @@ contain_mutable_functions_walker(Node *node, void *context)
884884
}
885885
/* else fall through to check args */
886886
}
887+
else if (IsA(node, Query))
888+
{
889+
/* Recurse into subselects */
890+
return query_tree_walker((Query *) node,
891+
contain_mutable_functions_walker,
892+
context, 0);
893+
}
887894
return expression_tree_walker(node, contain_mutable_functions_walker,
888895
context);
889896
}
@@ -898,11 +905,18 @@ contain_mutable_functions_walker(Node *node, void *context)
898905
* Recursively search for volatile functions within a clause.
899906
*
900907
* Returns true if any volatile function (or operator implemented by a
901-
* volatile function) is found. This test prevents invalid conversions
902-
* of volatile expressions into indexscan quals.
908+
* volatile function) is found. This test prevents, for example,
909+
* invalid conversions of volatile expressions into indexscan quals.
903910
*
904-
* XXX we do not examine sub-selects to see if they contain uses of
905-
* volatile functions. It's not real clear if that is correct or not...
911+
* We will recursively look into Query nodes (i.e., SubLink sub-selects)
912+
* but not into SubPlans. This is a bit odd, but intentional. If we are
913+
* looking at a SubLink, we are probably deciding whether a query tree
914+
* transformation is safe, and a contained sub-select should affect that;
915+
* for example, duplicating a sub-select containing a volatile function
916+
* would be bad. However, once we've got to the stage of having SubPlans,
917+
* subsequent planning need not consider volatility within those, since
918+
* the executor won't change its evaluation rules for a SubPlan based on
919+
* volatility.
906920
*/
907921
bool
908922
contain_volatile_functions(Node *clause)
@@ -1000,6 +1014,13 @@ contain_volatile_functions_walker(Node *node, void *context)
10001014
}
10011015
/* else fall through to check args */
10021016
}
1017+
else if (IsA(node, Query))
1018+
{
1019+
/* Recurse into subselects */
1020+
return query_tree_walker((Query *) node,
1021+
contain_volatile_functions_walker,
1022+
context, 0);
1023+
}
10031024
return expression_tree_walker(node, contain_volatile_functions_walker,
10041025
context);
10051026
}

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

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -639,3 +639,67 @@ where a.thousand = b.thousand
639639
----------
640640
(0 rows)
641641

642+
--
643+
-- Check that nested sub-selects are not pulled up if they contain volatiles
644+
--
645+
explain (verbose, costs off)
646+
select x, x from
647+
(select (select now()) as x from (values(1),(2)) v(y)) ss;
648+
QUERY PLAN
649+
---------------------------
650+
Values Scan on "*VALUES*"
651+
Output: $0, $1
652+
InitPlan 1 (returns $0)
653+
-> Result
654+
Output: now()
655+
InitPlan 2 (returns $1)
656+
-> Result
657+
Output: now()
658+
(8 rows)
659+
660+
explain (verbose, costs off)
661+
select x, x from
662+
(select (select random()) as x from (values(1),(2)) v(y)) ss;
663+
QUERY PLAN
664+
----------------------------------
665+
Subquery Scan on ss
666+
Output: ss.x, ss.x
667+
-> Values Scan on "*VALUES*"
668+
Output: $0
669+
InitPlan 1 (returns $0)
670+
-> Result
671+
Output: random()
672+
(7 rows)
673+
674+
explain (verbose, costs off)
675+
select x, x from
676+
(select (select now() where y=y) as x from (values(1),(2)) v(y)) ss;
677+
QUERY PLAN
678+
----------------------------------------------------------------------
679+
Values Scan on "*VALUES*"
680+
Output: (SubPlan 1), (SubPlan 2)
681+
SubPlan 1
682+
-> Result
683+
Output: now()
684+
One-Time Filter: ("*VALUES*".column1 = "*VALUES*".column1)
685+
SubPlan 2
686+
-> Result
687+
Output: now()
688+
One-Time Filter: ("*VALUES*".column1 = "*VALUES*".column1)
689+
(10 rows)
690+
691+
explain (verbose, costs off)
692+
select x, x from
693+
(select (select random() where y=y) as x from (values(1),(2)) v(y)) ss;
694+
QUERY PLAN
695+
----------------------------------------------------------------------------
696+
Subquery Scan on ss
697+
Output: ss.x, ss.x
698+
-> Values Scan on "*VALUES*"
699+
Output: (SubPlan 1)
700+
SubPlan 1
701+
-> Result
702+
Output: random()
703+
One-Time Filter: ("*VALUES*".column1 = "*VALUES*".column1)
704+
(8 rows)
705+

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -389,3 +389,19 @@ where a.thousand = b.thousand
389389
and exists ( select 1 from tenk1 c where b.hundred = c.hundred
390390
and not exists ( select 1 from tenk1 d
391391
where a.thousand = d.thousand ) );
392+
393+
--
394+
-- Check that nested sub-selects are not pulled up if they contain volatiles
395+
--
396+
explain (verbose, costs off)
397+
select x, x from
398+
(select (select now()) as x from (values(1),(2)) v(y)) ss;
399+
explain (verbose, costs off)
400+
select x, x from
401+
(select (select random()) as x from (values(1),(2)) v(y)) ss;
402+
explain (verbose, costs off)
403+
select x, x from
404+
(select (select now() where y=y) as x from (values(1),(2)) v(y)) ss;
405+
explain (verbose, costs off)
406+
select x, x from
407+
(select (select random() where y=y) as x from (values(1),(2)) v(y)) ss;

0 commit comments

Comments
 (0)