2018-01-22 23:15 GMT+01:00 Stephen Frost <[email protected]>:
> Pavel,
>
> * Pavel Stehule ([email protected]) 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;
</listitem>
</varlistentry>
+ </variablelist>
+ </sect2>
+
+ <sect2 id="runtime-config-query-plancache">
+ <title>Plan Cache Options</title>
+
+ <variablelist>
+
+ <varlistentry id="guc-plancache_mode" xreflabel="plancache_mode">
+ <term><varname>plancache_mode</varname> (<type>enum</type>)
+ <indexterm>
+ <primary><varname>plancache_mode</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ 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 <literal>default</literal>,
+ <literal>force_custom_plan</literal> and <literal>force_generic_plan</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</sect2>
</sect1>
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 cacheid, uint32 hashvalue);
static void PlanCacheSysCallback(Datum arg, int cacheid, uint32 hashvalue);
+/* GUC parameter */
+int plancache_mode;
/*
* InitPlanCache: initialize module during InitPostgres.
@@ -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;
+
/* 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 5884fa905e..52d9c20de1 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -403,6 +403,13 @@ static const struct config_enum_entry force_parallel_mode_options[] = {
{NULL, 0, false}
};
+static const struct config_enum_entry plancache_mode_options[] = {
+ {"default", PLANCACHE_DEFAULT, 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,
@@ -3945,6 +3952,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_DEFAULT, 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..0a71093f06 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_DEFAULT,
+ 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 c2eeff1614..cbe014a8d7 100644
--- a/src/test/regress/expected/plancache.out
+++ b/src/test/regress/expected/plancache.out
@@ -278,3 +278,82 @@ drop table list_part_1;
execute pstmt_def_insert(1);
drop table list_parted, list_part_null;
deallocate pstmt_def_insert;
+--
+-- Test plan cache strategy
+--
+create table test_strategy(a int);
+insert into test_strategy select 1 from generate_series(1,1000) union all select 2;
+create index on test_strategy(a);
+analyze test_strategy;
+prepare test_strategy_pp(int) as select count(*) from test_strategy where a = $1;
+-- without 5 evaluation pg uses custom plan
+explain (costs off) execute test_strategy_pp(2);
+ QUERY PLAN
+------------------------------------------------------------------
+ Aggregate
+ -> Index Only Scan using test_strategy_a_idx on test_strategy
+ Index Cond: (a = 2)
+(3 rows)
+
+-- we can force to generic plan
+set plancache_mode to force_generic_plan;
+explain (costs off) execute test_strategy_pp(2);
+ QUERY PLAN
+---------------------------------
+ Aggregate
+ -> Seq Scan on test_strategy
+ Filter: (a = $1)
+(3 rows)
+
+-- we can fix generic plan by 5 execution
+set plancache_mode to default;
+execute test_strategy_pp(1); -- 1x
+ count
+-------
+ 1000
+(1 row)
+
+execute test_strategy_pp(1); -- 2x
+ count
+-------
+ 1000
+(1 row)
+
+execute test_strategy_pp(1); -- 3x
+ count
+-------
+ 1000
+(1 row)
+
+execute test_strategy_pp(1); -- 4x
+ count
+-------
+ 1000
+(1 row)
+
+execute test_strategy_pp(1); -- 5x
+ count
+-------
+ 1000
+(1 row)
+
+-- we should to get really bad plan
+explain (costs off) execute test_strategy_pp(2);
+ QUERY PLAN
+---------------------------------
+ Aggregate
+ -> Seq Scan on test_strategy
+ Filter: (a = $1)
+(3 rows)
+
+-- but we can force to custom plan
+set plancache_mode to force_custom_plan;
+explain (costs off) execute test_strategy_pp(2);
+ QUERY PLAN
+------------------------------------------------------------------
+ Aggregate
+ -> Index Only Scan using test_strategy_a_idx on test_strategy
+ Index Cond: (a = 2)
+(3 rows)
+
+drop table test_strategy;
diff --git a/src/test/regress/sql/plancache.sql b/src/test/regress/sql/plancache.sql
index cb2a551487..8241c1464e 100644
--- a/src/test/regress/sql/plancache.sql
+++ b/src/test/regress/sql/plancache.sql
@@ -177,3 +177,37 @@ drop table list_part_1;
execute pstmt_def_insert(1);
drop table list_parted, list_part_null;
deallocate pstmt_def_insert;
+
+--
+-- Test plan cache strategy
+--
+create table test_strategy(a int);
+insert into test_strategy select 1 from generate_series(1,1000) union all select 2;
+create index on test_strategy(a);
+analyze test_strategy;
+
+prepare test_strategy_pp(int) as select count(*) from test_strategy where a = $1;
+
+-- without 5 evaluation pg uses custom plan
+explain (costs off) execute test_strategy_pp(2);
+
+-- we can force to generic plan
+set plancache_mode to force_generic_plan;
+explain (costs off) execute test_strategy_pp(2);
+
+-- we can fix generic plan by 5 execution
+set plancache_mode to default;
+execute test_strategy_pp(1); -- 1x
+execute test_strategy_pp(1); -- 2x
+execute test_strategy_pp(1); -- 3x
+execute test_strategy_pp(1); -- 4x
+execute test_strategy_pp(1); -- 5x
+
+-- we should to get really bad plan
+explain (costs off) execute test_strategy_pp(2);
+
+-- but we can force to custom plan
+set plancache_mode to force_custom_plan;
+explain (costs off) execute test_strategy_pp(2);
+
+drop table test_strategy;
