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

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 AM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
> 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.
>
> 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 only.

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?

--
Thanks, Amit Langote