Re: BUG #18830: ExecInitMerge Segfault on MERGE

On Tue, Mar 18, 2025 at 8:36 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On Mon, 17 Mar 2025 at 12:21, Amit Langote <amitlangote09@gmail.com> wrote:
> >
> > > 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?
>
> Yeah. It would have been nice to fix this without adding a new field
> to PlannedStmt at all, but I couldn't see an easy way to do that.
> Actually I don't think a list of lists or Btimapsets would be that
> large. There are unlikely to be more than one or two ModifyTable nodes
> in most queries, and the number of bottom-level elements would be less
> than in the flat resultRelations list. But still, it doesn't really
> seem worth the effort.

Ok, thanks -- appreciate your thoughts. I agree it would’ve been nice
to avoid adding a new field to PlannedStmt, but I couldn’t see a
simple way around it either, given the lock timing restrictions.

> > I've attached v5 with my commit message but were you planning to
> > commit it yourself?
>
> I'll leave it to you, since it was your commit originally :-)

Yes, of course. :-)

> > If not, does the commit message look good to you?
>
> It could perhaps be more specific about saying that
> ExecInitModifyTable() includes the first result relation if all others
> have been pruned, so it isn't always included.

Ok, done in the attached.

I'll push this tomorrow.

--
Thanks, Amit Langote