Re: Relids instead of Bitmapset * in plannode.h

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.

> 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