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