Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-29 Thread Robert Haas
On Mon, May 17, 2010 at 9:23 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Mon, May 17, 2010 at 4:01 PM, Tom Lane t...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-29 Thread Tom Lane
Robert Haas robertmh...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-17 Thread Tom Lane
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 -	1.195
--- src/backend/commands/indexcmds.c	17 May 2010 19:50:54 -
***
*** 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


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-17 Thread Robert Haas
On Mon, May 17, 2010 at 4:01 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 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?

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-17 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Mon, May 17, 2010 at 4:01 PM, Tom Lane t...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-17 Thread Robert Haas
On Mon, May 17, 2010 at 9:23 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Mon, May 17, 2010 at 4:01 PM, Tom Lane t...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-16 Thread Heikki Linnakangas
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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-16 Thread Tom Lane
Heikki Linnakangas heikki.linnakan...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-16 Thread Tom Lane
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.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-16 Thread Robert Haas
On Sat, May 15, 2010 at 9:51 PM, Tom Lane t...@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,10) i;
  count
 
  10
 (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,10) i;
  count
 
  10
 (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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-16 Thread Tom Lane
Robert Haas robertmh...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-16 Thread Robert Haas
On Sun, May 16, 2010 at 1:11 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-16 Thread Jaime Casanova
On Sun, May 16, 2010 at 1:20 PM, Robert Haas robertmh...@gmail.com wrote:
 On Sun, May 16, 2010 at 1:11 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@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.0 on 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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-16 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Sun, May 16, 2010 at 1:11 PM, Tom Lane t...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-16 Thread Robert Haas
On Sun, May 16, 2010 at 2:59 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Sun, May 16, 2010 at 1:11 PM, Tom Lane t...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-16 Thread Tom Lane
Robert Haas robertmh...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Performance problem in textanycat/anytextcat

2010-05-15 Thread Tom Lane
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,10) i;
 count  

 10
(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,10) i;
 count  

 10
(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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-15 Thread Jaime Casanova
On Sat, May 15, 2010 at 8:51 PM, Tom Lane t...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-15 Thread Tom Lane
Jaime Casanova ja...@2ndquadrant.com writes:
 On Sat, May 15, 2010 at 8:51 PM, Tom Lane t...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-15 Thread Jaime Casanova
On Sat, May 15, 2010 at 10:16 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Jaime Casanova ja...@2ndquadrant.com writes:
 On Sat, May 15, 2010 at 8:51 PM, Tom Lane t...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-15 Thread Tom Lane
Jaime Casanova ja...@2ndquadrant.com writes:
 On Sat, May 15, 2010 at 10:16 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Jaime Casanova ja...@2ndquadrant.com writes:
 On Sat, May 15, 2010 at 8:51 PM, Tom Lane t...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Performance problem in textanycat/anytextcat

2010-05-15 Thread Jaime Casanova
On Sat, May 15, 2010 at 10:21 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Jaime Casanova ja...@2ndquadrant.com writes:
 On Sat, May 15, 2010 at 10:16 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Jaime Casanova ja...@2ndquadrant.com writes:
 On Sat, May 15, 2010 at 8:51 PM, Tom Lane t...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers