Thread
-
Re: generic plans and "initial" pruning
amit <amitlangote09@gmail.com> — 2025-11-25T08:31:04Z
Hi Evan, On Mon, Nov 24, 2025 at 12:30 PM Chao Li <li.evan.chao@gmail.com> 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 <amitlangote09@gmail.com> 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