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

2018-01-30 9:06 GMT+01:00 Tatsuro Yamada <yamada(dot)tatsuro(at)lab(dot)ntt(dot)co(dot)jp>:

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

Regards

Pavel

> Regards,
> Tatsuro Yamada
>
>