Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Peter Eisentraut]

On 12.07.18 11:12, Pavel Stehule wrote:
> This appears to be the patch of record in this thread.  I think there is
> general desire for adding a setting like this, and the implementation is
> simple enough.
> 
> One change perhaps: How about naming the default setting "auto" instead
> of "default".  That makes it clearer what it does.
> 
> 
> I changed "default" to "auto"
> 
> updated patch attached

committed with some editing

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Tatsuro Yamada]

On 2018/07/12 18:12, Pavel Stehule wrote:

Hi

2018-07-10 12:01 GMT+02:00 Peter Eisentraut mailto:peter.eisentr...@2ndquadrant.com>>:

On 23.01.18 17:08, Pavel Stehule wrote:
 > attached updated patch

This appears to be the patch of record in this thread.  I think there is
general desire for adding a setting like this, and the implementation is
simple enough.

One change perhaps: How about naming the default setting "auto" instead
of "default".  That makes it clearer what it does.


I changed "default" to "auto"

updated patch attached

Regards

Pavel


Hi Pavel,

I tested your patch on 40ca70eb.
Here is the result.

===
 All 190 tests passed.
===

Thanks,
Tatsuro Yamada

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Pavel Stehule]

Hi

2018-07-10 12:01 GMT+02:00 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com>:

> On 23.01.18 17:08, Pavel Stehule wrote:
> > attached updated patch
>
> This appears to be the patch of record in this thread.  I think there is
> general desire for adding a setting like this, and the implementation is
> simple enough.
>
> One change perhaps: How about naming the default setting "auto" instead
> of "default".  That makes it clearer what it does.
>

I changed "default" to "auto"

updated patch attached

Regards

Pavel


>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
commit 456e1f284cdc7b5a024004e69deb53effccf7427
Author: Pavel Stehule 
Date:   Thu Jul 12 11:09:54 2018 +0200

initial

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e307bb4e8e..dc254b067c 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4616,6 +4616,31 @@ SELECT * FROM parent WHERE key = 2400;
   
  
 
+ 
+
+
+ 
+ Plan Cache Options
+
+ 
+
+ 
+  plancache_mode (enum)
+  
+   plancache_mode configuration parameter
+  
+  
+  
+   
+Prepared queries have custom and generic plans and the planner 
+will attempt to choose which is better; this can be set to override 
+the default behavior. The allowed values are auto,
+force_custom_plan and force_generic_plan.
+The default value is auto.
+   
+  
+ 
+
  
 

diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 0ad3e3c736..67cdfb63b1 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -106,6 +106,8 @@ static void PlanCacheRelCallback(Datum arg, Oid relid);
 static void PlanCacheFuncCallback(Datum arg, int cacheid, uint32 hashvalue);
 static void PlanCacheSysCallback(Datum arg, int cacheid, uint32 hashvalue);
 
+/* GUC parameter */
+int	plancache_mode;
 
 /*
  * InitPlanCache: initialize module during InitPostgres.
@@ -1033,6 +1035,12 @@ choose_custom_plan(CachedPlanSource *plansource, ParamListInfo boundParams)
 	if (IsTransactionStmtPlan(plansource))
 		return false;
 
+	/* See if settings wants to force the decision */
+	if (plancache_mode == PLANCACHE_FORCE_GENERIC_PLAN)
+		return false;
+	if (plancache_mode == PLANCACHE_FORCE_CUSTOM_PLAN)
+		return true;
+
 	/* See if caller wants to force the decision */
 	if (plansource->cursor_options & CURSOR_OPT_GENERIC_PLAN)
 		return false;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 17292e04fe..7b1a2921b4 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -405,6 +405,13 @@ static const struct config_enum_entry force_parallel_mode_options[] = {
 	{NULL, 0, false}
 };
 
