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: Amit Langote <amitlangote09@gmail.com>
Date: Wed, 7 Jan 2026 18:37:12 +0900
Message-ID: <CA+HiwqGCfYgT1DnnK836qRX63okKNeNR=CQmshC6=aATCw6VVA@mail.gmail.com>
Subject: Re: BUG #19355: Attempt to insert data unexpectedly during concurrent update
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Tender Wang <tndrwang@gmail.com>, Bh W <wangbihua.cn@gmail.com>, 
	Laurenz Albe <laurenz.albe@cybertec.at>, pgsql-bugs@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CA%2BHiwqGCfYgT1DnnK836qRX63okKNeNR%3DCQmshC6%3DaATCw6VVA%40mail.gmail.com>
Precedence: bulk

Hi Dean,

Apologies for the delay in getting back to this thread (vacation got
unexpectedly long for family reasons).

On Thu, Dec 25, 2025 at 8:12=E2=80=AFAM Dean Rasheed <dean.a.rasheed@gmail.=
com> wrote:
>
> 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 con=
tains. 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.
>
> 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'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.

Yes, I think the approach in your patch seems better for the reason
you mentioned, at least for back-patching sanity.

I intended all of these relid sets to account for prunable RELATION RTEs on=
ly.

Thanks Tender and Bernice for the additional analysis. I prefer Dean's
fix-the-executor approach for back-patching. Bernice, are there other
related issues you're aware of beyond this rowmark bug? Want to make
sure Dean's patch covers them too.

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

Yeah, I agree it should have been named relationRelids. Perhaps worth
renaming in a separate cleanup, though not urgent.

Thanks for the patch!  Do you intend to commit and back-patch this
yourself, or would you like me to handle it?

--=20
Thanks, Amit Langote