From: | Amit Langote <amitlangote09(at)gmail(dot)com> |
---|---|
To: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> |
Cc: | Tender Wang <tndrwang(at)gmail(dot)com>, David Rowley <dgrowleyml(at)gmail(dot)com>, pgsql-bugs(at)lists(dot)postgresql(dot)org, tharakan(at)gmail(dot)com |
Subject: | Re: BUG #18830: ExecInitMerge Segfault on MERGE |
Date: | 2025-03-17 12:21:22 |
Message-ID: | CA+HiwqFD+_i_O_97HxhxOX1KM8i-dgD_9eHRi5=1zCMmq9USGg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-bugs |
On Sun, Mar 16, 2025 at 6:57 PM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> 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.
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
Attachment | Content-Type | Size |
---|---|---|
v5-0001-Ensure-first-result-relation-is-included-even-if-.patch | application/octet-stream | 20.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tender Wang | 2025-03-17 15:21:33 | Re: BUG #18852: Unexpected expression in subquery output |
Previous Message | Daniel Gustafsson | 2025-03-17 08:26:43 | Re: BUG #18845: DEREF_OF_NULL.RET guc_malloc possibly returns NULL |