Re: BUG #18830: ExecInitMerge Segfault on MERGE

On Fri, 14 Mar 2025 at 02:48, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> 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.
>

OK, I think that makes sense. Trapping any plan invalidations early,
after ExecDoInitialPruning(), and before initialising the rest of the
executor state does seem preferable for that reason.

I have a couple of comments on the v3 patch:

* In ExecInitModifyTable(), it's possible to avoid code duplication. I
think that's worth it to make the code more maintainable if more
per-result relation logic is added in the future.

* In executor.h, the name of the new function argument doesn't match
the .c source file.

* In ExecDoInitialPruning(), there is room for improvement: we
actually only need to lock the first result relation if all other
result relations of the ModifyTable node have been pruned. As it
stands, there's no easy way to tell that, so I've just made a note of
it in a comment. I wondered about replacing the new field
"firstResultRels" with something like "mtResultRels", which would be a
list of lists, to allow that, but I didn't try it.

I'm attaching v4, which addresses those comments, and includes a few
other comment update suggestions.

Regards,
Dean