On Fri, Dec 9, 2022 at 8:37 PM Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > On 2022-Dec-09, Amit Langote wrote: > > > Pruning will be done afresh on every fetch of a given cached plan when > > CheckCachedPlan() is called on it, so the part_prune_results_list part > > will be discarded and rebuilt as many times as the plan is executed. > > You'll find a description around CachedPlanSavePartitionPruneResults() > > that's in v12. > > I see. > > In that case, a separate container struct seems warranted.
I thought about this today and played around with some container struct ideas. Though, I started feeling like putting all the new logic being added by this patch into plancache.c at the heart of GetCachedPlan() and tweaking its API in kind of unintuitive ways may not have been such a good idea to begin with. So I started thinking again about your GetRunnablePlan() wrapper idea and thought maybe we could do something with it. Let's say we name it GetCachedPlanLockPartitions() and put the logic that does initial pruning with the new ExecutorDoInitialPruning() in it, instead of in the normal GetCachedPlan() path. Any callers that call GetCachedPlan() instead call GetCachedPlanLockPartitions() with either the List ** parameter as now or some container struct if that seems better. Whether GetCachedPlanLockPartitions() needs to do anything other than return the CachedPlan returned by GetCachedPlan() can be decided by the latter setting, say, CachedPlan.has_unlocked_partitions. That will be done by AcquireExecutorLocks() when it sees containsInitialPrunnig in any of the PlannedStmts it sees, locking only the PlannedStmt.minLockRelids set (which is all relations where no pruning is needed!), leaving the partition locking to GetCachedPlanLockPartitions(). If the CachedPlan is invalidated during the partition locking phase, it calls GetCachedPlan() again; maybe some refactoring is needed to avoid too much useless work in such cases. Thoughts? -- Thanks, Amit Langote EDB: http://www.enterprisedb.com