On 2018/01/24 1:08, Pavel Stehule wrote:
2018-01-22 23:15 GMT+01:00 Stephen Frost <sfr...@snowman.net
<mailto:sfr...@snowman.net>>:
Pavel,
* Pavel Stehule (pavel.steh...@gmail.com <mailto: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
