Re: Relids instead of Bitmapset * in plannode.h

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Cc: Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Relids instead of Bitmapset * in plannode.h
Date: 2023-11-07 15:24:32
Message-ID: 122451.1699370672@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2023-11-07 15:26:42 Re: Protocol question regarding Portal vs Cursor
Previous Message Dean Rasheed 2023-11-07 15:10:10 MERGE: AFTER ROW trigger failure for cross-partition update