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

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

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