Horton

Thread

Commits

  1. Build src/common files as a library with -fPIC.

  2. Remove pqsignal() from libpq's official exports list.

  3. Build src/port files as a library with -fPIC, and use that in libpq.

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-26T23:10:40Z

    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.

Maybe use the extension "_shlib" (vs "_srv") for these .o and .a files.

Thoughts?

			regards, tom lane

  2. Re: Let's stop with the retail rebuilds of src/port/ files already

    Andres Freund <andres@anarazel.de> — 2018-09-26T23:13:31Z

    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

  3. Re: Let's stop with the retail rebuilds of src/port/ files already

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-27T04:03:05Z

    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

  4. Re: Let's stop with the retail rebuilds of src/port/ files already

    Andres Freund <andres@anarazel.de> — 2018-09-27T04:24:48Z

    
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.

  5. Re: Let's stop with the retail rebuilds of src/port/ files already

    Michael Paquier <michael@paquier.xyz> — 2018-09-27T05:10:59Z

    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

  6. Re: Let's stop with the retail rebuilds of src/port/ files already

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-27T14:17:33Z

    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

  7. Re: Let's stop with the retail rebuilds of src/port/ files already

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-27T19:48:36Z

    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

  8. Re: Let's stop with the retail rebuilds of src/port/ files already

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-09-28T14:39:58Z

    
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

  9. Re: Let's stop with the retail rebuilds of src/port/ files already

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-28T18:59:58Z

    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