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