Let's stop with the retail rebuilds of src/port/ files already

Hi,

On 2018-09-26 19:10:40 -0400, Tom Lane wrote:

I'm getting tired of having to make fixes like ce4887bd0. I think
we should rearrange things so that src/port/ and src/common/ compile
all their files a third time using shared-library-friendly switches,
put them into new .a files, and have libpq and the ecpg libraries
just include those libraries instead of what they're doing now.

This would result in compiling some of the port+common files uselessly,
since they'd never actually get pulled in by any shared library.
But I think we're approaching the point where we might have a net savings
of build time anyway, due to not having to compile the same files multiple
times in different subdirectories. And it'd sure be a savings of
developer brain-cells and sanity.

+1

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2018-09-26 19:10:40 -0400, Tom Lane wrote:

I'm getting tired of having to make fixes like ce4887bd0. I think
we should rearrange things so that src/port/ and src/common/ compile
all their files a third time using shared-library-friendly switches,
put them into new .a files, and have libpq and the ecpg libraries
just include those libraries instead of what they're doing now.

+1

Here's a partial patch for that: it adds the third build variant
to src/port/ and teaches libpq to use it. We'd want to likewise
modify src/common/ and fix up other callers such as ecpg, but this
seems to be enough to test whether the idea works or not.

I've tried this on Linux, macOS and HPUX and it seems to work in
those cases, but I'm not foolish enough to imagine that that's
exhaustive.

What I think would make sense is to push this and see what the
buildfarm thinks of it. If there are unfixable problems then
we won't have wasted time fleshing out the concept. Otherwise,
I'll do the remaining pieces.

regards, tom lane

Hi,

On September 26, 2018 9:03:05 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

On 2018-09-26 19:10:40 -0400, Tom Lane wrote:

I'm getting tired of having to make fixes like ce4887bd0. I think
we should rearrange things so that src/port/ and src/common/ compile
all their files a third time using shared-library-friendly switches,
put them into new .a files, and have libpq and the ecpg libraries
just include those libraries instead of what they're doing now.

+1

Here's a partial patch for that: it adds the third build variant
to src/port/ and teaches libpq to use it. We'd want to likewise
modify src/common/ and fix up other callers such as ecpg, but this
seems to be enough to test whether the idea works or not.

I've tried this on Linux, macOS and HPUX and it seems to work in
those cases, but I'm not foolish enough to imagine that that's
exhaustive.

What I think would make sense is to push this and see what the
buildfarm thinks of it. If there are unfixable problems then
we won't have wasted time fleshing out the concept. Otherwise,
I'll do the remaining pieces.

Sounds reasonable to me.

Medium-long term I think we should consider trying to reduce the duplication tho. Once we provide an elog and error handling wrapper, we really should be able to reduce duplication (code and build) a fair bit. But that should be tackled separately.

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On Wed, Sep 26, 2018 at 09:24:48PM -0700, Andres Freund wrote:

On September 26, 2018 9:03:05 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

What I think would make sense is to push this and see what the
buildfarm thinks of it. If there are unfixable problems then
we won't have wasted time fleshing out the concept. Otherwise,
I'll do the remaining pieces.

Sounds reasonable to me.

# libpgport is needed by some contrib
+# currently we don't install libpgport_shlib.a, maybe we should?

Likely you should as this could be used directly by out-of-core things.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On September 26, 2018 9:03:05 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

# libpgport is needed by some contrib
+# currently we don't install libpgport_shlib.a, maybe we should?

Likely you should as this could be used directly by out-of-core things.

Maybe, but what things exactly? Extension modules don't need it, as
they just call the versions built into the core backend.

regards, tom lane

I wrote:

Here's a partial patch for that: it adds the third build variant
to src/port/ and teaches libpq to use it. We'd want to likewise
modify src/common/ and fix up other callers such as ecpg, but this
seems to be enough to test whether the idea works or not.
...
What I think would make sense is to push this and see what the
buildfarm thinks of it. If there are unfixable problems then
we won't have wasted time fleshing out the concept. Otherwise,
I'll do the remaining pieces.

Well, the buildfarm did turn up one problem: on really old macOS
(cf prairiedog) the libpq link step fails with

ld: symbols names listed in -exported_symbols_list: exports.list not in linked objects
_pqsignal

Apparently, with that linker, the exported symbols list is resolved
against just what is found in the listed *.o files, not anything pulled
in from a library file.

Now, the question that raises in my mind is why is libpq.so exporting
pqsignal() at all? Probably there was once a reason for it, but nowadays
I would think that any client program using pqsignal() should get it
from -lpgport, not from libpq. Having more than one place to get it from
seems more likely to create issues than solve them. And we certainly
do not document it as a function available from libpq.

So my recommendation is to remove pqsignal from libpq's exports.txt.
I've verified that prairiedog builds happily with that change,
confirming my expectation that all consumers of the symbol can get it
from someplace else.

