pgsql: Generate fmgr prototypes automatically

Peter Eisentraut <peter_e@gmx.net> writes:

Generate fmgr prototypes automatically

Buildfarm's quite unhappy with this. Did you check parallel make?

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Peter Eisentraut <peter_e@gmx.net> writes:

Generate fmgr prototypes automatically

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? AFAICS that shouldn't be necessary, since
there are no declarations in builtins.h that weren't there before.
And seeing that this results in builtins.h being included in even more
places than before, surely we want to minimize that file's inclusion
footprint.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On 1/17/17 2:35 PM, Tom Lane wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

Generate fmgr prototypes automatically

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? AFAICS that shouldn't be necessary, since
there are no declarations in builtins.h that weren't there before.
And seeing that this results in builtins.h being included in even more
places than before, surely we want to minimize that file's inclusion
footprint.

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.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

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

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Tom Lane wrote:

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.

It is possible to replace many occurrences of builtins.h with
fmgrprotos.h. I just tried this
git grep -l 'include.*utils/builtins.h' -- *.c | xargs perl -pi -e 's{utils/builtins.h}{utils/fmgrprotos.h}'
There's a large number of changes that the oneliner produces that must
be reverted for the compile to be silent, but a large portion can
remain. (I only tried src/backend/access).

Anyway I support the idea of creating other header files for specific
purposes, such as ruleutils.h, varlena.h, etc. (I am against creating a
header file that contains commonly used macros such as
CStringGetTextDatum together with varstr_sortsupport. I think the
latter should have its own place).

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera wrote:

It is possible to replace many occurrences of builtins.h with
fmgrprotos.h. I just tried this
git grep -l 'include.*utils/builtins.h' -- *.c | xargs perl -pi -e 's{utils/builtins.h}{utils/fmgrprotos.h}'
There's a large number of changes that the oneliner produces that must
be reverted for the compile to be silent, but a large portion can
remain. (I only tried src/backend/access).

92 files are changed, 241 files still require builtins.h.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Alvaro Herrera wrote:

It is possible to replace many occurrences of builtins.h with
fmgrprotos.h. I just tried this
git grep -l 'include.*utils/builtins.h' -- *.c | xargs perl -pi -e 's{utils/builtins.h}{utils/fmgrprotos.h}'
There's a large number of changes that the oneliner produces that must
be reverted for the compile to be silent, but a large portion can
remain. (I only tried src/backend/access).

92 files are changed, 241 files still require builtins.h.

Let's hold off on that till we decide on the bigger-picture plan here.
If we're going to try to move those functions out of builtins.h,
this result will change considerably.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Eisentraut <peter_e@gmx.net> writes:

Generate fmgr prototypes automatically

BTW, I notice some suspicious-looking behavior with -j:

$ make -j8 -s
Writing fmgroids.h
Writing fmgroids.h
Writing postgres.bki
Writing fmgrprotos.h
Writing fmgrtab.c
Writing schemapg.h
Writing postgres.description
Writing postgres.shdescription
Writing fmgrprotos.h
Writing fmgrtab.c
...

Why do fmgroids.h, fmgrprotos.h, and fmgrtab.c now get mentioned
twice? I suspect there is something broken about the parallelization.
If indeed multiple instances of gmake are writing these files
concurrently, that's likely to result in irreproducible build failures.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 1/17/17 3:07 PM, Tom Lane wrote:

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.

Here is a patch that moves two blocks from builtins.h into separate
header files. That avoids having to include nodes/pg_list.h and
utils/sortsupport.h.

The remaining inclusion of nodes/nodes.h is for the oidparse() function.
I think that could be moved out of oid.c and put somewhere near parser/
or objectaddress.c. But the practical savings from avoiding nodes.h is
probably near zero, so I haven't done anything about that here.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

Here is a patch that moves two blocks from builtins.h into separate
header files. That avoids having to include nodes/pg_list.h and
utils/sortsupport.h.

Seems like a reasonable solution.

The remaining inclusion of nodes/nodes.h is for the oidparse() function.
I think that could be moved out of oid.c and put somewhere near parser/
or objectaddress.c. But the practical savings from avoiding nodes.h is
probably near zero, so I haven't done anything about that here.

Agreed, not much value in that.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 1/18/17 7:38 AM, Tom Lane wrote:

Why do fmgroids.h, fmgrprotos.h, and fmgrtab.c now get mentioned
twice? I suspect there is something broken about the parallelization.
If indeed multiple instances of gmake are writing these files
concurrently, that's likely to result in irreproducible build failures.

I've pushed a fix for this.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 1/18/17 7:38 AM, Tom Lane wrote:

Why do fmgroids.h, fmgrprotos.h, and fmgrtab.c now get mentioned
twice? I suspect there is something broken about the parallelization.

I've pushed a fix for this.

Seems to fix the problem for me --- thanks!

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers