> I would like to oppose to the current version a little.
>
> Commits de3a2ea3b264 and 1d477a907e63 introduced elements that are more
> closely related to the execution phase. While the parameter
> es_parallel_workers_to_launch could be considered a planning parameter,
> es_parallel_workers_launched and es_total_processed should not.
Sure, I saw these new fields more in the realm of
"es_parallel_workers_to_launch",
which are planning related but are tracked in Estate for statistical purposes.
> Furthermore, any new code that utilises ExecutorStart/Run/End must
> ensure that the fields es_cached_plan and es_is_generic_plan are set
> correctly.
We really need to make sure ExecutorStart sets this correctly only. Estate
is carried over.
> IMO, the concept of a generic or custom plan is a fundamental property
> of the plan and should be stored within it - essentially in the
> PlannedStmt structure.
I do agree with this statement, and this idea did cross my mind; but I felt
that Estate is more ideal for statistics related fields, since we have a
precedent for this. But, I am open to the PlannedStmt idea as well.
> Additionally, it is unclear why we incur significant costs to alter the
> interface of ExplainOnePlan solely to track these parameters.
If we want to get this info to Estate, we have to.... if we go with PlannedStmt,
we do not.
> It may be more efficient to set the is_generic_plan option at the top
> plan node (PlannedStmt) and reference it wherever necessary. To identify
> a cached plan, we may consider pushing the CachedPlan/CachedPlanSource
> pointer down throughout pg_plan_query and maintaining a reference to the
> plan (or simply setting a boolean flag) at the same location — inside
> the PlannedStmt.
We will need a field to store an enum. let's call it CachedPlanType
with the types of cached plan. We need to be able to differentiate
when cached plans are not used, so a simple boolean is not
sufficient.
```
typedef enum CachedPlanType
{
PLAN_CACHE_NOT_SET, /* Not a cached plan */
PLAN_CACHE_GENERIC, /* Generic cached plan */
PLAN_CACHE_CUSTOM, /* Custom cached plan */
} CachedPlanType;
```
We can track a field for this enum in PlannedStmt and initialize
it to PLAN_CACHE_NOT_SET whenever we makeNode(PlannedStmt).
In GetCachedPlan, we iterate through plan->stmt_list to set the
value for the PlannedStmt.
CachedPlanType will have to be defined in nodes.h for this to work.
We can avoid the struct altogether and define the new PlannedStmt
field as an "int" and bit-pack the types.
I prefer the CachedPlanType struct to do this, as it's cleaner and
self-documenting.
> Such knowledge may provide an extra profit for planning - a generic
> plan, stored in the plan cache, may be reused later, and it may be
> profitable for an analytics-related extension to spend extra cycles and
> apply optimisation tricks more boldly.
Sure, either solution will provide this benefit.
--
Sami
