Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 1/17/17 2:35 PM, Tom Lane wrote:
>> BTW, now that I've looked through this patch ... why does it add
>>
>> +#include "nodes/nodes.h"
>> +#include "nodes/pg_list.h"
>>
>> to utils/builtins.h?
> There are things in builtins.h that technically need those declarations.
> It seems to work without it now, but at some point during the
> refactoring they were necessary. I can remove them again.
[ looks around ... ] Hm, it looks like the reason it works without
them is that utils/sortsupport.h pulls in relcache.h, which pulls in a
veritable buttload of stuff; those two and a whole lot more beside.
That seems like a pretty bad idea. It's surely not the fault of this
patch that the sortsupport patch didn't give a damn about inclusion
footprints; but I still think we'd be well advised to minimize
builtins.h's footprint now that it's going to need to be included very
far and wide.
It looks like we could get rid of the need for sortsupport.h in
builtins.h via the good old "struct foo" forward reference trick,
that is
struct SortSupportData;
extern void varstr_sortsupport(struct SortSupportData *ssup, Oid collid, bool bpchar);
but I wonder if that's going far enough. We'd still be propagating
pg_list.h into a lot of places that don't necessarily need it.
Alternatively ... is there a specific reason why you chose to make
builtins.h the key inclusion file for this change, rather than having
callers include fmgrprotos.h directly? It seems like the stuff remaining
in builtins.h is just a laundry list of random utility functions.
Maybe dispersing those to other headers is the thing to do.
regards, tom lane