+static const struct config_enum_entry plancache_mode_options[] = {
+	{"auto", PLANCACHE_AUTO, false},
+	{"force_generic_plan", PLANCACHE_FORCE_GENERIC_PLAN, false},
+	{"force_custom_plan", PLANCACHE_FORCE_CUSTOM_PLAN, false},
+	{NULL, 0, false}
+};
+
 /*
  * password_encryption used to be a boolean, so accept all the likely
  * variants of "on", too. "off" used to store passwords in plaintext,
@@ -4150,6 +4157,18 @@ static struct config_enum ConfigureNamesEnum[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"plancache_mode", PGC_USERSET, QUERY_TUNING_OTHER,
+			gettext_noop("Controls the planner's selection of custom or generic plan."),
+			gettext_noop("Prepared queries have custom and generic plans and the planner "
+		 "will attempt to choose which is better; this can be set to override "
+		 "the default behavior.")
+		},
+		&plancache_mode,
+		PLANCACHE_AUTO, plancache_mode_options,
+		NULL, NULL, NULL
+	},
+
 	/* End-of-list marker */
 	{
 		{NULL, 0, 0, NULL, NULL}, NULL, 0, NULL, NULL, NULL, NULL
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index ab20aa04b0..cf8a0dd020 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -182,4 +182,16 @@ extern CachedPlan *GetCachedPlan(CachedPlanSource *plansource,
 			  QueryEnvironment *queryEnv);
 extern void ReleaseCachedPlan(CachedPlan *plan, bool useResOwner);
 
+/* possible values for plancache_mode */
+typedef enum
+{
+	PLANCACHE_AUTO,
+	PLANCACHE_FORCE_GENERIC_PLAN,
+	PLANCACHE_FORCE_CUSTOM_PLAN
+}			PlanCacheMode;
+
+
+/* GUC parameter */
+extern int plancache_mode;
+
 #endif			/* PLANCACHE_H */
diff --git a/src/test/regress/expected/plancache.out b/src/test/regress/expected/plancache.out
index 3086c68566..d8255915d6 100644
--- a/src/test/regress/expected/plancache.out
+++ b/src/test/regress/expected/plancache.out
@@ -278,3 +278,82 @@ drop table pc_list_part_1;
 execute pstmt_def_insert(1);
 drop table pc_list_parted, pc_list_part_null;
 deallocate pstmt_def_insert;
+--
+-- Test plan cache strategy
+--
+create table test_strategy(a int);
+insert into test_strategy s

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Peter Eisentraut]

On 23.01.18 17:08, Pavel Stehule wrote:
> attached updated patch

This appears to be the patch of record in this thread.  I think there is
general desire for adding a setting like this, and the implementation is
simple enough.

One change perhaps: How about naming the default setting "auto" instead
of "default".  That makes it clearer what it does.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Pavel Stehule]

2018-03-02 3:43 GMT+01:00 Pavel Stehule :

>
>
> 2018-03-02 3:38 GMT+01:00 Andres Freund :
>
>> On 2018-03-02 03:13:04 +0100, Pavel Stehule wrote:
>> > 2018-03-01 23:10 GMT+01:00 Andres Freund :
>> >
>> > > On 2018-01-23 17:08:56 +0100, Pavel Stehule wrote:
>> > > > 2018-01-22 23:15 GMT+01:00 Stephen Frost :
>> > > > > This really could use a new thread, imv.  This thread is a year
>> old and
>> > > > > about a completely different feature than what you've implemented
>> here.
>> > > > >
>> > > >
>> > > > true, but now it is too late
>> > >
>> > > At the very least the CF entry could be renamed moved out the
>> procedual
>> > > language category?
>> > >
>> >
>> > Why not?
>>
>> Because the patch adds GUCs that don't have a direct connection
>> toprocedual languages?  And the patch's topic still says "plpgsql plan
>> cache behave" which surely is misleading.
>>
>> Seems fairly obvious that neither category nor name is particularly
>> descriptive of the current state?
>>
>>
> ok
>
>
>> > Have you idea, what category is best?
>>
>> Server Features? Misc?  And as a title something about "add GUCs to
>> control custom plan logic"?
>>
>>
> I'll move it there.
>

done

Pavel


>
> Regards
>
> Pavel
>
>>
>> Greetings,
>>
>> Andres Freund
>>
>
>

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Pavel Stehule]

2018-03-02 3:38 GMT+01:00 Andres Freund :

> On 2018-03-02 03:13:04 +0100, Pavel Stehule wrote:
> > 2018-03-01 23:10 GMT+01:00 Andres Freund :
> >
> > > On 2018-01-23 17:08:56 +0100, Pavel Stehule wrote:
> > > > 2018-01-22 23:15 GMT+01:00 Stephen Frost :
> > > > > This really could use a new thread, imv.  This thread is a year
> old and
> > > > > about a completely different feature than what you've implemented
> here.
> > > > >
> > > >
> > > > true, but now it is too late
> > >
> > > At the very least the CF entry could be renamed moved out the procedual
> > > language category?
> > >
> >
> > Why not?
>
> Because the patch adds GUCs that don't have a direct connection
> toprocedual languages?  And the patch's topic still says "plpgsql plan
> cache behave" which surely is misleading.
>
> Seems fairly obvious that neither category nor name is particularly
> descriptive of the current state?
>
>
ok


