Hello,
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. Did you explore what the
consequences are? Starting here:
https://doxygen.postgresql.org/plannodes_8h.html
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. There's a couple of files that need to be
repaired for this change. windowfuncs.c is a no-brainer. However,
having to edit bootstrap.h is a bit surprising -- I think before
dac048f71ebb ("Build all Flex files standalone") this inclusion wasn't
necessary, because the .y file already includes parsenodes.h; but after
that commit, bootparse.h needs parsenodes.h to declare YYSTYPE, per
comments in bootscanner.l. Anyway, this seems a good change.
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.)
0003 here is your patch, which I include because of conflicts with my
0002. After my 0002, plannodes.h is pretty slim, so I'd be hesitant to
include pathnodes.h just to be able to change the Bitmapset * to Relids.
But on the other hand, it doesn't seem to have too bad an effect overall
(if only because plannodes.h is included by rather few files), so +0.1
on doing this. 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.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
