On Wed, Jan 7, 2026 at 6:45 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Jan 2, 2026 at 6:58 AM Bernice Southey
> <bernice.southey@gmail.com> wrote:
> > I've looked much harder at the code (I'm still at
> > stumbling-around-in-the-dark-with-a-match level) and AFAICT, the two
> > approaches are very similar. I think equal effort is required to check
> > that PlannerGlobal.allRelids, unprunableRelids, and es_unpruned_relids
> > are correct, whichever approach is used. I can't find any missed cases
> > in either approach - with my matchlight.
>
> Good catch on the history -- that's exactly when the bug was
> introduced.
I remembered more on why I insisted on putting only relations with OID
(RTE_RELATION and RTE_SUBQUERY with relid set (views)) into
PlannerGlobal.allRelids and thus PlannedStmt.unprunableRelids. The
reason is that I did not want loops iterating over unprunableRelids to
have to skip relations that do not have an OID. For example,
AcquireExecutorLocks() currently iterates over the rtable and skips
non-RELATION RTEs; in a now-reverted commit (525392d57) I had changed
it to iterate over unprunableRelids instead, which avoided the need
for that skip logic entirely, because unprunableRelids by design only
contains OID-bearing relations:
- foreach(lc2, plannedstmt->rtable)
+ while ((rtindex = bms_next_member(plannedstmt->unprunableRelids,
+ rtindex)) >= 0)
{
- RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc2);
+ RangeTblEntry *rte = list_nth_node(RangeTblEntry,
+ plannedstmt->rtable,
+ rtindex - 1);
- if (!(rte->rtekind == RTE_RELATION ||
- (rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid))))
- continue;
+ Assert(rte->rtekind == RTE_RELATION ||
+ (rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid)));
This is why adding all RTEs to allRelids, as Tender suggested, would
be problematic -- it would defeat the design intent that allows loops
over unprunableRelids to skip the filtering logic. While there are no
such loops today (I reverted the patch that added one), keeping this
design makes sense. Dean's fix addresses the buggy bms_is_member()
check directly, without changing allRelids - albeit the name's a bit
misleading as has been mentioned.
--
Thanks, Amit Langote
