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. > While looking at it, I noticed that tcopprot.h includes both plannodes.h > and parsenodes.h, but there's no reason to include the latter (or at > least headerscheck doesn't complain after removing it), so I propose to > remove it, per 0001 here. 0001 is ok, except check #include alphabetization. > I also noticed while looking that I messed up in commit 7103ebb7aae8 > ("Add support for MERGE SQL command") on this point, because I added > #include parsenodes.h to plannodes.h. This is because MergeAction, > which is in parsenodes.h, is also needed by some executor code. But the > real way to fix that is to define that struct in primnodes.h. 0002 does > that. (I'm forced to also move enum OverridingKind there, which is a > bit annoying.) This seems OK. It seems to me that parsenodes.h has been required by plannodes.h for a long time, but if we can decouple them, all the better. > 0003 here is your patch, which I include because of conflicts with my > 0002. Still don't like it. > ... I would be more at ease if we didn't have to include > parsenodes.h in pathnodes.h, though, but that looks more difficult to > achieve. Yeah, that dependency has been there a long time too. I'm not too fussed by dependencies on parsenodes.h, because anything involved with either planning or execution will certainly be looking at query trees too. But I don't want to add dependencies that tie planning and execution together. regards, tom lane