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

Pavel,

* Pavel Stehule (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.

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

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.

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

(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...

This is also missing documentation updates.

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!

Stephen