> > Have you idea, what category is best?
>
> Server Features? Misc?  And as a title something about "add GUCs to
> control custom plan logic"?
>
>
I'll move it there.

Regards

Pavel

>
> Greetings,
>
> Andres Freund
>

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Andres Freund]

On 2018-03-02 03:13:04 +0100, Pavel Stehule wrote:
> 2018-03-01 23:10 GMT+01:00 Andres Freund :
> 
> > On 2018-01-23 17:08:56 +0100, Pavel Stehule wrote:
> > > 2018-01-22 23:15 GMT+01:00 Stephen Frost :
> > > > This really could use a new thread, imv.  This thread is a year old and
> > > > about a completely different feature than what you've implemented here.
> > > >
> > >
> > > true, but now it is too late
> >
> > At the very least the CF entry could be renamed moved out the procedual
> > language category?
> >
> 
> Why not?

Because the patch adds GUCs that don't have a direct connection
toprocedual languages?  And the patch's topic still says "plpgsql plan
cache behave" which surely is misleading.

Seems fairly obvious that neither category nor name is particularly
descriptive of the current state?


> Have you idea, what category is best?

Server Features? Misc?  And as a title something about "add GUCs to
control custom plan logic"?


Greetings,

Andres Freund

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Pavel Stehule]

2018-03-01 23:10 GMT+01:00 Andres Freund :

> On 2018-01-23 17:08:56 +0100, Pavel Stehule wrote:
> > 2018-01-22 23:15 GMT+01:00 Stephen Frost :
> > > This really could use a new thread, imv.  This thread is a year old and
> > > about a completely different feature than what you've implemented here.
> > >
> >
> > true, but now it is too late
>
> At the very least the CF entry could be renamed moved out the procedual
> language category?
>

Why not? Have you idea, what category is best?

Regards

Pavel

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Andres Freund]

On 2018-01-23 17:08:56 +0100, Pavel Stehule wrote:
> 2018-01-22 23:15 GMT+01:00 Stephen Frost :
> > This really could use a new thread, imv.  This thread is a year old and
> > about a completely different feature than what you've implemented here.
> >
> 
> true, but now it is too late

At the very least the CF entry could be renamed moved out the procedual
language category?

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Pavel Stehule]

2018-01-30 9:06 GMT+01:00 Tatsuro Yamada :

