dynamic SQL - possible performance regression in 9.2

On 12/27/12 1:07 AM, Pavel Stehule wrote:

Hello

I rechecked performance of dynamic SQL and it is significantly slower
in 9.2 than 9.1

-- 9.1
postgres=# create or replace function test() returns void as $$ begin
for i in 1..1000000 loop execute 'select 1'; end loop; end $$ language
plpgsql;

I think this is the same as the case discussed at
<CAD4+=qWnGU0qi+iq=EPh6EGPuUnSCYsGDTgKazizEvrGgjo0Sg@mail.gmail.com>.

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

On 28.12.2012 23:53, Peter Eisentraut wrote:

On 12/27/12 1:07 AM, Pavel Stehule wrote:

Hello

I rechecked performance of dynamic SQL and it is significantly slower
in 9.2 than 9.1

-- 9.1
postgres=# create or replace function test() returns void as $$ begin
for i in 1..1000000 loop execute 'select 1'; end loop; end $$ language
plpgsql;

I think this is the same as the case discussed at
<CAD4+=qWnGU0qi+iq=EPh6EGPuUnSCYsGDTgKazizEvrGgjo0Sg@mail.gmail.com>.

Yeah, probably so.

As it happens, I just spent a lot of time today narrowing down yet
another report of a regression in 9.2, when running DBT-2:
http://archives.postgresql.org/pgsql-performance/2012-11/msg00007.php.
It looks like that is also caused by the plancache changes. DBT-2
implements the transactions using C functions, which use SPI_execute()
to run all the queries.

It looks like the regression is caused by extra copying of the parse
tree and plan trees. Node-copy-related functions like AllocSetAlloc and
_copy* are high in the profile, They are also high in the 9.1 profile,
but even more so in 9.2.

I hacked together a quick&dirty patch to reduce the copying of
single-shot plans, and was able to buy back much of the regression I was
seeing on DBT-2. Patch attached. But of course, DBT-2 really should be
preparing the queries once with SPI_prepare, and reusing them thereafter.

- Heikki

Hello

2012/12/28 Heikki Linnakangas <hlinnakangas@vmware.com>:

On 28.12.2012 23:53, Peter Eisentraut wrote:

On 12/27/12 1:07 AM, Pavel Stehule wrote:

Hello

I rechecked performance of dynamic SQL and it is significantly slower
in 9.2 than 9.1

-- 9.1
postgres=# create or replace function test() returns void as $$ begin
for i in 1..1000000 loop execute 'select 1'; end loop; end $$ language
plpgsql;

I think this is the same as the case discussed at
<CAD4+=qWnGU0qi+iq=EPh6EGPuUnSCYsGDTgKazizEvrGgjo0Sg@mail.gmail.com>.

Yeah, probably so.

As it happens, I just spent a lot of time today narrowing down yet another
report of a regression in 9.2, when running DBT-2:
http://archives.postgresql.org/pgsql-performance/2012-11/msg00007.php. It
looks like that is also caused by the plancache changes. DBT-2 implements
the transactions using C functions, which use SPI_execute() to run all the
queries.

It looks like the regression is caused by extra copying of the parse tree
and plan trees. Node-copy-related functions like AllocSetAlloc and _copy*
are high in the profile, They are also high in the 9.1 profile, but even
more so in 9.2.

I hacked together a quick&dirty patch to reduce the copying of single-shot
plans, and was able to buy back much of the regression I was seeing on
DBT-2. Patch attached. But of course, DBT-2 really should be preparing the
queries once with SPI_prepare, and reusing them thereafter.

performance regression is about 30-50%.

You copy_reduce_patch increase speed about 8%

Regards

Pavel

- Heikki

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

On 12/28/12 5:11 PM, Heikki Linnakangas wrote:

As it happens, I just spent a lot of time today narrowing down yet
another report of a regression in 9.2, when running DBT-2:
http://archives.postgresql.org/pgsql-performance/2012-11/msg00007.php.
It looks like that is also caused by the plancache changes. DBT-2
implements the transactions using C functions, which use SPI_execute()
to run all the queries.

It looks like the regression is caused by extra copying of the parse
tree and plan trees. Node-copy-related functions like AllocSetAlloc and
_copy* are high in the profile, They are also high in the 9.1 profile,
but even more so in 9.2.

I hacked together a quick&dirty patch to reduce the copying of
single-shot plans, and was able to buy back much of the regression I was
seeing on DBT-2. Patch attached. But of course, DBT-2 really should be
preparing the queries once with SPI_prepare, and reusing them thereafter.

