pgsql: Build src/port files as a library with -fPIC, and use that in li

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

libpq and ecpg need shared-library-friendly versions of assorted src/port/
and src/common/ modules. Up to now, they got those by symlinking the
individual source files and compiling them locally. That's baroque, and a
pain to maintain, and it results in some amount of duplicated compile work.
It might've made sense when only a couple of files were needed, but the
list has grown and grown and grown :-(

It makes more sense to have the originating directory build a third variant
of libpgport.a/libpgcommon.a containing modules built with $(CFLAGS_SL),
and just link that into the shared library. Unused files won't get linked,
so the end result should be the same.

This patch makes a down payment on that idea by having src/port/ build
such a library and making libpq use it. If the buildfarm doesn't expose
fatal problems with the approach, I'll extend it to the other cases.

Discussion: https://postgr.es/m/13022.1538003440@sss.pgh.pa.us

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/ea53100d5671b5b243f77898b0b04d23c38b2820

Modified Files
--------------
src/interfaces/libpq/.gitignore | 21 ----------------
src/interfaces/libpq/Makefile | 41 +++++++++---------------------
src/port/.gitignore | 1 +
src/port/Makefile | 55 ++++++++++++++++++++++++++++++++---------
4 files changed, 55 insertions(+), 63 deletions(-)

Re: Tom Lane 2018-09-27 <E1g5Y8r-0006vs-QA(at)gemulon(dot)postgresql(dot)org>
> Build src/port files as a library with -fPIC, and use that in libpq.

This made the "pqsignal" symbol disappear from libpq5.so:

13:27:55 dpkg-gensymbols: error: some symbols or patterns disappeared in the symbols file: see diff output below
13:27:55 dpkg-gensymbols: warning: debian/libpq5/DEBIAN/symbols doesn't match completely debian/libpq5.symbols
13:27:55 --- debian/libpq5.symbols (libpq5_12~~devel~20180928.1058-1~226.git92a0342.pgdg+1_amd64)
13:27:55 +++ dpkg-gensymbolsoXZn54 2018-09-28 11:27:55.499157237 +0000
13:27:55 @@ -168,7 +168,7 @@
13:27:55 pg_valid_server_encoding(at)Base 0
13:27:55 pg_valid_server_encoding_id(at)Base 8.3~beta1-2~
13:27:55 pgresStatus(at)Base 0
13:27:55 - pqsignal(at)Base 0
13:27:55 +#MISSING: 12~~devel~20180928.1058-1~226.git92a0342.pgdg+1# pqsignal(at)Base 0
13:27:55 printfPQExpBuffer(at)Base 0
13:27:55 resetPQExpBuffer(at)Base 0
13:27:55 termPQExpBuffer(at)Base 0

Is this is a problem for libpq5 users?

Christoph

Christoph Berg <myon(at)debian(dot)org> writes:
> Re: Tom Lane 2018-09-27 <E1g5Y8r-0006vs-QA(at)gemulon(dot)postgresql(dot)org>
>> Build src/port files as a library with -fPIC, and use that in libpq.

> This made the "pqsignal" symbol disappear from libpq5.so:

Oh, interesting. I'd seen an actual error on prairiedog, but apparently
some other linkers just silently omit the export, if the symbol is in
a .a file rather than .o.

> Is this is a problem for libpq5 users?

I proposed in
https://www.postgresql.org/message-id/19581.1538077716@sss.pgh.pa.us

that we should remove pqsignal, as well as
pg_utf_mblen
pg_encoding_to_char
pg_char_to_encoding
pg_valid_server_encoding
pg_valid_server_encoding_id

from libpq's exports, on the grounds that (a) nobody should be using
those (they're undocumented as exports), and (b) anybody who is using
them should get them from libpgport/libpgcommon instead. Thoughts?

regards, tom lane

Re: Tom Lane 2018-09-28 <19404(dot)1538140436(at)sss(dot)pgh(dot)pa(dot)us>
> > Is this is a problem for libpq5 users?
>
> I proposed in
> https://www.postgresql.org/message-id/19581.1538077716@sss.pgh.pa.us
>
> that we should remove pqsignal, as well as
> pg_utf_mblen
> pg_encoding_to_char
> pg_char_to_encoding
> pg_valid_server_encoding
> pg_valid_server_encoding_id
>
> from libpq's exports, on the grounds that (a) nobody should be using
> those (they're undocumented as exports), and (b) anybody who is using
> them should get them from libpgport/libpgcommon instead. Thoughts?

I'm fine with that if we say (a) should be true, and even if it is
not, (b) is an easy workaround. Bumping the libpq SONAME just because
of this seems excessive.

On the Debian side, I can simply remove the symbol from the tracking
file and the buildsystem will be happy again.

Christoph

Christoph Berg <myon(at)debian(dot)org> writes:
> Re: Tom Lane 2018-09-28 <19404(dot)1538140436(at)sss(dot)pgh(dot)pa(dot)us>
>> I proposed in
>> https://www.postgresql.org/message-id/19581.1538077716@sss.pgh.pa.us
>>
>> that we should remove pqsignal, as well as
>> pg_utf_mblen
>> pg_encoding_to_char
>> pg_char_to_encoding
>> pg_valid_server_encoding
>> pg_valid_server_encoding_id
>>
>> from libpq's exports, on the grounds that (a) nobody should be using
>> those (they're undocumented as exports), and (b) anybody who is using
>> them should get them from libpgport/libpgcommon instead. Thoughts?

> I'm fine with that if we say (a) should be true, and even if it is
> not, (b) is an easy workaround. Bumping the libpq SONAME just because
> of this seems excessive.

Yeah, if anyone insists that this requires a soname bump, I'd probably
look for another solution. Making the makefiles a bit cleaner is not
worth the churn that would cause, IMO.

The place where we'd probably end up if anyone's unhappy about this
is that we'd still be symlinking the three files pqsignal.c, wchar.c,
and encnames.c into libpq. That's not very desirable, but at least
it'd be a fixed list rather than something we're continually having
to change.

regards, tom lane

On 09/27/2018 11:23 AM, Tom Lane wrote:
> Build src/port files as a library with -fPIC, and use that in libpq.
>
> libpq and ecpg need shared-library-friendly versions of assorted src/port/
> and src/common/ modules. Up to now, they got those by symlinking the
> individual source files and compiling them locally. That's baroque, and a
> pain to maintain, and it results in some amount of duplicated compile work.
> It might've made sense when only a couple of files were needed, but the
> list has grown and grown and grown :-(
>
> It makes more sense to have the originating directory build a third variant
> of libpgport.a/libpgcommon.a containing modules built with $(CFLAGS_SL),
> and just link that into the shared library. Unused files won't get linked,
> so the end result should be the same.
>
> This patch makes a down payment on that idea by having src/port/ build
> such a library and making libpq use it. If the buildfarm doesn't expose
> fatal problems with the approach, I'll extend it to the other cases.
>

Looks like we have a bunch of failures, including prairiedog.

cheers

andrew

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

Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> writes:
> Looks like we have a bunch of failures, including prairiedog.

Yeah, please see discussion, in this thread and
also at
https://www.postgresql.org/message-id/19581.1538077716@sss.pgh.pa.us

regards, tom lane

Re: Tom Lane 2018-09-28 <20671(dot)1538142538(at)sss(dot)pgh(dot)pa(dot)us>
> >> I proposed in
> >> https://www.postgresql.org/message-id/19581.1538077716@sss.pgh.pa.us
> >>
> >> that we should remove pqsignal, as well as
> >> pg_utf_mblen
> >> pg_encoding_to_char
> >> pg_char_to_encoding
> >> pg_valid_server_encoding
> >> pg_valid_server_encoding_id
> >>
> >> from libpq's exports, on the grounds that (a) nobody should be using
> >> those (they're undocumented as exports), and (b) anybody who is using
> >> them should get them from libpgport/libpgcommon instead. Thoughts?
>
> > I'm fine with that if we say (a) should be true, and even if it is
> > not, (b) is an easy workaround. Bumping the libpq SONAME just because
> > of this seems excessive.
>
> Yeah, if anyone insists that this requires a soname bump, I'd probably
> look for another solution. Making the makefiles a bit cleaner is not
> worth the churn that would cause, IMO.

It took a while to notice, but this change does break 8.2's initdb if
libpq5 from PG12 is installed:

$ /usr/lib/postgresql/8.2/bin/initdb
/usr/lib/postgresql/8.2/bin/initdb: symbol lookup error: /usr/lib/postgresql/8.2/bin/initdb: undefined symbol: pqsignal

postgres 8.2 itself seems to work fine.

Christoph

Christoph Berg <myon(at)debian(dot)org> writes:
> Re: Tom Lane 2018-09-28 <20671(dot)1538142538(at)sss(dot)pgh(dot)pa(dot)us>
>> I proposed in
>> https://www.postgresql.org/message-id/19581.1538077716@sss.pgh.pa.us
>> that we should remove pqsignal, as well as
>> pg_utf_mblen
>> pg_encoding_to_char
>> pg_char_to_encoding
>> pg_valid_server_encoding
>> pg_valid_server_encoding_id
>>
>> from libpq's exports, on the grounds that (a) nobody should be using
>> those (they're undocumented as exports), and (b) anybody who is using
>> them should get them from libpgport/libpgcommon instead. Thoughts?

> It took a while to notice, but this change does break 8.2's initdb if
> libpq5 from PG12 is installed:
> $ /usr/lib/postgresql/8.2/bin/initdb
> /usr/lib/postgresql/8.2/bin/initdb: symbol lookup error: /usr/lib/postgresql/8.2/bin/initdb: undefined symbol: pqsignal

Well, 8.2 is *very* long out of support, and there are plenty of
nasty bugs you're at risk of if you keep using it. So I don't
find this to be a good argument for contorting what we do in v12.

If you really want to keep using 8.2 (and even make new installations
with it!?), you could back-patch the 8.3 patch that made sure that
initdb didn't absorb pqsignal, pg_encoding_to_char, etc from libpq.
It looks like what you'd need is a portion of the Makefile changes
from 8468146b03c87f86162cee62e0de25f6e2d24b56.

BTW, I noticed this in that patch's commit message:

Going forward, we want to fix things so that encoding IDs can be changed
without an ABI break, and this commit includes the changes needed to allow
libpq's encoding IDs to be treated as fully independent of the backend's.
The main issue is that libpq clients should not include pg_wchar.h or
otherwise assume they know the specific values of libpq's encoding IDs,
since they might encounter version skew between pg_wchar.h and the libpq.so
they are using. To fix, have libpq officially export functions needed for
encoding name<=>ID conversion and validity checking; it was doing this
anyway unofficially.

So it was wrong of me to propose moving pg_encoding_to_char() et al
into libpgcommon: doing so would've re-introduced the hazard of
client code misinterpreting the encoding IDs returned by
PQclientEncoding() (and how the heck did I miss that libpq.sgml
does document that function for exactly that purpose?).

Fortunately, I didn't do that ...

regards, tom lane

Re: Tom Lane 2019-01-29 <27653(dot)1548774621(at)sss(dot)pgh(dot)pa(dot)us>
> > It took a while to notice, but this change does break 8.2's initdb if
> > libpq5 from PG12 is installed:
> > $ /usr/lib/postgresql/8.2/bin/initdb
> > /usr/lib/postgresql/8.2/bin/initdb: symbol lookup error: /usr/lib/postgresql/8.2/bin/initdb: undefined symbol: pqsignal
>
> Well, 8.2 is *very* long out of support, and there are plenty of
> nasty bugs you're at risk of if you keep using it. So I don't
> find this to be a good argument for contorting what we do in v12.

I try to keep the old versions alive on apt.pg.o for Debian unstable (only)
so I have something to grab when customers ask questions about the old
versions. We are probably lucky that 8.2 broke only now, and leaving
it broken is a fair thing. I was just mentioning for completeness, I
didn't mean to insist on fixing it.

> If you really want to keep using 8.2 (and even make new installations
> with it!?), you could back-patch the 8.3 patch that made sure that
> initdb didn't absorb pqsignal, pg_encoding_to_char, etc from libpq.
> It looks like what you'd need is a portion of the Makefile changes
> from 8468146b03c87f86162cee62e0de25f6e2d24b56.

I might give that a shot, but if it's too hard, dropping 8.2 "support"
is not a problem. Thanks for digging up the commit hash.

Christoph

On Tue, Jan 29, 2019 at 10:10 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Well, 8.2 is *very* long out of support, and there are plenty of
> nasty bugs you're at risk of if you keep using it. So I don't
> find this to be a good argument for contorting what we do in v12.

Isn't the issue more a matter of whether it's OK to break
compatibility without changing SO_*_VERSION?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Jan 29, 2019 at 10:10 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Well, 8.2 is *very* long out of support, and there are plenty of
>> nasty bugs you're at risk of if you keep using it. So I don't
>> find this to be a good argument for contorting what we do in v12.

> Isn't the issue more a matter of whether it's OK to break
> compatibility without changing SO_*_VERSION?

We're breaking compatibility only if you regard pqsignal() as
part of the advertised API, which I don't.

regards, tom lane

On Thu, Jan 31, 2019 at 2:50 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > Isn't the issue more a matter of whether it's OK to break
> > compatibility without changing SO_*_VERSION?
>
> We're breaking compatibility only if you regard pqsignal() as
> part of the advertised API, which I don't.

Well, I would agree with that if it weren't the case that binaries we
ship apparently use that API. If we say that the libraries are
API-compatible, but our own binaries don't work, our veracity is
questionable.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Jan 31, 2019 at 2:50 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> We're breaking compatibility only if you regard pqsignal() as
>> part of the advertised API, which I don't.

> Well, I would agree with that if it weren't the case that binaries we
> ship apparently use that API. If we say that the libraries are
> API-compatible, but our own binaries don't work, our veracity is
> questionable.

initdb hasn't depended on those libpq exports since 8.2, and it was
a bug that it did so even then.

regards, tom lane

Re: Tom Lane 2019-01-31 <12792(dot)1548965044(at)sss(dot)pgh(dot)pa(dot)us>
> initdb hasn't depended on those libpq exports since 8.2, and it was
> a bug that it did so even then.

9.2 psql (and createuser, ...) is also broken:

/usr/lib/postgresql/9.2/bin/psql: symbol lookup error: /usr/lib/postgresql/9.2/bin/psql: undefined symbol: pqsignal

Could we at least put a compat symbol back?

Christoph