Re: Unused header file inclusion

Hi,

On 2019-07-31 19:25:01 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2019-07-31 16:55:31 -0400, Tom Lane wrote:
> >> Yeah. I seem to recall a proposal that nodes.h should contain
> >>
> >> typedef struct Foo Foo;
> >>
> >> for every node type Foo, and then the other headers would just
> >> fill in the structs, and we could get rid of a lot of ad-hoc
> >> forward struct declarations and other hackery.
>
> > That to me just seems objectively worse. Now adding a new struct as a
> > minor implementation detail of some subsystem doesn't just require
> > recompiling the relevant files, but just about all of pg.
>
> Er, what? This list of typedefs would change exactly when enum NodeTag
> changes, so AFAICS your objection is bogus.

> It's true that this proposal doesn't help for structs that aren't Nodes,
> but my sense is that > 90% of our ad-hoc struct references are for Nodes.

Ah, well, I somehow assumed you were talking about all nodes. I don't
think I agree with the 90% figure. In headers I feel like most the
references are to things like Relation, Snapshot, HeapTuple, etc.

> > Right now we really have weird dependencies between largely independent
> > subsystem.
>
> Agreed, but I think fixing that will take some actually serious design
> work. It's not going to mechanically fall out of changing typedef rules.

No, but without finding a more workable approach than what we're doing
often doing now wrt includes and forward declares, we'll have a lot
harder time to separate subsystems out more.

> > The only reason the explicit forward declaration is needed in the common
> > case of a 'struct foo*' parameter is that C has weird visibility rules
> > about the scope of forward declarations in paramters.
>
> Yeah, but there's not much we can do about that, nor is getting rid
> of typedefs in favor of "struct" going to make it even a little bit
> better.

It imo pretty fundamentally does. You cannot redefine typedefs, but you
can forward declare structs.

E.g. in the attached series of patches, I'm removing a good portion of
unnecessary dependencies to fmgr.h. But to actually make a difference
that requires referencing two structs without including the header - and
I don't think restructing fmgr.h into two headers is a particularly
attractive alternative (would make it a lot more work and a lot more
invasive).

Think the first three are pretty clearly a good idea, I'm a bit less
sanguine about the fourth:
Headers like utils/timestamp.h are often included just because we need a
TimestampTz type somewhere, or call GetCurrentTimestamp(). Approximately
none of these need the PG_GETARG_* macros, which are the only reason for
including fmgr.h in these headers. As they're macros that's not
actually needed, although I think normally good style. But I' think here
avoiding exposing fmgr.h to more headers is a bigger win.

Greetings,

Andres Freund