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
reviewingin 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
aretrying to make the code clearer as you stated in the commit comment. However, the new function is not called,
meaning0001 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
“executorsetup logic unchanged”. Because in old code, before permission check, there was no PushActiveSnapshot(), but
inthe patch, before check permission, PushActiveSnapshot() is done, which may introduce different behavior, I just
wonderwhy 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
raisethe above comments first. I will continue reviewing this patch tomorrow.
Thanks, I appreciate your review.
--
Thanks, Amit Langote