Message-ID: <a0432b5d-84e1-4678-9963-bf5ecb749dd0@gmail.com>
Date: Tue, 22 Jul 2025 15:44:11 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: track generic and custom plans in pg_stat_statements
To: Michael Paquier <michael@paquier.xyz>, Sami Imseih <samimseih@gmail.com>
Cc: Greg Sabino Mullane <htamfids@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>,
 Nikolay Samokhvalov <nik@postgres.ai>,
 Ilia Evdokimov <ilya.evdokimov@tantorlabs.com>
References: 
 <CAA5RZ0v4Lqc3KOzFCOJd0hjBpF10W=-E6xn0z_HizZY9uxPR0w@mail.gmail.com>
 <CAA5RZ0vK_fah+ZS00hdg9uH2J_zBgV75x6ojYrc6+OJudnbKtQ@mail.gmail.com>
 <5f775987-8a93-452d-bb3f-a1de53836406@gmail.com>
 <CAA5RZ0uxcVu9MPXQc6qJ5mtYhH=_7b7jjaDnvg6Sft8jJ1NbXw@mail.gmail.com>
 <56d31e81-79d8-40bd-b957-0d6dc7c46589@gmail.com>
 <CAA5RZ0tdc4HeHdFRvVPWBdXP1JwCSjPPgiQyFk8fwhv=4fKmjw@mail.gmail.com>
 <2f2bde53-972d-402c-9da3-c5467cca00bf@gmail.com>
 <CAA5RZ0tfr50T1xYET=1Ei=NkbmS3e7MFhMJTrnoLi2oO7n2JsQ@mail.gmail.com>
 <064b9efd-92c9-4670-95f4-d911d4c7ddeb@gmail.com>
 <CAA5RZ0ss-jmqeze0G8anztzLRJuMEgz9y3gTd-rgTc2Ejtwzvg@mail.gmail.com>
 <aH7IPp_Hku02xHP2@paquier.xyz>
Content-Language: en-US
From: Andrei Lepikhov <lepihov@gmail.com>
In-Reply-To: <aH7IPp_Hku02xHP2@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/a0432b5d-84e1-4678-9963-bf5ecb749dd0%40gmail.com>
Precedence: bulk

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