Now, if we go forward with that solution, there will be issues with
some other things that libpq exports without having defined itself:

src/backend/utils/mb/wchar.c:
pg_utf_mblen
src/backend/utils/mb/encnames.c:
pg_encoding_to_char
pg_char_to_encoding
pg_valid_server_encoding
pg_valid_server_encoding_id

Now, I'd already had my eye on those two files, because after applying a
similar fix for src/common/, those two files would be the only ones that
libpq needs to symlink from somewhere else.

What I was thinking of proposing was to move those two files out of the
backend and into src/common/, thereby normalizing their status as
modules available in both frontend and backend, and removing the need
for a special build rule for them in libpq. (initdb could be simplified
too.) Per this discovery, we'd need to also remove these symbols from
libpq's exports list, meaning that clients *must* get them from -lpgcommon
not from libpq.

There's a small chance that this'd break third-party clients that
are using these symbols out of libpq. We've never documented them
as being available, but somebody might be using them anyway.
If that does happen, it could be repaired by linking against -lpgcommon
along with libpq, but it'd possibly still make people unhappy.

Comments?

regards, tom lane

On 09/27/2018 03:48 PM, Tom Lane wrote:

I wrote:

Here's a partial patch for that: it adds the third build variant
to src/port/ and teaches libpq to use it. We'd want to likewise
modify src/common/ and fix up other callers such as ecpg, but this
seems to be enough to test whether the idea works or not.
...
What I think would make sense is to push this and see what the
buildfarm thinks of it. If there are unfixable problems then
we won't have wasted time fleshing out the concept. Otherwise,
I'll do the remaining pieces.

Well, the buildfarm did turn up one problem: on really old macOS
(cf prairiedog) the libpq link step fails with

ld: symbols names listed in -exported_symbols_list: exports.list not in linked objects
_pqsignal

Apparently, with that linker, the exported symbols list is resolved
against just what is found in the listed *.o files, not anything pulled
in from a library file.

Now, the question that raises in my mind is why is libpq.so exporting
pqsignal() at all? Probably there was once a reason for it, but nowadays
I would think that any client program using pqsignal() should get it
from -lpgport, not from libpq. Having more than one place to get it from
seems more likely to create issues than solve them. And we certainly
do not document it as a function available from libpq.

So my recommendation is to remove pqsignal from libpq's exports.txt.
I've verified that prairiedog builds happily with that change,
confirming my expectation that all consumers of the symbol can get it
from someplace else.

Now, if we go forward with that solution, there will be issues with
some other things that libpq exports without having defined itself:

src/backend/utils/mb/wchar.c:
pg_utf_mblen
src/backend/utils/mb/encnames.c:
pg_encoding_to_char
pg_char_to_encoding
pg_valid_server_encoding
pg_valid_server_encoding_id

Now, I'd already had my eye on those two files, because after applying a
similar fix for src/common/, those two files would be the only ones that
libpq needs to symlink from somewhere else.

What I was thinking of proposing was to move those two files out of the
backend and into src/common/, thereby normalizing their status as
modules available in both frontend and backend, and removing the need
for a special build rule for them in libpq. (initdb could be simplified
too.) Per this discovery, we'd need to also remove these symbols from
libpq's exports list, meaning that clients *must* get them from -lpgcommon
not from libpq.

There's a small chance that this'd break third-party clients that
are using these symbols out of libpq. We've never documented them
as being available, but somebody might be using them anyway.
If that does happen, it could be repaired by linking against -lpgcommon
along with libpq, but it'd possibly still make people unhappy.

Seems a small enough price to pay.

cheers

andrew

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

I wrote:

Now, if we go forward with that solution, there will be issues with
some other things that libpq exports without having defined itself:
src/backend/utils/mb/wchar.c:
pg_utf_mblen
src/backend/utils/mb/encnames.c:
pg_encoding_to_char
pg_char_to_encoding
pg_valid_server_encoding
pg_valid_server_encoding_id
What I was thinking of proposing was to move those two files out of the
backend and into src/common/, thereby normalizing their status as
modules available in both frontend and backend, and removing the need
for a special build rule for them in libpq. (initdb could be simplified
too.) Per this discovery, we'd need to also remove these symbols from
libpq's exports list, meaning that clients *must* get them from -lpgcommon
not from libpq.

After further study I've concluded that moving those two files would
be more neatnik-ism than is justified. While it'd get rid of the
symlink-a-source-file technique in libpq, there'd still be other
occurrences of that in our tree, so the actual cleanup benefit seems
pretty limited. And while I'm prepared to believe that nobody outside PG
uses pqsignal() or should do so, it's a little harder to make that case
for the encnames.c functions; so the risk of causing problems seems
noticeably greater.

Accordingly, I cleaned up the usage of the existing src/common/ files
but didn't move anything around. I plan to stop here unless the
buildfarm shows more issues.

regards, tom lane