> On 2018/01/24 1:08, Pavel Stehule wrote:
>
>>
>>
>> 2018-01-22 23:15 GMT+01:00 Stephen Frost > sfr...@snowman.net>>:
>>
>> Pavel,
>>
>>
>> * Pavel Stehule (pavel.steh...@gmail.com > pavel.steh...@gmail.com>) wrote:
>> > here is a GUC based patch for plancache controlling. Looks so this
>> code is
>> > working.
>>
>> This really could use a new thread, imv.  This thread is a year old
>> and
>> about a completely different feature than what you've implemented
>> here.
>>
>>
>> true, but now it is too late
>>
>>
>> > It is hard to create regress tests. Any ideas?
>>
>> Honestly, my idea for this would be to add another option to EXPLAIN
>> which would make it spit out generic and custom plans, or something of
>> that sort.
>>
>>
>> I looked there, but it needs cycle dependency between CachedPlan and
>> PlannedStmt. It needs more code than this patch and code will not be nicer.
>> I try to do some game with prepared statements
>>
>> Please review your patches before sending them and don't send in
>> patches
>> which have random unrelated whitespace changes.
>>
>>  > diff --git a/src/backend/utils/cache/plancache.c
>> b/src/backend/utils/cache/plancache.c
>>  > index ad8a82f1e3..cc99cf6dcc 100644
>>  > --- a/src/backend/utils/cache/plancache.c
>>  > +++ b/src/backend/utils/cache/plancache.c
>>  > @@ -1031,6 +1033,12 @@ choose_custom_plan(CachedPlanSource
>> *plansource, ParamListInfo boundParams)
>>  >   if (IsTransactionStmtPlan(plansource))
>>  >   return false;
>>  >
>>  > + /* See if settings wants to force the decision */
>>  > + if (plancache_mode & PLANCACHE_FORCE_GENERIC_PLAN)
>>  > + return false;
>>  > + if (plancache_mode & PLANCACHE_FORCE_CUSTOM_PLAN)
>>  > + return true;
>>
>> Not a big deal, but I probably would have just expanded the
>> conditional
>> checks against cursor_options (just below) rather than adding
>> independent if() statements.
>>
>>
>> I don't think so proposed change is better - My opinion is not strong -
>> and commiter can change it simply
>>
>>
>>  > diff --git a/src/backend/utils/misc/guc.c
>> b/src/backend/utils/misc/guc.c
>>  > index ae22185fbd..4ce275e39d 100644
>>  > --- a/src/backend/utils/misc/guc.c
>>  > +++ b/src/backend/utils/misc/guc.c
>>  > @@ -3916,6 +3923,16 @@ static struct config_enum
>> ConfigureNamesEnum[] =
>>  >   NULL, NULL, NULL
>>  >   },
>>  >
>>  > + {
>>  > + {"plancache_mode", PGC_USERSET, QUERY_TUNING_OTHER,
>>  > + gettext_noop("Forces use of custom or
>> generic plans."),
>>  > + gettext_noop("It can control query plan
>> cache.")
>>  > + },
>>  > + &plancache_mode,
>>  > + PLANCACHE_DEFAULT, plancache_mode_options,
>>  > + NULL, NULL, NULL
>>  > + },
>>
>> That long description is shorter than the short description.  How
>> about:
>>
>> short: Controls the planner's selection of custom or generic plan.
>> long: Prepared queries have custom and generic plans and the planner
>> will attempt to choose which is better; this can be set to override
>> the default behavior.
>>
>>
>> changed - thank you for text
>>
>>
>> (either that, or just ditch the long description)
>>
>>  > diff --git a/src/include/utils/plancache.h
>> b/src/include/utils/plancache.h
>>  > index 87fab19f3c..962895cc1a 100644
>>  > --- a/src/include/utils/plancache.h
>>  > +++ b/src/include/utils/plancache.h
>>  > @@ -143,7 +143,6 @@ typedef struct CachedPlan
>>  >   MemoryContext context;  /* context containing this
>> CachedPlan */
>>  >  } CachedPlan;
>>  >
>>  > -
>>  >  extern void InitPlanCache(void);
>>  >  extern void ResetPlanCache(void);
>>  >
>>
>> Random unrelated whitespace change...
>>
>>
>> fixed
>>
>>
>> This is also missing documentation updates.
>>
>>
>> I wrote some basic doc, but native speaker should to write more words
>> about used strategies.
>>
>>
>> Setting to Waiting for Author, but with those changes I would think we
>> could mark it ready-for-committer, it seems pretty straight-forward to
>> me and there seems to be general agreement (now) that it's worthwhile
>> to
>> have.
>>
>> Thanks!
>>
>>
>> attached updated patch
>>
>> Regards
>>
>> Pavel
>>
>>
>> Stephen
>>
>
> Hi Pavel,
>
> I tested your patch on a044378ce2f6268a996c8cce2b7bfb5d82b05c90.
> I share my results to you.
>
>  - Result of patch command
>
> $ patch -p1 < guc-plancache_mode-180123.patch
> patching file doc/src/sgml/config.sgml
> Hunk #1 succeeded at 4400 (offset 13 lines).
> patching file src/backend/utils/cache/plancache.c
> patching file src/backend/utils/m

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Tatsuro Yamada]

On 2018/01/24 1:08, Pavel Stehule wrote:



2018-01-22 23:15 GMT+01:00 Stephen Frost mailto:sfr...@snowman.net>>:

Pavel,

* Pavel Stehule (pavel.steh...@gmail.com ) 
wrote:
> here is a GUC based patch for plancache controlling. Looks so this code is
> working.

This really could use a new thread, imv.  This thread is a year old and
about a completely different feature than what you've implemented here.


true, but now it is too late


> It is hard to create regress tests. Any ideas?

Honestly, my idea for this would be to add another option to EXPLAIN
which would make it spit out generic and custom plans, or something of
that sort.


I looked there, but it needs cycle dependency between CachedPlan and 
PlannedStmt. It needs more code than this patch and code will not be nicer. I 
try to do some game with prepared statements

