Re: BUG #18830: ExecInitMerge Segfault on MERGE

On Sun, Mar 16, 2025 at 6:57 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On Fri, 14 Mar 2025 at 02:48, Amit Langote <amitlangote09@gmail.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.

Thanks for making those changes.

> * 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.

Or a List of Bitmapset, so that bms_intersect() can be used to check
whether all result relations of a given ModifyTable have been pruned.
But whether it’s that or a List of Lists, both would add a potentially
large amount of memory to PlannedStmt, to avoid taking an extra lock.
Probably not a great tradeoff, but we can revisit if it becomes
worthwhile?

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

I've attached v5 with my commit message but were you planning to
commit it yourself? If not, does the commit message look good to you?

--
Thanks, Amit Langote