Re: BUG #18830: ExecInitMerge Segfault on MERGE

On Thu, Mar 13, 2025 at 11:42 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On Thu, 13 Mar 2025 at 12:44, Amit Langote <amitlangote09@gmail.com> wrote:
> >
> > One thing I'd like to avoid is taking any locks during ExecInitNode(),
> > including in functions like ExecGetRangeTableRelation(). Doing so
> > would require checking whether the CachedPlan is still valid and
> > handling invalidation if it isn't -- see commit 525392d5727f for
> > background.
>
> Yes, I wondered about that. I saw that earlier commit, and the notes
> in the executor README, but I still don't quite understand.
> Specifically, how is locking this pruned leaf partition in
> ExecInitModifyTable() different from locking any other pruned leaf
> partition later in the INSERT path? (For example, as part of an UPDATE
> that moves a tuple from an unpruned partition to a pruned one.)

I think the key distinction is that the result relation we’re locking
was already present in the plan, and the executor code that uses
resultRelInfo[0] doesn’t rely on traversing the plan tree -- it
operates entirely on the state info built in ExecInitModifyTable(). So
in this specific case, the usual plan invalidation hazard may not
apply in the same way.

I understand the comparison to locking pruned partitions during UPDATE
execution, but I think that case is different: by then, the executor
is fully initialized, and while invalidations can still happen,
they’re assumed to be harmless for the remainder of execution. That’s
why the system doesn’t check for plan validity or attempt to restart
once execution has begun -- it’s expected to continue with the
existing plan, under the assumption that any changes would only affect
pruned partitions that are unreachable, especially via the scan path.
Since those plan nodes remain unreferenced during tuple routing which
only uses state initialized during execution, such changes don’t
impact correctness.

That said, my concern remains that taking any new locks during
ExecInitNode() risks triggering CachedPlan validation logic that we’ve
intentionally avoided dealing with, as per 525392d5727f. Even if this
particular usage might not trip over it, it still feels like we’re
stepping into a sensitive area.  So I’d prefer to keep the locking in
ExecDoInitialPruning(), where interacting with plan invalidation is
expected and safe — and we don’t risk leaving the executor in a
partially initialized state.

--
Thanks, Amit Langote