2018-01-30 9:06 GMT+01:00 Tatsuro Yamada <yamada.tats...@lab.ntt.co.jp>:
> 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. > ======================= > > Thank you very much Regards Pavel > Regards, > Tatsuro Yamada > >