Re: [HACKERS] PoC plpgsql - possibility to force custom or genericplan
От | Tatsuro Yamada |
---|---|
Тема | Re: [HACKERS] PoC plpgsql - possibility to force custom or genericplan |
Дата | |
Msg-id | 9d64aea7-7b0e-13c2-c772-b8559c6727b4@lab.ntt.co.jp обсуждение исходный текст |
Ответ на | Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan (Pavel Stehule <pavel.stehule@gmail.com>) |
Ответы |
Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan
|
Список | pgsql-hackers |
On 2018/01/24 1:08, Pavel Stehule wrote: > > > 2018-01-22 23:15 GMT+01:00 Stephen Frost <sfrost@snowman.net <mailto:sfrost@snowman.net>>: > > Pavel, > > * Pavel Stehule (pavel.stehule@gmail.com <mailto:pavel.stehule@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 andcode 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
В списке pgsql-hackers по дате отправления: