Re: BUG #19355: Attempt to insert data unexpectedly during concurrent update

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> 于2025年12月25日周四 07:12写道：

> On Wed, 24 Dec 2025 at 12:07, Tender Wang <tndrwang(at)gmail(dot)com> wrote:
> >
> > I did some debugging, and I found that:
> > In add_rte_to_flat_rtable(), the RTE of value was not added into
> glob->AllRelids, because below codes:
> > .....
> > the estate->es_unpruned_relids equals with result->unprunableRelids
> contains. So the rowMark was skipped incorrectly.
> >
> > I did a quick fix as the attached patch.
> > Any thoughts?
>
> Yes. However, it's not sufficient to only add RTE_VALUES RTEs to what
> gets included in PlannerGlobal.allRelids. Rowmarks can be attached to
> other kinds of RTEs. An obvious example is an RTE_SUBQUERY RTE that is
> an actual subquery that did not come from a view. So, for this
> approach to work robustly, it really should include *all* RTEs in
> PlannerGlobal.allRelids.
>

Yes, it is. I can reproduce the same with a subquery as below:

merge into t1 using (select a from (values(1,1),(2,2) as t4(a,b)) as t3(a)
on (t1.a = t3.a)
when matched then
update set b = t1.b + 1
when not matched then
insert (a,b) values (1,1);
or
merge into t1 using (select generate_series(1,2) as a) as t3 on (t1.a =
t3.a)
when matched then
update set b = t1.b + 1
when not matched then
insert (a,b) values (1,1);

All reported the same error. My approach obviously can not cover all cases.

>
> I took a slightly different approach, which was to change the test in
> InitPlan() (and also in ExecInitLockRows() and ExecInitModifyTable())
> to only ignore rowmarks for pruned relations that are plain
> RTE_RELATION relations, since those are the only relations that are
> ever actually pruned. So rowmarks attached to any other kind of RTE
> are not ignored. I also added an isolation test case.
>

I failed to apply your patch, some errors, like "error: git apply: bad
git-diff - expected /dev/null on line 2"
or "Patch format detection failed."
I looked through the patch. It worked for me.

> I'm somewhat conflicted as to which approach is better. I think maybe
> there is less chance of other unintended side-effects if the set of
> RTEs included in PlannerGlobal.allRelids, unprunableRelids, and
> es_unpruned_relids is not changed. However, as it stands,
> PlannerGlobal.allRelids is misnamed (it should probably have been
> called "relationRelids", in line with the "relationOids" field).
> Making it actually include all RTEs would solve that.
>
.....
/*
* RT indexes of all relation RTEs in finalrtable (RTE_RELATION and
* RTE_SUBQUERY RTEs of views)
*/
Bitmapset *allRelids;
......
Per the comment, I guess Amit didn't plan to include all RTEs in his design.

--
Thanks,
Tender Wang