1
0
mirror of https://github.com/postgres/postgres.git synced 2025-08-24 09:27:52 +03:00

Ensure we preprocess expressions before checking their volatility.

contain_mutable_functions and contain_volatile_functions give
reliable answers only after expression preprocessing (specifically
eval_const_expressions).  Some places understand this, but some did
not get the memo --- which is not entirely their fault, because the
problem is documented only in places far away from those functions.
Introduce wrapper functions that allow doing the right thing easily,
and add commentary in hopes of preventing future mistakes from
copy-and-paste of code that's only conditionally safe.

Two actual bugs of this ilk are fixed here.  We failed to preprocess
column GENERATED expressions before checking mutability, so that the
code could fail to detect the use of a volatile function
default-argument expression, or it could reject a polymorphic function
that is actually immutable on the datatype of interest.  Likewise,
column DEFAULT expressions weren't preprocessed before determining if
it's safe to apply the attmissingval mechanism.  A false negative
would just result in an unnecessary table rewrite, but a false
positive could allow the attmissingval mechanism to be used in a case
where it should not be, resulting in unexpected initial values in a
new column.

In passing, re-order the steps in ComputePartitionAttrs so that its
checks for invalid column references are done before applying
expression_planner, rather than after.  The previous coding would
not complain if a partition expression contains a disallowed column
reference that gets optimized away by constant folding, which seems
to me to be a behavior we do not want.

Per bug #18097 from Jim Keener.  Back-patch to all supported versions.

Discussion: https://postgr.es/m/18097-ebb179674f22932f@postgresql.org
This commit is contained in:
Tom Lane
2023-11-16 10:05:14 -05:00
parent 2927b1dca7
commit f07a3039c7
10 changed files with 141 additions and 56 deletions

View File

@@ -360,6 +360,11 @@ contain_subplans_walker(Node *node, void *context)
* mistakenly think that something like "WHERE random() < 0.5" can be treated
* as a constant qualification.
*
* This will give the right answer only for clauses that have been put
* through expression preprocessing. Callers outside the planner typically
* should use contain_mutable_functions_after_planning() instead, for the
* reasons given there.
*
* We will recursively look into Query nodes (i.e., SubLink sub-selects)
* but not into SubPlans. See comments for contain_volatile_functions().
*/
@@ -446,6 +451,34 @@ contain_mutable_functions_walker(Node *node, void *context)
context);
}
/*
* contain_mutable_functions_after_planning
* Test whether given expression contains mutable functions.
*
* This is a wrapper for contain_mutable_functions() that is safe to use from
* outside the planner. The difference is that it first runs the expression
* through expression_planner(). There are two key reasons why we need that:
*
* First, function default arguments will get inserted, which may affect
* volatility (consider "default now()").
*
* Second, inline-able functions will get inlined, which may allow us to
* conclude that the function is really less volatile than it's marked.
* As an example, polymorphic functions must be marked with the most volatile
* behavior that they have for any input type, but once we inline the
* function we may be able to conclude that it's not so volatile for the
* particular input type we're dealing with.
*/
bool
contain_mutable_functions_after_planning(Expr *expr)
{
/* We assume here that expression_planner() won't scribble on its input */
expr = expression_planner(expr);
/* Now we can search for non-immutable functions */
return contain_mutable_functions((Node *) expr);
}
/*****************************************************************************
* Check clauses for volatile functions
@@ -459,6 +492,11 @@ contain_mutable_functions_walker(Node *node, void *context)
* volatile function) is found. This test prevents, for example,
* invalid conversions of volatile expressions into indexscan quals.
*
* This will give the right answer only for clauses that have been put
* through expression preprocessing. Callers outside the planner typically
* should use contain_volatile_functions_after_planning() instead, for the
* reasons given there.
*
* 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
@@ -582,6 +620,34 @@ contain_volatile_functions_walker(Node *node, void *context)
context);
}
/*
* contain_volatile_functions_after_planning
* Test whether given expression contains volatile functions.
*
* This is a wrapper for contain_volatile_functions() that is safe to use from
* outside the planner. The difference is that it first runs the expression
* through expression_planner(). There are two key reasons why we need that:
*
* First, function default arguments will get inserted, which may affect
* volatility (consider "default random()").
*
* Second, inline-able functions will get inlined, which may allow us to
* conclude that the function is really less volatile than it's marked.
* As an example, polymorphic functions must be marked with the most volatile
* behavior that they have for any input type, but once we inline the
* function we may be able to conclude that it's not so volatile for the
* particular input type we're dealing with.
*/
bool
contain_volatile_functions_after_planning(Expr *expr)
{
/* We assume here that expression_planner() won't scribble on its input */
expr = expression_planner(expr);
/* Now we can search for volatile functions */
return contain_volatile_functions((Node *) expr);
}
/*
* Special purpose version of contain_volatile_functions() for use in COPY:
* ignore nextval(), but treat all other functions normally.