Re: Proposed refactoring of planner header files

Andres Freund <andres(at)anarazel(dot)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.

> 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'".

>> I'm really unhappy that force_parallel_mode and
>> parallel_leader_participation are being treated as planner GUCs.
>> They are not that, IMO, because they also affect the behavior of
>> the executor, cf HandleParallelMessage, ExecGather, ExecGatherMerge.
>> This is somewhere between ill-considered and outright broken: what
>> happens if the values change between planning and execution?

> Stylistically I agree with that (and it's what I ended up doing for JIT
> compilation as well), but do you see concrete harm here?

Well, I don't know: I haven't tried to trace the actual effects if
the values change. Seems like they could be pretty bad, in the worst
case if this wasn't thought through, which it looks like it wasn't.

In any case, if these settings are somehow global rather than being
genuinely planner GUCs, then the planner shouldn't be responsible
for defining them ... maybe they belong in miscadmin.h?

>> 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.

regards, tom lane