Re: Relids instead of Bitmapset * in plannode.h

On Tue, Nov 7, 2023 at 8:54 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> > On 2023-Oct-31, Ashutosh Bapat wrote:
> >> For some reason plannode.h has declared variable to hold RTIs as
> >> Bitmapset * instead of Relids like other places. Here's patch to fix
> >> it. This is superficial change as Relids is typedefed to Bitmapset *.
> >> Build succeeds for me and also make check passes.
>
> > I think the reason for having done it this way, was mostly to avoid
> > including pathnodes.h in plannodes.h.
>
> Yes, I'm pretty sure that's exactly the reason, and I'm strongly
> against the initially-proposed patch. The include footprint of
> pathnodes.h would be greatly expanded, for no real benefit.
> Among other things, that fuzzes the distinction between planner
> modules and non-planner modules.

As I mentioned in [1] the Bitmapset implementation is not space
efficient to be used as Relids when there are thousands of partitions.
I was assessing all usages of Bitmapset to find if there are other
places where this is an issue. That's when I noticed this. At some
point in future (possibly quite near) when queries will involved
thousands of relations (partitions or otherwise) we will need to
implement Relids in more space efficient way. Having all Relids usages
of Bitmapset labelled as Relids will help us then. If we don't want to
add pathnodes.h to plannodes.h there will be more work to identify
Relids usage. That shouldn't be a couple of days work, so it's ok.

Other possibilities are
1. Define Relids in bitmapset.h itself and use Relids everywhere
Bitmapset * is really Relids. Wherever Relids is used bitmapset.h must
have been included one or other other way. That's a bigger churn.

2. Replace RTIs with Relids in the comments and add the following
comment somewhere near the #include section. "The Relids members in
various structures in this file have been declared as Bitmapset * to
avoid including pathnodes.h in this file. This include has greatly
expanded footprint for no real benefit.".

3. Do nothing right now. If and when we implement Relids as a separate
datastructure, it will get its own module. We will be able to place it
somewhere properly.

I have no additional comments on other patches.

[1] https://www.postgresql.org/message-id/CAExHW5s4EqY43oB%3Dne6B2%3D-xLgrs9ZGeTr1NXwkGFt2j-OmaQQ%40mail.gmail.com

--
Best Wishes,
Ashutosh Bapat