On 22/7/2025 01:07, Michael Paquier wrote:
> On Mon, Jul 21, 2025 at 04:47:31PM -0500, Sami Imseih wrote:
>> Last week I published a v11 that adds a field to QueryDesc, but as I thought
>> about this more, how about we just add 2 bool fields in QueryDesc->estate
>> ( es_cached_plan and es_is_generic_plan ), a field in CachedPlan (
>> is_generic_plan )
>> and after ExecutorStart, and if we have a cplan, we set the
>> appropriate plan cache
>> mode value. I think it's better to confine these new fields in Estate
>> rather than
>> at a higher level like QueryDesc. See v12.
>
> Yes, I think that this is a much better idea to isolate the whole
> concept and let pgss grab these values. We have lived with such
> additions for monitoring in EState a few times already, see for
> example de3a2ea3b264 and 1d477a907e63 that are tainted with my
> fingerprints.
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.
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.
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.
Additionally, it is unclear why we incur significant costs to alter the
interface of ExplainOnePlan solely to track these parameters.
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.
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.
--
regards, Andrei Lepikhov
