On Tue, Nov 7, 2023 at 8:54 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > > Alvaro Herrera <alvhe...@alvh.no-ip.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