MIME-Version: 1.0
References: <19355-57d7d52ea4980dc6@postgresql.org> <868ff2a518820c8864b6d28510294b2457a126af.camel@cybertec.at>
 <CANjBVm3AMm=9-mB7=uqJjaWN-+RrA8+w-gXhZn6L7VHu7K_Urw@mail.gmail.com>
 <CAEZATCVcV4cfqGnmPGVxwjsPh7rxsTD510=KOGFyi2_FMgqWaQ@mail.gmail.com>
 <CA+HiwqE8gNK7Utqo2EAKh24KExWfypt06m1EFNXBd5TFDO2f6w@mail.gmail.com>
 <CAHewXNn8ApcYfZKjZ1R8Z7yHRoi+C+mu5r9LbZUbb6691d6Rrw@mail.gmail.com> <CAEZATCVr+=F=iTbi3XyABSEr5Cqw=ptm2PyGc75SaQ1JYrPnsg@mail.gmail.com>
In-Reply-To: <CAEZATCVr+=F=iTbi3XyABSEr5Cqw=ptm2PyGc75SaQ1JYrPnsg@mail.gmail.com>
From: Tender Wang <tndrwang@gmail.com>
Date: Thu, 25 Dec 2025 12:48:08 +0800
Message-ID: <CAHewXN=D2SUh1CvE0gnk1Ev_k4RAEO9aoEgQZG_7eAuSWxZA0Q@mail.gmail.com>
Subject: Re: BUG #19355: Attempt to insert data unexpectedly during concurrent update
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Amit Langote <amitlangote09@gmail.com>, Bh W <wangbihua.cn@gmail.com>, 
	Laurenz Albe <laurenz.albe@cybertec.at>, pgsql-bugs@lists.postgresql.org
Content-Type: multipart/alternative; boundary="00000000000078bd7d0646bf7cbe"
Archived-At: <https://www.postgresql.org/message-id/CAHewXN%3DD2SUh1CvE0gnk1Ev_k4RAEO9aoEgQZG_7eAuSWxZA0Q%40mail.gmail.com>
Precedence: bulk

--00000000000078bd7d0646bf7cbe
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Dean Rasheed <dean.a.rasheed@gmail.com> =E4=BA=8E2025=E5=B9=B412=E6=9C=8825=
=E6=97=A5=E5=91=A8=E5=9B=9B 07:12=E5=86=99=E9=81=93=EF=BC=9A

> On Wed, 24 Dec 2025 at 12:07, Tender Wang <tndrwang@gmail.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 =3D t3.a)
when matched then
      update set b =3D 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 =3D
t3.a)
when matched then
      update set b =3D 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=
.


--=20
Thanks,
Tender Wang

