From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Relids instead of Bitmapset * in plannode.h
In-reply-to: <202311071106.6y7b2ascqjlz@alvherre.pgsql>
References: <202311071106.6y7b2ascqjlz@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@alvh.no-ip.org>
	message dated "Tue, 07 Nov 2023 12:06:28 +0100"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <122450.1699370672.1@sss.pgh.pa.us>
Date: Tue, 07 Nov 2023 10:24:32 -0500
Message-ID: <122451.1699370672@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/122451.1699370672%40sss.pgh.pa.us>
Precedence: bulk

Alvaro Herrera <alvherre@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