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

2018-01-30 9:06 GMT+01:00 Tatsuro Yamada <yamada.tatsuro@lab.ntt.co.jp>:

> 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.
>     =======================
>
>
Thank you very much

Regards

Pavel


> Regards,
> Tatsuro Yamada
>
>