I wrote:
> Back at
> http://www.postgresql.org/message-id/520D221E.2060008@gmail.com
> there was a complaint about strange behavior of a query that looks
> basically like this:
> SELECT ...
> FROM
> (SELECT ... ,
> ( SELECT ...
> ORDER BY random()
> LIMIT 1
> ) AS color_id
> FROM ...
> ) s
> LEFT JOIN ...
> ...
> I first wondered why the instance of random() didn't prevent pullup
> in this example. That's because contain_volatile_functions() does
> not recurse into SubLinks, which maybe is the wrong thing; but
> I'm hesitant to change it without detailed analysis of all the
> (many) call sites.
> However, I think that a good case could also be made for fixing this
> by deciding that we shouldn't pull up if there are SubLinks in the
> subquery targetlist, period. Even without any volatile functions,
> multiple copies of a subquery seem like a probable loser cost-wise.
I experimented with the latter behavior and decided it was too invasive;
in particular, it changes the plans chosen for some queries in the
regression tests, and I think it's actually breaking those tests, in the
sense that the queries no longer exercise the planner code paths they
were designed to test.
So I went back to the first idea of allowing contain_volatile_functions()
to recurse into sub-selects. It turns out that this is not as big a deal
as I feared, because recursing into SubLinks will only affect behavior
before the planner has converted SubLinks to SubPlans, and most of the
existing calls to contain_volatile_functions/contain_mutable_functions
are after that and so don't need individual analysis. I've pretty well
convinced myself that looking into sub-selects is a good idea in the
places that examine volatility before that. Accordingly, I propose the
attached patch, which fixes the complained-of behavior but doesn't
change any existing regression test results.
regards, tom lane
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 4fa73a9..add29f5 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*************** contain_subplans_walker(Node *node, void
*** 833,840 ****
* mistakenly think that something like "WHERE random() < 0.5" can be treated
* as a constant qualification.
*
! * XXX we do not examine sub-selects to see if they contain uses of
! * mutable functions. It's not real clear if that is correct or not...
*/
bool
contain_mutable_functions(Node *clause)
--- 833,840 ----
* mistakenly think that something like "WHERE random() < 0.5" can be treated
* as a constant qualification.
*
! * We will recursively look into Query nodes (i.e., SubLink sub-selects)
! * but not into SubPlans. See comments for contain_volatile_functions().
*/
bool
contain_mutable_functions(Node *clause)
*************** contain_mutable_functions_walker(Node *n
*** 931,936 ****
--- 931,943 ----
}
/* else fall through to check args */
}
+ else if (IsA(node, Query))
+ {
+ /* Recurse into subselects */
+ return query_tree_walker((Query *) node,
+ contain_mutable_functions_walker,
+ context, 0);
+ }
return expression_tree_walker(node, contain_mutable_functions_walker,
context);
}
*************** contain_mutable_functions_walker(Node *n
*** 945,955 ****
* Recursively search for volatile functions within a clause.
*
* Returns true if any volatile function (or operator implemented by a
! * volatile function) is found. This test prevents invalid conversions
! * of volatile expressions into indexscan quals.
*
! * XXX we do not examine sub-selects to see if they contain uses of
! * volatile functions. It's not real clear if that is correct or not...
*/
bool
contain_volatile_functions(Node *clause)
--- 952,969 ----
* Recursively search for volatile functions within a clause.
*
* Returns true if any volatile function (or operator implemented by a
! * volatile function) is found. This test prevents, for example,
! * invalid conversions of volatile expressions into indexscan quals.
*
! * We will recursively look into Query nodes (i.e., SubLink sub-selects)
! * but not into SubPlans. This is a bit odd, but intentional. If we are
! * looking at a SubLink, we are probably deciding whether a query tree
! * transformation is safe, and a contained sub-select should affect that;
! * for example, duplicating a sub-select containing a volatile function
! * would be bad. However, once we've got to the stage of having SubPlans,
! * subsequent planning need not consider volatility within those, since
! * the executor won't change its evaluation rules for a SubPlan based on
! * volatility.
*/
bool
contain_volatile_functions(Node *clause)
*************** contain_volatile_functions_walker(Node *
*** 1047,1052 ****
--- 1061,1073 ----
}
/* else fall through to check args */
}
+ else if (IsA(node, Query))
+ {
+ /* Recurse into subselects */
+ return query_tree_walker((Query *) node,
+ contain_volatile_functions_walker,
+ context, 0);
+ }
return expression_tree_walker(node, contain_volatile_functions_walker,
context);
}