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@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 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