Please review your patches before sending them and don't send in patches
which have random unrelated whitespace changes.

 > diff --git a/src/backend/utils/cache/plancache.c 
b/src/backend/utils/cache/plancache.c
 > index ad8a82f1e3..cc99cf6dcc 100644
 > --- a/src/backend/utils/cache/plancache.c
 > +++ b/src/backend/utils/cache/plancache.c
 > @@ -1031,6 +1033,12 @@ choose_custom_plan(CachedPlanSource *plansource, 
ParamListInfo boundParams)
 >       if (IsTransactionStmtPlan(plansource))
 >               return false;
 >
 > +     /* See if settings wants to force the decision */
 > +     if (plancache_mode & PLANCACHE_FORCE_GENERIC_PLAN)
 > +             return false;
 > +     if (plancache_mode & PLANCACHE_FORCE_CUSTOM_PLAN)
 > +             return true;

Not a big deal, but I probably would have just expanded the conditional
checks against cursor_options (just below) rather than adding
independent if() statements.


I don't think so proposed change is better - My opinion is not strong - and 
commiter can change it simply


 > diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
 > index ae22185fbd..4ce275e39d 100644
 > --- a/src/backend/utils/misc/guc.c
 > +++ b/src/backend/utils/misc/guc.c
 > @@ -3916,6 +3923,16 @@ static struct config_enum ConfigureNamesEnum[] =
 >               NULL, NULL, NULL
 >       },
 >
 > +     {
 > +             {"plancache_mode", PGC_USERSET, QUERY_TUNING_OTHER,
 > +                     gettext_noop("Forces use of custom or generic 
plans."),
 > +                     gettext_noop("It can control query plan cache.")
 > +             },
 > +             &plancache_mode,
 > +             PLANCACHE_DEFAULT, plancache_mode_options,
 > +             NULL, NULL, NULL
 > +     },

That long description is shorter than the short description.  How about:

short: Controls the planner's selection of custom or generic plan.
long: Prepared queries have custom and generic plans and the planner
will attempt to choose which is better; this can be set to override
the default behavior.


changed - thank you for text


(either that, or just ditch the long description)

 > diff --git a/src/include/utils/plancache.h 
b/src/include/utils/plancache.h
 > index 87fab19f3c..962895cc1a 100644
 > --- a/src/include/utils/plancache.h
 > +++ b/src/include/utils/plancache.h
 > @@ -143,7 +143,6 @@ typedef struct CachedPlan
 >       MemoryContext context;          /* context containing this 
CachedPlan */
 >  } CachedPlan;
 >
 > -
 >  extern void InitPlanCache(void);
 >  extern void ResetPlanCache(void);
 >

Random unrelated whitespace change...


fixed


This is also missing documentation updates.


I wrote some basic doc, but native speaker should to write more words about 
used strategies.


Setting to Waiting for Author, but with those changes I would think we
could mark it ready-for-committer, it seems pretty straight-forward to
me and there seems to be general agreement (now) that it's worthwhile to
have.

Thanks!


attached updated patch

Regards

Pavel


Stephen


Hi Pavel,

I tested your patch on a044378ce2f6268a996c8cce2b7bfb5d82b05c90.
I share my results to you.

 - Result of patch command

$ patch -p1 < guc-plancache_mode-180123.patch
patching file doc/src/sgml/config.sgml
Hunk #1 succeeded at 4400 (offset 13 lines).
patching file src/backend/utils/cache/plancache.c
patching file src/backend/utils/misc/guc.c
patching file src/include/utils/plancache.h
patching file src/test/regress/expected/plancache.out
patching file src/test/regress/sql/plancache.sql

 - Result of regression test

===
 All 186 tests passed.
===

Regards,
Tatsuro Yamada

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Pavel Stehule]

2018-01-22 23:15 GMT+01:00 Stephen Frost :

> Pavel,
>
> * Pavel Stehule (pavel.steh...@gmail.com) wrote:
> > here is a GUC based patch for plancache controlling. Looks so this code
> is
> > working.
>
> This really could use a new thread, imv.  This thread is a year old and
> about a completely different feature than what you've implemented here.
>

true, but now it is too late


> > It is hard to create regress tests. Any ideas?
>
> Honestly, my idea for this would be to add another option to EXPLAIN
> which would make it spit out generic and custom plans, or something of
> that sort.
>
>
I looked there, but it needs cycle dependency between CachedPlan and
PlannedStmt. It needs more code than this patch and code will not be nicer.
I try to do some game with prepared statements



