Re: Proposed refactoring of planner header files

Hi,

On 2019-01-28 15:50:22 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-01-28 15:17:19 -0500, Tom Lane wrote:
> >> Since I was intentionally trying to limit what optimizer.h pulls in,
> >> and in particular not let it include relation.h, I needed an opaque
> >> typedef for PlannerInfo.  On the other hand relation.h also needs to
> >> typedef that.  I fixed that with a method that we've not used in our
> >> code AFAIK, but is really common in system headers: there's a #define
> >> symbol to remember whether we've created the typedef, and including
> >> both headers in either order will work fine.
> 
> > Ugh, isn't it nicer to just use the underlying struct type instead of
> > that?
> 
> No, because that'd mean that anyplace relying on optimizer.h would also
> have to write "struct PlannerInfo" rather than "PlannerInfo"; the
> effects wouldn't be limited to the header itself.

Why? It can be called with the typedef'd version, or not.  Unless it
calling code has on-stack pointers to it, it ought to never deal with
PlannerInfo vs struct PlannerInfo.  In a lot of cases the code calling
the function will either get the PlannerInfo from somewhere (in which
case that'll either have the typedef'd version or not).


> > Or alternatively we could expand our compiler demands to require
> > that redundant typedefs are allowed - I'm not sure there's any animal
> > left that doesn't support that (rather than triggering an error it via
> > an intentionally set flag).
> 
> I'd be happy with that if it actually works, but I strongly suspect
> that it does not.  Or can you cite chapter and verse where C99
> requires it to work?  My own compiler complains about "redefinition
> of typedef 'foo'".

It's not required by C99, it however is required by C11. But a lot of
compilers have allowed it as an extension for a long time (like before
C99), unless suppressed by some option. I think that's partially because
C++ has allowed it for longer.  I don't know how many of the BF
compilers could be made to accept that - I'd be very suprised if yours couldn't.


> >> I would have exposed estimate_rel_size, which is needed by
> >> access/hash/hash.c, except that it requires Relation and
> >> BlockNumber typedefs.  The incremental value from keeping
> >> hash.c from using plancat.h probably isn't worth widening
> >> optimizer.h's #include footprint further.  Also, I wonder
> >> whether that whole area needs a rethink for pluggable storage.
> 
> > What kind of rejiggering were you thinking of re pluggable storage?
> 
> I wasn't; I was just thinking that I didn't want to advertise it
> as a stable globally-accessible API if it's likely to get whacked
> around soon.

Ah. So far the signature didn't need to change, and I'm not aware of
pending issues requiring it.

Greetings,

Andres Freund