I wrote:
>> Heikki Linnakangas <heikki.linnakan...@enterprisedb.com> writes:
>>> Marking textanycat as not immutable would forbid using it in
>>> expression indexes, too.
>> True. On the other hand, the current state of affairs allows one to
>> create an index on expressions that aren't really immutable, with
>> ensuing hilarity.
> It strikes me that we could avoid any possible functional regression
> here by having CREATE INDEX perform expression preprocessing (in
> particular, function inlining) before it tests to see if the index
> expression contains any non-immutable functions.
I looked into this and found that it's a pretty trivial change to make
CREATE INDEX do it that way. Furthermore, this will cover us against
a subtle gotcha that was introduced by the addition of default arguments
for functions: what happens if a marked-as-immutable function has a
volatile default argument? I don't think that's an insane combination,
since the function's own processing might be perfectly immutable. But
right now, CREATE INDEX will draw the wrong conclusion about whether a
function call that uses the default is safe to put into an index.
BTW, I looked around for other places where we might be making the same
mistake. AFAICT, these two checks in CREATE INDEX are the only places
outside the planner that use either contain_mutable_functions or
contain_volatile_functions. The ones inside-the-planner are OK because
they are looking at already-preprocessed expressions.
Perhaps this is a backpatchable bug fix. Comments?
regards, tom lane
Index: src/backend/commands/indexcmds.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/indexcmds.c,v
retrieving revision 1.195
diff -c -r1.195 indexcmds.c
*** src/backend/commands/indexcmds.c 22 Mar 2010 15:24:11 -0000 1.195
--- src/backend/commands/indexcmds.c 17 May 2010 19:50:54 -0000
***************
*** 35,40 ****
--- 35,41 ----
#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
#include "optimizer/clauses.h"
+ #include "optimizer/planner.h"
#include "parser/parse_coerce.h"
#include "parser/parse_func.h"
#include "parser/parse_oper.h"
***************
*** 776,781 ****
--- 777,809 ----
/*
+ * CheckMutability
+ * Test whether given expression is mutable
+ */
+ static bool
+ CheckMutability(Expr *expr)
+ {
+ /*
+ * First run the expression through the planner. This has a couple of
+ * important consequences. 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.
+ *
+ * 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);
+ }
+
+
+ /*
* CheckPredicate
* Checks that the given partial-index predicate is valid.
*
***************
*** 806,812 ****
* A predicate using mutable functions is probably wrong, for the same
* reasons that we don't allow an index expression to use one.
*/
! if (contain_mutable_functions((Node *) predicate))
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("functions in index predicate must be marked IMMUTABLE")));
--- 834,840 ----
* A predicate using mutable functions is probably wrong, for the same
* reasons that we don't allow an index expression to use one.
*/
! if (CheckMutability(predicate))
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("functions in index predicate must be marked IMMUTABLE")));
***************
*** 922,928 ****
* if you aren't going to get the same result for the same data
* every time, it's not clear what the index entries mean at all.
*/
! if (contain_mutable_functions(attribute->expr))
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("functions in index expression must be marked IMMUTABLE")));
--- 950,956 ----
* if you aren't going to get the same result for the same data
* every time, it's not clear what the index entries mean at all.
*/
! if (CheckMutability((Expr *) attribute->expr))
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("functions in index expression must be marked IMMUTABLE")));
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