> Please review your patches before sending them and don't send in patches
> which have random unrelated whitespace changes.
>
> > diff --git a/src/backend/utils/cache/plancache.c
> b/src/backend/utils/cache/plancache.c
> > index ad8a82f1e3..cc99cf6dcc 100644
> > --- a/src/backend/utils/cache/plancache.c
> > +++ b/src/backend/utils/cache/plancache.c
> > @@ -1031,6 +1033,12 @@ choose_custom_plan(CachedPlanSource *plansource,
> ParamListInfo boundParams)
> >   if (IsTransactionStmtPlan(plansource))
> >   return false;
> >
> > + /* See if settings wants to force the decision */
> > + if (plancache_mode & PLANCACHE_FORCE_GENERIC_PLAN)
> > + return false;
> > + if (plancache_mode & PLANCACHE_FORCE_CUSTOM_PLAN)
> > + return true;
>
> Not a big deal, but I probably would have just expanded the conditional
> checks against cursor_options (just below) rather than adding
> independent if() statements.
>

I don't think so proposed change is better - My opinion is not strong - and
commiter can change it simply

>
> > diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
> > index ae22185fbd..4ce275e39d 100644
> > --- a/src/backend/utils/misc/guc.c
> > +++ b/src/backend/utils/misc/guc.c
> > @@ -3916,6 +3923,16 @@ static struct config_enum ConfigureNamesEnum[] =
> >   NULL, NULL, NULL
> >   },
> >
> > + {
> > + {"plancache_mode", PGC_USERSET, QUERY_TUNING_OTHER,
> > + gettext_noop("Forces use of custom or generic
> plans."),
> > + gettext_noop("It can control query plan cache.")
> > + },
> > + &plancache_mode,
> > + PLANCACHE_DEFAULT, plancache_mode_options,
> > + NULL, NULL, NULL
> > + },
>
> That long description is shorter than the short description.  How about:
>
> short: Controls the planner's selection of custom or generic plan.
> long: Prepared queries have custom and generic plans and the planner
> will attempt to choose which is better; this can be set to override
> the default behavior.
>

changed - thank you for text

>
> (either that, or just ditch the long description)
>
> > diff --git a/src/include/utils/plancache.h
> b/src/include/utils/plancache.h
> > index 87fab19f3c..962895cc1a 100644
> > --- a/src/include/utils/plancache.h
> > +++ b/src/include/utils/plancache.h
> > @@ -143,7 +143,6 @@ typedef struct CachedPlan
> >   MemoryContext context;  /* context containing this
> CachedPlan */
> >  } CachedPlan;
> >
> > -
> >  extern void InitPlanCache(void);
> >  extern void ResetPlanCache(void);
> >
>
> Random unrelated whitespace change...
>

fixed

>
> This is also missing documentation updates.
>

I wrote some basic doc, but native speaker should to write more words about
used strategies.


> Setting to Waiting for Author, but with those changes I would think we
> could mark it ready-for-committer, it seems pretty straight-forward to
> me and there seems to be general agreement (now) that it's worthwhile to
> have.
>
> Thanks!
>

attached updated patch

Regards

Pavel


> Stephen
>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 45b2af14eb..d6cef97ca9 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4387,6 +4387,30 @@ SELECT * FROM parent WHERE key = 2400;
   
  
 
+ 
+
+
+ 
+ Plan Cache Options
+
+ 
+
+ 
+  plancache_mode (enum)
+  
+   plancache_mode configuration parameter
+  
+  
+  
+   
+Prepared queries have custom and generic plans and the planner 
+will attempt to choose which is better; this can be set to override 
+the default behavior. The allowed values are default,
+force_custom_plan and force_generic_plan.
+   
+  
+ 
+
  
 

diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 8d7d8e04c9..1f8884896b 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -106,6 +106,8 @@ static void PlanCacheRelCallback(Datum arg, Oid relid);
 static void PlanCacheFuncCallback(Datum arg, int

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Pavel Stehule]

2018-01-23 15:35 GMT+01:00 Robert Haas :

