Re: track generic and custom plans in pg_stat_statements

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Sami Imseih <samimseih(at)gmail(dot)com>
Cc: Andrei Lepikhov <lepihov(at)gmail(dot)com>, Greg Sabino Mullane <htamfids(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Nikolay Samokhvalov <nik(at)postgres(dot)ai>, Ilia Evdokimov <ilya(dot)evdokimov(at)tantorlabs(dot)com>
Subject: Re: track generic and custom plans in pg_stat_statements
Date: 2025-07-23 03:15:06
Message-ID: aIBTulWwle8XseNt@paquier.xyz
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Jul 22, 2025 at 02:52:49PM -0500, Sami Imseih wrote:
>> 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.

Guess so.

>> 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.
>
> v13 is the implementation using PlannedStmt as described above.

+ PLAN_CACHE_NOT_SET, /* Not a cached plan */

This should be set to 0 by default, but we could enforce the
definition as well with a PLAN_CACHE_NOT_SET = 0? Nit: perhaps name
it PLAN_CACHE_NONE to outline the fact that it is just not a cached
plan?

--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -131,6 +131,9 @@ typedef struct PlannedStmt
/* non-null if this is utility stmt */
Node *utilityStmt;

+ /* type of cached plan, if it is one */
+ CachedPlanType cached_plan_type;
[...]
+ foreach(lc, plan->stmt_list)
+ {
+ PlannedStmt *pstmt = (PlannedStmt *) lfirst(lc);
+
+ pstmt->cached_plan_type = customplan ? PLAN_CACHE_CUSTOM : PLAN_CACHE_GENERIC;
+ }

Yes, this implementation feels much more natural, with a state
set to the result that we get when directly retrieving it from the
cached plan layer, pushing the data in the executor end hook in PGSS.
No objections to this approach; I can live with that.

A small thing that would be cleaner is to split the patch into two
parts: one for the in-core backend addition and a second for PGSS.
Code paths are different, so it's simple to do.

Andrei, what do you think about all that?
--
Michael

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2025-07-23 03:23:37 Re: Verify predefined LWLocks tranches have entries in wait_event_names.txt
Previous Message Thomas Munro 2025-07-23 02:44:29 Re: Optimize LISTEN/NOTIFY