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

2018-01-22 23:15 GMT+01:00 Stephen Frost <sfrost@snowman.net>:

> Pavel,
>
> * Pavel Stehule (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 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
>