> On Mon, Jan 22, 2018 at 5:15 PM, Stephen Frost  wrote:
> > Pavel,
> >
> > * Pavel Stehule (pavel.steh...@gmail.com) wrote:
> >> here is a GUC based patch for plancache controlling. Looks so this code
> is
> >> working.
> >
> > This really could use a new thread, imv.  This thread is a year old and
> > about a completely different feature than what you've implemented here.
>
> + if (plancache_mode & PLANCACHE_FORCE_GENERIC_PLAN)
> + return false;
> + if (plancache_mode & PLANCACHE_FORCE_CUSTOM_PLAN)
> + return true;
>
> This should be ==, not &.
>
> I could explain why & happens to work, but I won't.
>

you have true, thank you

Pavel


> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Robert Haas]

On Mon, Jan 22, 2018 at 5:15 PM, Stephen Frost  wrote:
> Pavel,
>
> * Pavel Stehule (pavel.steh...@gmail.com) wrote:
>> here is a GUC based patch for plancache controlling. Looks so this code is
>> working.
>
> This really could use a new thread, imv.  This thread is a year old and
> about a completely different feature than what you've implemented here.

+ if (plancache_mode & PLANCACHE_FORCE_GENERIC_PLAN)
+ return false;
+ if (plancache_mode & PLANCACHE_FORCE_CUSTOM_PLAN)
+ return true;

This should be ==, not &.

I could explain why & happens to work, but I won't.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Stephen Frost]

Pavel,

* Pavel Stehule (pavel.steh...@gmail.com) wrote:
> here is a GUC based patch for plancache controlling. Looks so this code is
> working.

This really could use a new thread, imv.  This thread is a year old and
about a completely different feature than what you've implemented here.

> It is hard to create regress tests. Any ideas?

Honestly, my idea for this would be to add another option to EXPLAIN
which would make it spit out generic and custom plans, or something of
that sort.

Please review your patches before sending them and don't send in patches
which have random unrelated whitespace changes.

> diff --git a/src/backend/utils/cache/plancache.c 
> b/src/backend/utils/cache/plancache.c
> index ad8a82f1e3..cc99cf6dcc 100644
> --- a/src/backend/utils/cache/plancache.c
> +++ b/src/backend/utils/cache/plancache.c
> @@ -1031,6 +1033,12 @@ choose_custom_plan(CachedPlanSource *plansource, 
> ParamListInfo boundParams)
>   if (IsTransactionStmtPlan(plansource))
>   return false;
>  
> + /* See if settings wants to force the decision */
> + if (plancache_mode & PLANCACHE_FORCE_GENERIC_PLAN)
> + return false;
> + if (plancache_mode & PLANCACHE_FORCE_CUSTOM_PLAN)
> + return true;

Not a big deal, but I probably would have just expanded the conditional
checks against cursor_options (just below) rather than adding
independent if() statements.

> diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
> index ae22185fbd..4ce275e39d 100644
> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
> @@ -3916,6 +3923,16 @@ static struct config_enum ConfigureNamesEnum[] =
>   NULL, NULL, NULL
>   },
>  
> + {
> + {"plancache_mode", PGC_USERSET, QUERY_TUNING_OTHER,
> + gettext_noop("Forces use of custom or generic plans."),
> + gettext_noop("It can control query plan cache.")
> + },
> + &plancache_mode,
> + PLANCACHE_DEFAULT, plancache_mode_options,
> + NULL, NULL, NULL
> + },

That long description is shorter than the short description.  How about:

short: Controls the planner's selection of custom or generic plan.
long: Prepared queries have custom and generic plans and the planner
will attempt to choose which is better; this can be set to override
the default behavior.

(either that, or just ditch the long description)

> diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
> index 87fab19f3c..962895cc1a 100644
> --- a/src/include/utils/plancache.h
> +++ b/src/include/utils/plancache.h
> @@ -143,7 +143,6 @@ typedef struct CachedPlan
>   MemoryContext context;  /* context containing this CachedPlan */
>  } CachedPlan;
>  
> -
>  extern void InitPlanCache(void);
>  extern void ResetPlanCache(void);
>  

Random unrelated whitespace change...

This is also missing documentation updates.

Setting to Waiting for Author, but with those changes I would think we
could mark it ready-for-committer, it seems pretty straight-forward to
me and there seems to be general agreement (now) that it's worthwhile to
have.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Michael Paquier]

On Thu, Oct 12, 2017 at 6:31 PM, Pavel Stehule  wrote:
> It is hard to create regress tests. Any ideas?

Moving to next CF per lack of reviews.
-- 
Michael