Обсуждение: Performance problem in textanycat/anytextcat
I noticed by accident that there are some cases where the planner fails
to inline the SQL functions that underlie the || operator for text vs
non-text cases. The reason is that these functions are marked
immutable, but their expansion involves a coercion to text that might
not be immutable. The planner will not inline a function if the
resulting expression would be more volatile than the function claims
itself to be, because sometimes the point of such a function is to hide
the expression's volatility. In this case, however, we don't want to
hide the true nature of the expression, and we definitely don't want
to pay the performance price of calling a SQL function. That price
is pretty significant, eg on a rather slow machine I get
regression=# select count(localtimestamp || i::text) from generate_series(1,100000) i;count
--------100000
(1 row)
Time: 12512.624 ms
regression=# update pg_proc set provolatile = 'v' where oid = 2004;
UPDATE 1
Time: 7.104 ms
regression=# select count(localtimestamp || i::text) from generate_series(1,100000) i;count
--------100000
(1 row)
Time: 4967.086 ms
so the added overhead more than doubles the cost of this case.
There's also a possibility of an outright wrong behavior, since the
immutable marking will allow the concatenation of two constants to
be folded to a constant in contexts where perhaps it shouldn't be.
Continuing the above example, 'now'::timestamp || 'foo' will be folded
to a constant on sight, which is wrong because the coercion to text
depends on DateStyle and ought to react to a later change in DateStyle.
So I think that labeling textanycat/anytextcat as immutable was a
thinko, and we instead ought to label them volatile so that the planner
can inline them no matter what the behavior of the underlying text cast
is.
Is it reasonable to fix this now, and if so should I bump catversion
or leave it alone? My own preference is to fix it in pg_proc.h but
not touch catversion; but you could argue that different ways.
regards, tom lane
On Sat, May 15, 2010 at 8:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Is it reasonable to fix this now, and if so should I bump catversion > or leave it alone? My own preference is to fix it in pg_proc.h but > not touch catversion; but you could argue that different ways. > are you planning to backpatch this? if so, i say no to bump catversion but only mention in the release notes that if you are upgrading you have to make those updates manually... we have made that before... otherwise we will require an initdb for minor version upgrade and being that no one noted this before that seems excessive to me, IMHO -- Jaime Casanova www.2ndQuadrant.com Soporte y capacitación de PostgreSQL
Jaime Casanova <jaime@2ndquadrant.com> writes:
> On Sat, May 15, 2010 at 8:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Is it reasonable to fix this now, and if so should I bump catversion
>> or leave it alone? My own preference is to fix it in pg_proc.h but
>> not touch catversion; but you could argue that different ways.
> are you planning to backpatch this?
I wasn't planning to; as you say, without field complaints it doesn't
seem compelling to fix in existing releases.
regards, tom lane
On Sat, May 15, 2010 at 10:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Jaime Casanova <jaime@2ndquadrant.com> writes: >> On Sat, May 15, 2010 at 8:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Is it reasonable to fix this now, and if so should I bump catversion >>> or leave it alone? My own preference is to fix it in pg_proc.h but >>> not touch catversion; but you could argue that different ways. > >> are you planning to backpatch this? > > I wasn't planning to; as you say, without field complaints it doesn't > seem compelling to fix in existing releases. > ok, then is up to you if you think that it is worth an initdb in beta... i still think is excessive... btw, is it worth documenting that somewhere for older releases? -- Jaime Casanova www.2ndQuadrant.com Soporte y capacitación de PostgreSQL
Jaime Casanova <jaime@2ndquadrant.com> writes:
> On Sat, May 15, 2010 at 10:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Jaime Casanova <jaime@2ndquadrant.com> writes:
>>> On Sat, May 15, 2010 at 8:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Is it reasonable to fix this now, and if so should I bump catversion
>>> or leave it alone? My own preference is to fix it in pg_proc.h but
>>> not touch catversion; but you could argue that different ways.
> ok, then is up to you if you think that it is worth an initdb in
> beta... i still think is excessive...
The point of not wanting to change catversion is to not force an
initdb.
regards, tom lane
On Sat, May 15, 2010 at 10:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Jaime Casanova <jaime@2ndquadrant.com> writes: >> On Sat, May 15, 2010 at 10:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Jaime Casanova <jaime@2ndquadrant.com> writes: >>>> On Sat, May 15, 2010 at 8:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>> Is it reasonable to fix this now, and if so should I bump catversion >>>> or leave it alone? My own preference is to fix it in pg_proc.h but >>>> not touch catversion; but you could argue that different ways. > >> ok, then is up to you if you think that it is worth an initdb in >> beta... i still think is excessive... > > The point of not wanting to change catversion is to not force an > initdb. > ah! yeah! you are the one that doesn't want to touch catversion, so i'm barking at the wrong tree then. i have been busy and need to rest a little ;) +1 to not touch catversion -- Jaime Casanova www.2ndQuadrant.com Soporte y capacitación de PostgreSQL
Tom Lane wrote: > So I think that labeling textanycat/anytextcat as immutable was a > thinko, and we instead ought to label them volatile so that the planner > can inline them no matter what the behavior of the underlying text cast > is. That feels backwards, having to label functions as more volatile than they really are, just to allow optimizations elsewhere. Marking textanycat as not immutable would forbid using it in expression indexes, too. > ... The planner will not inline a function if the > resulting expression would be more volatile than the function claims > itself to be, because sometimes the point of such a function is to > hide the expression's volatility. ... Could we inline the function anyway, if it came from an operator? Presumably if you want to hide an expresssion's volatility, you use an explicit function call - I can't imagine using an operator for that purpose. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Tom Lane wrote:
>> So I think that labeling textanycat/anytextcat as immutable was a
>> thinko, and we instead ought to label them volatile so that the planner
>> can inline them no matter what the behavior of the underlying text cast
>> is.
> That feels backwards, having to label functions as more volatile than
> they really are, just to allow optimizations elsewhere.
Well, it's also bogus to label functions as less volatile than they
really are. An error in that direction is unsafe, while marking a
function as more volatile than it truly is cannot result in wrong
behavior.
> 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.
The basic problem here is that these functions are polymorphic and their
actual volatility can vary depending on the argument datatype. We don't
have any way to describe that in the present pg_proc definition. It
does seem like we ought to think about improving this, but that's
clearly a research project. In terms of what we could reasonably do
for 9.0, I think it's change the volatility marking or nothing ...
and changing the marking looks like the better bet to me.
>> ... The planner will not inline a function if the
>> resulting expression would be more volatile than the function claims
>> itself to be, because sometimes the point of such a function is to
>> hide the expression's volatility. ...
> Could we inline the function anyway, if it came from an operator?
No, that's just a crock with no semantic justification at all.
regards, tom lane
I wrote:
> Heikki Linnakangas <heikki.linnakangas@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.
regards, tom lane
On Sat, May 15, 2010 at 9:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I noticed by accident that there are some cases where the planner fails > to inline the SQL functions that underlie the || operator for text vs > non-text cases. The reason is that these functions are marked > immutable, but their expansion involves a coercion to text that might > not be immutable. The planner will not inline a function if the > resulting expression would be more volatile than the function claims > itself to be, because sometimes the point of such a function is to hide > the expression's volatility. In this case, however, we don't want to > hide the true nature of the expression, and we definitely don't want > to pay the performance price of calling a SQL function. That price > is pretty significant, eg on a rather slow machine I get > > regression=# select count(localtimestamp || i::text) from generate_series(1,100000) i; > count > -------- > 100000 > (1 row) > > Time: 12512.624 ms > regression=# update pg_proc set provolatile = 'v' where oid = 2004; > UPDATE 1 > Time: 7.104 ms > regression=# select count(localtimestamp || i::text) from generate_series(1,100000) i; > count > -------- > 100000 > (1 row) > > Time: 4967.086 ms > > so the added overhead more than doubles the cost of this case. > > There's also a possibility of an outright wrong behavior, since the > immutable marking will allow the concatenation of two constants to > be folded to a constant in contexts where perhaps it shouldn't be. > Continuing the above example, 'now'::timestamp || 'foo' will be folded > to a constant on sight, which is wrong because the coercion to text > depends on DateStyle and ought to react to a later change in DateStyle. > > So I think that labeling textanycat/anytextcat as immutable was a > thinko, and we instead ought to label them volatile so that the planner > can inline them no matter what the behavior of the underlying text cast > is. Couldn't you apply this argument to any built-in immutable function whatsoever? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> writes:
> Couldn't you apply this argument to any built-in immutable function whatsoever?
No, only the ones that are built on top of other functions that aren't
immutable.
I did go looking for other potential problems of the same ilk. The only
one I can find at present is to_timestamp(double), which is an immutable
SQL function but it uses timestamptz + interval, which is marked as not
immutable. I believe the reason for that is that if the interval
includes month or day components then the addition result can depend on
the timezone setting. However, the usage in to_timestamp() involves only
a pure seconds component so the immutable marking should be correct.
Still, we might want to think about reimplementing to_timestamp() as a
pure C function sometime, because the current implementation is many
times slower than it needs to be.
regards, tom lane
On Sun, May 16, 2010 at 1:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> Couldn't you apply this argument to any built-in immutable function whatsoever? > > No, only the ones that are built on top of other functions that aren't > immutable. Built on top of? I don't get it. It seems like anything of the form immutablefunction(volatilefunction()) is vulnerable to this, and you can give a volatile function as an argument to any function you like. If you're saying we're testing for immutability by looking only at the outermost function call, that seems pretty broken. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
On Sun, May 16, 2010 at 1:20 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Sun, May 16, 2010 at 1:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Robert Haas <robertmhaas@gmail.com> writes: >>> Couldn't you apply this argument to any built-in immutable function whatsoever? >> >> No, only the ones that are built on top of other functions that aren't >> immutable. > > Built on top of? I don't get it. It seems like anything of the form > immutablefunction(volatilefunction()) is vulnerable to this, and you > can give a volatile function as an argument to any function you like. > If you're saying we're testing for immutability by looking only at the > outermost function call, that seems pretty broken. > you mean we shouldn't allow this? """ select version(); version -------------------------------------------------------------------------------------------------------------------PostgreSQL 8.4.0on x86_64-unknown-linux-gnu, compiled by GCC gcc (GCC) 3.4.6 20060404 (Red Hat 3.4.6-10), 64-bit (1 row) create table t1 (col1 int); create function f1(int) returns double precision as $$ select random() * $1; $$ language sql immutable; create index idx on t1(f1(col1)); """ then, welcome to the club... there were various conversations on this same topic -- Jaime Casanova www.2ndQuadrant.com Soporte y capacitación de PostgreSQL
Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, May 16, 2010 at 1:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> No, only the ones that are built on top of other functions that aren't
>> immutable.
> Built on top of? I don't get it. It seems like anything of the form
> immutablefunction(volatilefunction()) is vulnerable to this,
Uh, no, you misunderstand completely. The problematic case is where the
function's own expansion contains a non-immutable function. In
particular, what we have for these functions is that textanycat(a,b)
expands to a || b::text, and depending on what the type of b is, the
cast from that to text might not be immutable. This is entirely
independent of whether the argument expressions are volatile or not.
Rather, the problem is that inlining the function definition could
by itself increase the expression's apparent volatility. (Decreasing
the volatility is not problematic. Increasing it is.)
regards, tom lane
On Sun, May 16, 2010 at 2:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Sun, May 16, 2010 at 1:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> No, only the ones that are built on top of other functions that aren't >>> immutable. > >> Built on top of? I don't get it. It seems like anything of the form >> immutablefunction(volatilefunction()) is vulnerable to this, > > Uh, no, you misunderstand completely. The problematic case is where the > function's own expansion contains a non-immutable function. In > particular, what we have for these functions is that textanycat(a,b) > expands to a || b::text, and depending on what the type of b is, the > cast from that to text might not be immutable. This is entirely > independent of whether the argument expressions are volatile or not. > Rather, the problem is that inlining the function definition could > by itself increase the expression's apparent volatility. (Decreasing > the volatility is not problematic. Increasing it is.) I guess my point is that the actual volatility of an expression is presumably independent of whether it gets inlined. (If inlining is changing the semantics, that's a problem.) So if inlining is changing it's apparent volatility, then there's something wrong with the way we're computing apparent volatility. No? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> writes:
> I guess my point is that the actual volatility of an expression is
> presumably independent of whether it gets inlined. (If inlining is
> changing the semantics, that's a problem.) So if inlining is changing
> it's apparent volatility, then there's something wrong with the way
> we're computing apparent volatility. No?
Well, the way we're computing volatility is certainly an
oversimplification of reality, as was already noted upthread --- the
fundamental issue here is that polymorphism of the textcat functions
renders it impossible to know their true volatility status without
knowing the specific datatype they're being applied to. Perhaps we
could generalize the pg_proc representation to accommodate that, but
it's certainly in the nature of a research project. And there are
*many* cases where we're already approximating for other reasons; the
timestamptz plus interval case that I mentioned earlier is one. So long
as any approximations are in the direction of supposing the expression
is more volatile than it really is, they are not dangerous. But right
at the moment, the textcat functions are on the wrong side of that,
because they're claiming to be immutable when they sometimes aren't.
regards, tom lane
I wrote:
>> Heikki Linnakangas <heikki.linnakangas@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")));
On Mon, May 17, 2010 at 4:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wrote: >>> Heikki Linnakangas <heikki.linnakangas@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? I can't say whether this is safe enough to back-patch, but the way this is set up, don't we also need to fix some catalog entries and, if yes, isn't that problematic? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, May 17, 2010 at 4:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Perhaps this is a backpatchable bug fix. �Comments?
> I can't say whether this is safe enough to back-patch, but the way
> this is set up, don't we also need to fix some catalog entries and, if
> yes, isn't that problematic?
The only catalog entries at issue, AFAICT, are the textanycat/anytextcat
ones. I am not sure whether we should attempt to back-patch changes for
them, but this patch wouldn't make the situation in the back branches
worse. In particular, if we apply this patch but don't change the
catalog entries, then nothing would change at all about the problematic
cases, because the planner would decide it couldn't safely inline the
function. The only cases where inlining will happen is where the
expression's apparent volatility stays the same or decreases, so as far
as that issue is concerned this patch will never make CREATE INDEX
reject a case it would have accepted otherwise. The patch *will* make
CREATE INDEX reject cases with volatile default arguments hiding under
non-volatile functions, but that's got nothing to do with any built-in
functions; and that's the case I claim is clearly a bug fix.
regards, tom lane
On Mon, May 17, 2010 at 9:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Mon, May 17, 2010 at 4:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Perhaps this is a backpatchable bug fix. Comments? > >> I can't say whether this is safe enough to back-patch, but the way >> this is set up, don't we also need to fix some catalog entries and, if >> yes, isn't that problematic? > > The only catalog entries at issue, AFAICT, are the textanycat/anytextcat > ones. I am not sure whether we should attempt to back-patch changes for > them, but this patch wouldn't make the situation in the back branches > worse. In particular, if we apply this patch but don't change the > catalog entries, then nothing would change at all about the problematic > cases, because the planner would decide it couldn't safely inline the > function. The only cases where inlining will happen is where the > expression's apparent volatility stays the same or decreases, so as far > as that issue is concerned this patch will never make CREATE INDEX > reject a case it would have accepted otherwise. The patch *will* make > CREATE INDEX reject cases with volatile default arguments hiding under > non-volatile functions, but that's got nothing to do with any built-in > functions; and that's the case I claim is clearly a bug fix. Well, I guess it boils down to what you think the chances that this change will unintentionally break something are, then. I don't have a good feeling for that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
On Mon, May 17, 2010 at 9:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Mon, May 17, 2010 at 4:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Perhaps this is a backpatchable bug fix. Comments? > >> I can't say whether this is safe enough to back-patch, but the way >> this is set up, don't we also need to fix some catalog entries and, if >> yes, isn't that problematic? > > The only catalog entries at issue, AFAICT, are the textanycat/anytextcat > ones. I am not sure whether we should attempt to back-patch changes for > them, but this patch wouldn't make the situation in the back branches > worse. In particular, if we apply this patch but don't change the > catalog entries, then nothing would change at all about the problematic > cases, because the planner would decide it couldn't safely inline the > function. The only cases where inlining will happen is where the > expression's apparent volatility stays the same or decreases, so as far > as that issue is concerned this patch will never make CREATE INDEX > reject a case it would have accepted otherwise. The patch *will* make > CREATE INDEX reject cases with volatile default arguments hiding under > non-volatile functions, but that's got nothing to do with any built-in > functions; and that's the case I claim is clearly a bug fix. This is still on the 9.0 open items list, but ISTM you fixed it with two commits on May 27th. Is that correct? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> writes:
> This is still on the 9.0 open items list, but ISTM you fixed it with
> two commits on May 27th. Is that correct?
Oh, sorry, forgot to update the open items. Done now.
regards, tom lane