Hi Evan, On Mon, Nov 24, 2025 at 12:30 PM Chao Li <[email protected]> wrote: > > Hi, Amit, > > Locking only surviving partitions sounds a good optimization. I started to > review this patch, but I cannot finish reviewing in one day. I will post my > comments as long as I finished some commits.
Thank you very much for taking the time to review. > > On Nov 20, 2025, at 15:30, Amit Langote <[email protected]> wrote: > > > > <v3-0004-Use-pruning-aware-locking-in-cached-plans.patch><v3-0005-Add-test-exercising-prep-cleanup-on-cached-plan-i.patch><v3-0002-Introduce-ExecutorPrep-and-refactor-executor-star.patch><v3-0006-Make-SQL-function-executor-track-ExecutorPrep-sta.patch><v3-0003-Reuse-partition-pruning-results-in-parallel-worke.patch><v3-0001-Refactor-partition-pruning-initialization-for-cla.patch> > > > 0001 splits creations of es_part_prune_states into a new function > ExecCreatePartitionPruneStates(). With that, you are trying to make the code > clearer as you stated in the commit comment. However, the new function is not > called, meaning 0001 is not self-contained, feels unusual to me according to > the patches I have reviewed so far. Oops, that is not intentional. > I would suggest have ExecDoInitialPruning() call > ExecCreatePartitionPruneStates() when es_part_prune_states is still NIL., so > that current logic is unchanged, and 0001 can be pushed independently. 0002 adds a call to ExecDoInitialPruning() in ExecutorPrep(), preceded by a call to ExecCreatePartitionPruneStates(), and that is how I think it should be. So in the attached updated 0001, I have made InitPlan() call ExecCreatePartitionPruneStates() before calling ExecDoInitialPruning(). > 0002 moves check permission etc logic from InitPlan() to the new function > ExecutorPrep(). The commit message says “executor setup logic unchanged”. > Because in old code, before permission check, there was no > PushActiveSnapshot(), but in the patch, before check permission, > PushActiveSnapshot() is done, which may introduce different behavior, I just > wonder why PushActiveSnapshot() is added? That is a valid concern. I found it necessary because the initial pruning code (which runs in ExecDoInitialPruning()) may require ActiveSnapshot to be valid if pruning expressions end up calling code that invokes EnsurePortalSnapshotExists(). That requirement already existed when ExecDoInitialPruning() was driven from ExecutorStart(), but ExecutorPrep() can now be called from places that do not otherwise push a snapshot. The snapshot push is only there to cover those callers. It does not change permission checking itself, it just ensures ExecutorPrep() runs with the same preconditions that ExecutorStart() always had. > Actually, I am still trying to understand 0002-0004, it would take me some > time to fully understand the patch. I’d raise the above comments first. I > will continue reviewing this patch tomorrow. Thanks, I appreciate your review. -- Thanks, Amit Langote
v4-0002-Introduce-ExecutorPrep-and-refactor-executor-star.patch
Description: Binary data
v4-0003-Reuse-partition-pruning-results-in-parallel-worke.patch
Description: Binary data
v4-0006-Make-SQL-function-executor-track-ExecutorPrep-sta.patch
Description: Binary data
v4-0004-Use-pruning-aware-locking-in-cached-plans.patch
Description: Binary data
v4-0005-Add-test-exercising-prep-cleanup-on-cached-plan-i.patch
Description: Binary data
v4-0001-Refactor-partition-pruning-initialization-for-cla.patch
Description: Binary data
