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

2018-01-22 23:15 GMT+01:00 Stephen Frost <sfrost(at)snowman(dot)net>:

> Pavel,
>
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)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
>