Re: dynamic SQL - possible performance regression in 9.2

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;
CREATE FUNCTION
postgres=# \timing
Timing is on.
postgres=# select test();
test
------

(1 row)

Time: 7652.904 ms
postgres=# select test();
test
------

(1 row)

Time: 7828.025 ms

-- 9.2
postgres=# create or replace function test() returns void as $$ begin
for i in 1..1000000 loop execute 'select 1'; end loop; end $$ language
plpgsql;
CREATE FUNCTION
Time: 59.272 ms
postgres=# select test();
test
------

(1 row)

Time: 11153.646 ms
postgres=# select test();
test
------

(1 row)

Time: 11081.899 ms

This test is synthetic, but it shows so somebody who use dynamic SQL
in triggers (for partitioning) can has slower operations.

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(at)mail(dot)gmail(dot)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(at)mail(dot)gmail(dot)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(at)vmware(dot)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(at)mail(dot)gmail(dot)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

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.

Peter Eisentraut <peter_e(at)gmx(dot)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

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(at)mail(dot)gmail(dot)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<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.

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(dot)janes(at)gmail(dot)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

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.

Peter Eisentraut <peter_e(at)gmx(dot)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

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(at)sss(dot)pgh(dot)pa(dot)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(at)vmware(dot)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

2013/1/4 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> "Dong Ye" <yed(at)vmware(dot)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

> 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

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

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

Josh Berkus <josh(at)agliodbs(dot)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

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

> 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

On Wednesday, January 2, 2013, Tom Lane wrote:

> Jeff Janes <jeff(dot)janes(at)gmail(dot)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.
>

Right, it is not related to the dynamic SQL, but is to the plan-cache.

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

Sorry, when exiled to the hinterlands I have more time to test various
things but not a good enough connectivity to describe them well. I'm
attaching the test case to load 1e5 rows into a very skinny table with 100
partitions using rules.

"origin" is from a few days ago, "origin_reduce_copies" is Heikki's patch,
and "origin_one_shot" is your now-committed patch. (unshown are
e6faf910d75027 and e6faf910d75027_prev, but that is where the regression
was introduced)

JJ /usr/local/pgsql_REL9_1_7/
Time: 64252.6907920837 ms
JJ origin/
Time: 186657.824039459 ms
JJ origin_reduce_copies/
Time: 185370.236873627 ms
JJ origin_one_shot/
Time: 189104.484081268 ms

The root problem is that it thinks the generic plan costs about 50% more
than the custom one. I don't know why it thinks that, or how much it is
worth chasing it down.

On the other hand, your patch does fix almost all of the 9.2.[012]
regression of using the following dynamic SQL trigger (instead of RULES) to
load into the same test case.

CREATE OR REPLACE FUNCTION foo_insert_trigger()
RETURNS trigger AS $$
DECLARE tablename varchar(24);
BEGIN
tablename = 'foo_' || new.partition;
EXECUTE 'INSERT INTO '|| tablename ||' VALUES (($1).*)' USING NEW ;
RETURN NULL;
END;
$$
LANGUAGE plpgsql;

CREATE TRIGGER foo_insert_trigger
BEFORE INSERT ON foo
FOR EACH ROW EXECUTE PROCEDURE foo_insert_trigger();

Cheers,

Jeff

Jeff Janes <jeff(dot)janes(at)gmail(dot)com> writes:
> On Wednesday, January 2, 2013, Tom Lane wrote:
>> 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.

> Sorry, when exiled to the hinterlands I have more time to test various
> things but not a good enough connectivity to describe them well. I'm
> attaching the test case to load 1e5 rows into a very skinny table with 100
> partitions using rules.

Ah. I see what's going on: the generic plan has got 100 separate
subplans that look like

Insert on foo_10 (cost=0.00..0.01 rows=1 width=0)
-> Result (cost=0.00..0.01 rows=1 width=0)
One-Time Filter: ($2 = 10)

Insert on foo_11 (cost=0.00..0.01 rows=1 width=0)
-> Result (cost=0.00..0.01 rows=1 width=0)
One-Time Filter: ($2 = 11)

Insert on foo_12 (cost=0.00..0.01 rows=1 width=0)
-> Result (cost=0.00..0.01 rows=1 width=0)
One-Time Filter: ($2 = 12)

Insert on foo_13 (cost=0.00..0.01 rows=1 width=0)
-> Result (cost=0.00..0.01 rows=1 width=0)
One-Time Filter: ($2 = 13)

while a custom plan simplifies that to something like

Insert on foo_10 (cost=0.00..0.01 rows=1 width=0)
-> Result (cost=0.00..0.01 rows=1 width=0)
One-Time Filter: false

Insert on foo_11 (cost=0.00..0.01 rows=1 width=0)
-> Result (cost=0.00..0.01 rows=1 width=0)

Insert on foo_12 (cost=0.00..0.01 rows=1 width=0)
-> Result (cost=0.00..0.01 rows=1 width=0)
One-Time Filter: false

Insert on foo_13 (cost=0.00..0.01 rows=1 width=0)
-> Result (cost=0.00..0.01 rows=1 width=0)
One-Time Filter: false

(here foo_11 is the actual target). This is indeed a bit faster than
the generic plan, and would be more so if we tried harder to get rid of
no-op subplans entirely ... but it's not enough faster to justify the
extra planning time.

In the particular case at hand, we're getting estimated costs of about
1.01 for a custom plan versus 1.51 for the generic plan. So I think
that whether the 50% ratio is accurate is a bit beside the point --- the
real point is that we're only saving half a cost unit of estimated cost.
So that suggests that choose_custom_plan should impose not only a
minimum fractional savings but also a minimum absolute savings to
justify selecting the custom plan approach. I tried this and verified
that it fixed the problem:

diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/planc
index cbc7c49..1973ed3 100644
*** a/src/backend/utils/cache/plancache.c
--- b/src/backend/utils/cache/plancache.c
*************** choose_custom_plan(CachedPlanSource *pla
*** 990,995 ****
--- 990,997 ----
*/
if (plansource->generic_cost < avg_custom_cost * 1.1)
return false;
+ if (plansource->generic_cost < avg_custom_cost + 10)
+ return false;

return true;
}

but of course it's hard to say what that cutoff number ought to be.

Back during the development of the plancache patch we speculated about
how we might take cost-of-planning into account in deciding whether to
use a custom plan or not. The root problem is that we've got no data
about how much work the planner does for a given query and how that
compares to estimated-cost numbers. There was some argument that we
could just take gettimeofday() measurements and drive it off that, but
I don't care for that too much; for one thing it would make the behavior
unstable with variations in system load.

Anyway the bottom line is that we never went back to do the research
about what the policy followed by choose_custom_plan ought to be.
It's probably time to think about that, or at least find a better
stopgap solution than what's in there.

regards, tom lane