--00000000000078bd7d0646bf7cbe
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote g=
mail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">Dean Rasheed &l=
t;<a href=3D"mailto:dean.a.rasheed@gmail.com">dean.a.rasheed@gmail.com</a>&=
gt; =E4=BA=8E2025=E5=B9=B412=E6=9C=8825=E6=97=A5=E5=91=A8=E5=9B=9B 07:12=E5=
=86=99=E9=81=93=EF=BC=9A<br></div><blockquote class=3D"gmail_quote" style=
=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding=
-left:1ex">On Wed, 24 Dec 2025 at 12:07, Tender Wang &lt;<a href=3D"mailto:=
tndrwang@gmail.com" target=3D"_blank">tndrwang@gmail.com</a>&gt; wrote:<br>
&gt;<br>
&gt; I did some debugging, and I found that:<br>
&gt; In=C2=A0 add_rte_to_flat_rtable(),=C2=A0 the RTE of value was not adde=
d into glob-&gt;AllRelids, because below codes:<br>
&gt; .....<br>
&gt; the estate-&gt;es_unpruned_relids equals with result-&gt;unprunableRel=
ids contains. So the rowMark was skipped incorrectly.<br>
&gt;<br>
&gt; I did a quick fix as the attached patch.<br>
&gt; Any thoughts?<br>
<br>
Yes. However, it&#39;s not sufficient to only add RTE_VALUES RTEs to what<b=
r>
gets included in PlannerGlobal.allRelids. Rowmarks can be attached to<br>
other kinds of RTEs. An obvious example is an RTE_SUBQUERY RTE that is<br>
an actual subquery that did not come from a view. So, for this<br>
approach to work robustly, it really should include *all* RTEs in<br>
PlannerGlobal.allRelids.<br></blockquote><div><br></div><div>Yes,=C2=A0 it =
is. I can reproduce the same with a subquery as below:</div><div><br></div>=
merge into t1 using (select a from (values(1,1),(2,2) as t4(a,b)) as t3(a) =
on (t1.a =3D t3.a)<br>when matched then<br>=C2=A0 =C2=A0 =C2=A0 update set =
b =3D t1.b + 1<br>when not matched then<br><div>=C2=A0 =C2=A0 =C2=A0 insert=
 (a,b) values (1,1);</div><div>or</div>merge into t1 using (select generate=
_series(1,2) as a) as t3 =C2=A0on (t1.a =3D t3.a)<br>when matched then<br>=
=C2=A0 =C2=A0 =C2=A0 update set b =3D t1.b + 1<br>when not matched then<br>=
<div>=C2=A0 =C2=A0 =C2=A0 insert (a,b) values (1,1);</div><div>=C2=A0</div>=
<div>All reported the same error. My approach obviously can not cover all c=
ases.</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.=
8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I took a slightly different approach, which was to change the test in<br>
InitPlan() (and also in ExecInitLockRows() and ExecInitModifyTable())<br>
to only ignore rowmarks for pruned relations that are plain<br>
RTE_RELATION relations, since those are the only relations that are<br>
ever actually pruned. So rowmarks attached to any other kind of RTE<br>
are not ignored. I also added an isolation test case.<br></blockquote><div>=
<br></div><div>I failed to apply your patch, some errors, like &quot;error:=
 git apply: bad git-diff - expected /dev/null on line 2&quot;</div><div>or =
&quot;Patch format detection failed.&quot;</div><div>=C2=A0I looked through=
 the patch. It worked for me.</div><div><br></div><blockquote class=3D"gmai=
l_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,20=
4,204);padding-left:1ex">
<br>
I&#39;m somewhat conflicted as to which approach is better. I think maybe<b=
r>
there is less chance of other unintended side-effects if the set of<br>
RTEs included in PlannerGlobal.allRelids, unprunableRelids, and<br>
es_unpruned_relids is not changed. However, as it stands,<br>
PlannerGlobal.allRelids is misnamed (it should probably have been<br>
called &quot;relationRelids&quot;, in line with the &quot;relationOids&quot=
; field).<br>
Making it actually include all RTEs would solve that.<br></blockquote><div>=
.....</div><div>=C2=A0/*</div>	 * RT indexes of all relation RTEs in finalr=
table (RTE_RELATION and<br>	 * RTE_SUBQUERY RTEs of views)<br>	 */</div><di=
v class=3D"gmail_quote gmail_quote_container">Bitmapset =C2=A0*allRelids;</=
div><div class=3D"gmail_quote gmail_quote_container">......</div><div class=
=3D"gmail_quote gmail_quote_container">Per the comment, I guess Amit didn&#=
39;t plan to include all RTEs in his design.</div><div class=3D"gmail_quote=
 gmail_quote_container">=C2=A0</div><div><br></div><span class=3D"gmail_sig=
nature_prefix">-- </span><br><div dir=3D"ltr" class=3D"gmail_signature"><di=
v dir=3D"ltr"><div>Thanks,</div><div>Tender Wang</div></div></div></div>

--00000000000078bd7d0646bf7cbe--