I was recently profiling an application that uses a fair amount of
PL/pgSQL with dynamic queries and also noticed AllocSetAlloc high in the
profile. I was getting suspicious now and compared 9.1 and 9.2
performance: 9.2 is consistently about 3% slower. Your patch doesn't
seem to have a measurable effect, but it might be if I ran the test for
longer.

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

Peter Eisentraut <peter_e@gmx.net> writes:

On 12/28/12 5:11 PM, Heikki Linnakangas wrote:

It looks like the regression is caused by extra copying of the parse
tree and plan trees. Node-copy-related functions like AllocSetAlloc and
_copy* are high in the profile, They are also high in the 9.1 profile,
but even more so in 9.2.

Hm ... those 9.2 changes were supposed to *improve* performance, and I
believe they did so for code paths where the plan cache is actually
doing something useful. In this path, it's basically useless.

I hacked together a quick&dirty patch to reduce the copying of
single-shot plans, and was able to buy back much of the regression I was
seeing on DBT-2. Patch attached. But of course, DBT-2 really should be
preparing the queries once with SPI_prepare, and reusing them thereafter.

I was recently profiling an application that uses a fair amount of
PL/pgSQL with dynamic queries and also noticed AllocSetAlloc high in the
profile. I was getting suspicious now and compared 9.1 and 9.2
performance: 9.2 is consistently about 3% slower. Your patch doesn't
seem to have a measurable effect, but it might be if I ran the test for
longer.

I'm inclined to think that Heikki's patch doesn't go far enough, if we
want to optimize behavior in this case. What we really want to happen
is that parsing, planning, and execution all happen in the caller's
memory context, with no copying of parse or plan trees at all - and we
could do without overhead such as dependency extraction and invalidation
checking, too. This would make SPI_execute a lot more comparable to the
behavior of exec_simple_query().

So basically plancache.c has got no useful functionality to offer here.

But to avoid having multiple drastically different code paths in spi.c,
it would be nice if we had some sort of "shell" API that would provide
the illusion of using a CachedPlan without any of the overhead that
comes along with actually being able to save or reuse the plan.
Heikki's "oneshot" concept is moving in that direction, but not far
enough IMO. I'm thinking we don't want it to create any new memory
contexts at all, just palloc a CachedPlan object directly in the
caller's memory context and link to the caller-supplied trees.

I'll take a whack at that ...

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

I wrote:

I'm inclined to think that Heikki's patch doesn't go far enough, if we
want to optimize behavior in this case. What we really want to happen
is that parsing, planning, and execution all happen in the caller's
memory context, with no copying of parse or plan trees at all - and we
could do without overhead such as dependency extraction and invalidation
checking, too. This would make SPI_execute a lot more comparable to the
behavior of exec_simple_query().

Here's a draft patch for that. My initial hack at it had a
disadvantage, which was that because no invalidation checking happened,
a SPI_execute query string containing a DDL command (such as ALTER TABLE)
followed by a command affected by the DDL would fail to reparse/replan
the second command properly. (I suspect that Heikki's version had a
related defect, but haven't looked closely.) Now that's not a huge deal
IMO, because in many common cases parse analysis of the second command
would fail anyway. For instance, this has never worked in any PG
release:

do $$ begin execute 'create table foo(f1 int); insert into foo values(1);'; end $$;

However it troubled me that there might be some regression there, and
after a bit of reflection I decided the right fix would be to rearrange
the code in spi.c so that parse analysis of later parsetrees follows
execution of earlier ones. This makes the behavior of SPI_execute()
even more like that of exec_simple_query(), and shouldn't cost anything
noticeable given the other changes here.

I'm not entirely sure about performance of this fix, though. I got
numbers varying between roughly-on-par with 9.1 and 10% slower than 9.1
for Pavel's example, depending on seemingly not-performance-related
rearrangements of the code in spi.c. I think this must be chance
effects of cache line alignment, but it would be good to hear what other
people get, both on Pavel's example and the other ones alluded to.
In any case this seems better than unmodified HEAD, which was 40% slower
than 9.1 for me.

regards, tom lane

On Friday, December 28, 2012, Heikki Linnakangas wrote:

On 28.12.2012 23:53, Peter Eisentraut wrote:

On 12/27/12 1:07 AM, Pavel Stehule wrote:

Hello

I rechecked performance of dynamic SQL and it is significantly slower
in 9.2 than 9.1

-- 9.1
postgres=# create or replace function test() returns void as $$ begin
for i in 1..1000000 loop execute 'select 1'; end loop; end $$ language
plpgsql;

I think this is the same as the case discussed at
<CAD4+=qWnGU0qi+iq=EPh6EGPuUnSCYsGDTgKazizEvrGgjo0Sg@mail.gmail.com>.

Yeah, probably so.

As it happens, I just spent a lot of time today narrowing down yet another
report of a regression in 9.2, when running DBT-2:
http://archives.postgresql.**org/pgsql-performance/2012-11/**msg00007.php&lt;http://archives.postgresql.org/pgsql-performance/2012-11/msg00007.php&gt;.
It looks like that is also caused by the plancache changes. DBT-2
implements the transactions using C functions, which use SPI_execute() to
run all the queries.

It looks like the regression is caused by extra copying of the parse tree
and plan trees. Node-copy-related functions like AllocSetAlloc and _copy*
are high in the profile, They are also high in the 9.1 profile, but even
more so in 9.2.

I hacked together a quick&dirty patch to reduce the copying of single-shot
plans, and was able to buy back much of the regression I was seeing on
DBT-2. Patch attached.

The plancache change slowed down a dynamic sql partitioning trigger about
26%, and your patch redeems about 1/2 of that cost.

Using a RULE-based partitioning instead with row by row insertion, the
plancache changes slowed it down by 300%, and this patch doesn't change
that. But that seems to be down to the insertion getting planned
repeatedly, because it decides the custom plan is cheaper than the generic
plan. Whatever savings the custom plan may have are clearly less than the
cost of doing the planning repeatedly.

Cheers,

Jeff

Jeff Janes <jeff.janes@gmail.com> writes:

Using a RULE-based partitioning instead with row by row insertion, the
plancache changes slowed it down by 300%, and this patch doesn't change
that. But that seems to be down to the insertion getting planned
repeatedly, because it decides the custom plan is cheaper than the generic
plan. Whatever savings the custom plan may have are clearly less than the
cost of doing the planning repeatedly.

That scenario doesn't sound like it has anything to do with the one being
discussed in this thread. But what do you mean by "rule-based
partitioning" exactly? A rule per se wouldn't result in a cached plan
at all, let alone one with parameters, which would be necessary to
trigger any use of the custom-cached-plan code path.

Test cases are way more interesting than hand-wavy complaints.

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

On 1/1/13 6:48 PM, Tom Lane wrote:

I wrote:

I'm inclined to think that Heikki's patch doesn't go far enough, if we
want to optimize behavior in this case. What we really want to happen
is that parsing, planning, and execution all happen in the caller's
memory context, with no copying of parse or plan trees at all - and we
could do without overhead such as dependency extraction and invalidation
checking, too. This would make SPI_execute a lot more comparable to the
behavior of exec_simple_query().

Here's a draft patch for that.

This didn't make a difference in my test case. I might have to do some
bisecting to find where the problem was introduced.

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

Peter Eisentraut <peter_e@gmx.net> writes:

On 1/1/13 6:48 PM, Tom Lane wrote:

Here's a draft patch for that.

This didn't make a difference in my test case. I might have to do some
bisecting to find where the problem was introduced.

Could we see the test case? Or at least oprofile results for it?

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

I did three back-to-back runs using the same settings as in
http://archives.postgresql.org/pgsql-performance/2012-11/msg00007.php
Except:
- use no prepared statement
- use 40 db connections
- build source from postgresql.git on the server box using: REL9_1_7,
REL9_2_2, REL9_2_2 + this patch

NOTPM results:
REL9_1_7: 46512.66
REL9_2_2: 42828.66
REL9_2_2 + this patch: 46973.70

Thanks,
Dong

PS, the top 20 lines of oprofile of these runs attached.

-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Sent: Tuesday, January 01, 2013 6:48 PM
To: Peter Eisentraut
Cc: Heikki Linnakangas; Pavel Stehule; PostgreSQL Hackers; Dong Ye
Subject: Re: [HACKERS] dynamic SQL - possible performance regression in
9.2

I wrote:

I'm inclined to think that Heikki's patch doesn't go far enough, if we
want to optimize behavior in this case. What we really want to happen
is that parsing, planning, and execution all happen in the caller's
memory context, with no copying of parse or plan trees at all - and we
could do without overhead such as dependency extraction and invalidation
checking, too. This would make SPI_execute a lot more comparable to the
behavior of exec_simple_query().

Here's a draft patch for that. My initial hack at it had a
disadvantage, which was that because no invalidation checking happened,
a SPI_execute query string containing a DDL command (such as ALTER TABLE)
followed by a command affected by the DDL would fail to reparse/replan
the second command properly. (I suspect that Heikki's version had a
related defect, but haven't looked closely.) Now that's not a huge deal
IMO, because in many common cases parse analysis of the second command
would fail anyway. For instance, this has never worked in any PG
release:

do $$ begin execute 'create table foo(f1 int); insert into foo
values(1);'; end $$;

However it troubled me that there might be some regression there, and
after a bit of reflection I decided the right fix would be to rearrange
the code in spi.c so that parse analysis of later parsetrees follows
execution of earlier ones. This makes the behavior of SPI_execute()
even more like that of exec_simple_query(), and shouldn't cost anything
noticeable given the other changes here.

I'm not entirely sure about performance of this fix, though. I got
numbers varying between roughly-on-par with 9.1 and 10% slower than 9.1
for Pavel's example, depending on seemingly not-performance-related
rearrangements of the code in spi.c. I think this must be chance
effects of cache line alignment, but it would be good to hear what other
people get, both on Pavel's example and the other ones alluded to.
In any case this seems better than unmodified HEAD, which was 40% slower
than 9.1 for me.

regards, tom lane

"Dong Ye" <yed@vmware.com> writes:

I did three back-to-back runs using the same settings as in
http://archives.postgresql.org/pgsql-performance/2012-11/msg00007.php
Except:
- use no prepared statement
- use 40 db connections
- build source from postgresql.git on the server box using: REL9_1_7,
REL9_2_2, REL9_2_2 + this patch

NOTPM results:
REL9_1_7: 46512.66
REL9_2_2: 42828.66
REL9_2_2 + this patch: 46973.70

Thanks! I think this is probably sufficient evidence to conclude that
we should apply this patch, at least in HEAD. Whatever Peter is seeing
must be some other issue, which we can address whenever we understand
what it is.

Next question is what people think about back-patching into 9.2 so as
to eliminate the performance regression vs 9.1. I believe this would
be safe (although some care would have to be taken to put the added
boolean fields into places where they'd not result in an ABI break).
However it may not be worth the risk. The 40% slowdown seen with
Pavel's example seems to me to be an extreme corner case --- Dong's
result of 8% slowdown is probably more realistic for normal uses
of SPI_execute. Might be better to just live with it in 9.2.
Thoughts?

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

2013/1/4 Tom Lane <tgl@sss.pgh.pa.us>:

"Dong Ye" <yed@vmware.com> writes:

I did three back-to-back runs using the same settings as in
http://archives.postgresql.org/pgsql-performance/2012-11/msg00007.php
Except:
- use no prepared statement
- use 40 db connections
- build source from postgresql.git on the server box using: REL9_1_7,
REL9_2_2, REL9_2_2 + this patch

NOTPM results:
REL9_1_7: 46512.66
REL9_2_2: 42828.66
REL9_2_2 + this patch: 46973.70

Thanks! I think this is probably sufficient evidence to conclude that
we should apply this patch, at least in HEAD. Whatever Peter is seeing
must be some other issue, which we can address whenever we understand
what it is.

Next question is what people think about back-patching into 9.2 so as
to eliminate the performance regression vs 9.1. I believe this would
be safe (although some care would have to be taken to put the added
boolean fields into places where they'd not result in an ABI break).
However it may not be worth the risk. The 40% slowdown seen with
Pavel's example seems to me to be an extreme corner case --- Dong's
result of 8% slowdown is probably more realistic for normal uses
of SPI_execute. Might be better to just live with it in 9.2.
Thoughts?

I am for back-patching - I agree with you so my example is corner
case, and cannot be worse example - but performance regression about
5-10% can be confusing for users - because they can searching
regression in their application.

Regards

Pavel Stehule

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

Next question is what people think about back-patching into 9.2 so as
to eliminate the performance regression vs 9.1. I believe this would
be safe (although some care would have to be taken to put the added
boolean fields into places where they'd not result in an ABI break).
However it may not be worth the risk. The 40% slowdown seen with
Pavel's example seems to me to be an extreme corner case --- Dong's
result of 8% slowdown is probably more realistic for normal uses
of SPI_execute. Might be better to just live with it in 9.2.
Thoughts?

8% is a pretty serious regression, for those of us with applications
which do a lot of dynamic SQL. As a reminder, many people do dynamic
SQL even in repetitive, performance-sensitive functions in order to
avoid plan caching. Also partition-handlers often use dynamic SQL, and
a 10% drop in loading rows/second would be a big deal.

Let's put it this way: if the community doesn't backport it, we'll end
up doing so ad-hoc for some of our customers.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

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

On 04.01.2013 22:05, Josh Berkus wrote:

Next question is what people think about back-patching into 9.2 so as
to eliminate the performance regression vs 9.1. I believe this would
be safe (although some care would have to be taken to put the added
boolean fields into places where they'd not result in an ABI break).
However it may not be worth the risk. The 40% slowdown seen with
Pavel's example seems to me to be an extreme corner case --- Dong's
result of 8% slowdown is probably more realistic for normal uses
of SPI_execute. Might be better to just live with it in 9.2.
Thoughts?

8% is a pretty serious regression, for those of us with applications
which do a lot of dynamic SQL. As a reminder, many people do dynamic
SQL even in repetitive, performance-sensitive functions in order to
avoid plan caching. Also partition-handlers often use dynamic SQL, and
a 10% drop in loading rows/second would be a big deal.

+1 for backpatching.

- Heikki

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

On 01/04/2013 12:05 PM, Josh Berkus wrote:

Next question is what people think about back-patching into 9.2 so as
to eliminate the performance regression vs 9.1. I believe this would
be safe (although some care would have to be taken to put the added
boolean fields into places where they'd not result in an ABI break).
However it may not be worth the risk. The 40% slowdown seen with
Pavel's example seems to me to be an extreme corner case --- Dong's
result of 8% slowdown is probably more realistic for normal uses
of SPI_execute. Might be better to just live with it in 9.2.
Thoughts?

8% is a pretty serious regression, for those of us with applications
which do a lot of dynamic SQL. As a reminder, many people do dynamic
SQL even in repetitive, performance-sensitive functions in order to
avoid plan caching. Also partition-handlers often use dynamic SQL, and
a 10% drop in loading rows/second would be a big deal.

Let's put it this way: if the community doesn't backport it, we'll end
up doing so ad-hoc for some of our customers.

Exactly. This is a significant reduction in the production quality of
PostgreSQL as it pertains to dynamic SQL. To put it more bluntly, we
will have people not upgrade to 9.2 specifically because of this problem.

Joshua D. Drake

--
Command Prompt, Inc. - http://www.commandprompt.com/
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, Postgres-XC
@cmdpromptinc - 509-416-6579

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

Josh Berkus <josh@agliodbs.com> writes:

Next question is what people think about back-patching into 9.2 so as
to eliminate the performance regression vs 9.1.

8% is a pretty serious regression, for those of us with applications
which do a lot of dynamic SQL. As a reminder, many people do dynamic
SQL even in repetitive, performance-sensitive functions in order to
avoid plan caching.

Well, of course, people with that type of problem should probably
rethink their use of dynamic SQL when they move to 9.2 anyway, because
that's the case where the new plancache code could actually help them
if they'd only let it.

But anyway, nobody seems to be speaking against back-patching, so
I'll go do it.

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

On 01/04/2013 01:17 PM, Tom Lane wrote:

Well, of course, people with that type of problem should probably
rethink their use of dynamic SQL when they move to 9.2 anyway, because
that's the case where the new plancache code could actually help them
if they'd only let it.

Not to further the argument because of the +1 from you but I think it is
important to keep in mind that the less work we make it to upgrade, the
more likely it is they will.

It took forever (and we still have stragglers) to get people past 8.2
because of the casting change in 8.3. This is a similar problem in that
if we want them to upgrade to take advantage of features, we have to
make it so the least amount of work possible is needed to make that
upgrade happen.

Rewriting many thousands of lines of dynamic sql to upgrade to 9.2 is
certainly not doing that :).

Sincerely,

Joshua D. Drake

But anyway, nobody seems to be speaking against back-patching, so
I'll go do it.

regards, tom lane

--
Command Prompt, Inc. - http://www.commandprompt.com/
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, Postgres-XC
@cmdpromptinc - 509-416-6579

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

Well, of course, people with that type of problem should probably
rethink their use of dynamic SQL when they move to 9.2 anyway, because
that's the case where the new plancache code could actually help them
if they'd only let it.

Oh, no question. But it'll take time for users with 1000's of lines of
PLpgSQL from 8.2 to rewrite it.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

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