Horton

Thread

Commits

  1. Put back pqsignal() as an exported libpq symbol.

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

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

  4. Use SA_RESTART for all signals, including SIGALRM.

  5. Move pqsignal() to libpgport.

  1. pgsql: Remove pqsignal() from libpq's official exports list.

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-28T16:38:17Z

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

Client applications should get this function, if they need it, from
libpgport.

The fact that it's exported from libpq is a hack left over from before
we set up libpgport.  It's never been documented, and there's no good
reason for non-PG code to be calling it anyway, so hopefully this won't
cause any problems.  Moreover, with the previous setup it was not real
clear whether our clients that use the function were getting it from
libpgport or libpq, so this might actually prevent problems.

The reason for changing it now is that in the wake of commit ea53100d5,
some linkers won't export the symbol, apparently because it's coming from
a .a library instead of a .o file.  We could get around that by continuing
to symlink pqsignal.c into libpq as before; but unless somebody complains
very hard, I don't want to adopt such a kluge.

Discussion: https://postgr.es/m/13022.1538003440@sss.pgh.pa.us
Discussion: https://postgr.es/m/E1g5Y8r-0006vs-QA@gemulon.postgresql.org

Branch
------
master

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

Modified Files
--------------
src/interfaces/libpq/exports.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

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

    Christoph Berg <myon@debian.org> — 2019-10-04T09:56:31Z

    Re: Tom Lane 2018-09-28 <E1g5vmT-0003K1-6S@gemulon.postgresql.org>
> Remove pqsignal() from libpq's official exports list.
> 
> Client applications should get this function, if they need it, from
> libpgport.
> 
> The fact that it's exported from libpq is a hack left over from before
> we set up libpgport.  It's never been documented, and there's no good
> reason for non-PG code to be calling it anyway, so hopefully this won't
> cause any problems.  Moreover, with the previous setup it was not real
> clear whether our clients that use the function were getting it from
> libpgport or libpq, so this might actually prevent problems.
> 
> The reason for changing it now is that in the wake of commit ea53100d5,
> some linkers won't export the symbol, apparently because it's coming from
> a .a library instead of a .o file.  We could get around that by continuing
> to symlink pqsignal.c into libpq as before; but unless somebody complains
> very hard, I don't want to adopt such a kluge.

This is starting to hurt in several places:

04 11:41 <magnush> mha@xindi:~$ psql
04 11:41 <magnush> /usr/lib/postgresql/9.2/bin/psql: symbol lookup error:
                   /usr/lib/postgresql/9.2/bin/psql: undefined symbol: pqsignal

pg_repack linked against libpq5 11 breaks with libpq5 12:

--- /tmp/autopkgtest-lxc.evololie/downtmp/build.XvF/src/regress/expected/repack-run.out	2018-10-18 11:30:46.000000000 +0000
+++ /tmp/autopkgtest-lxc.evololie/downtmp/build.XvF/src/regress/results/repack-run.out	2019-10-03 21:24:29.049576631 +0000
@@ -2,19 +2,8 @@
 -- do repack
 --
 \! pg_repack --dbname=contrib_regression --table=tbl_cluster
-INFO: repacking table "public.tbl_cluster"
+pg_repack: symbol lookup error: pg_repack: undefined symbol: pqsignal

https://ci.debian.net/data/autopkgtest/testing/amd64/p/pg-repack/3070831/log.gz

Christoph

  3. Re: pgsql: Remove pqsignal() from libpq's official exports list.

    Michael Paquier <michael@paquier.xyz> — 2019-10-07T06:59:16Z

    On Fri, Oct 04, 2019 at 11:56:31AM +0200, Christoph Berg wrote:
> This is starting to hurt in several places:
> 
> 04 11:41 <magnush> mha@xindi:~$ psql
> 04 11:41 <magnush> /usr/lib/postgresql/9.2/bin/psql: symbol lookup error:
>                    /usr/lib/postgresql/9.2/bin/psql: undefined symbol: pqsignal
> 
> pg_repack linked against libpq5 11 breaks with libpq5 12:

Ouch.  So that's commit f7ab802.  I agree that this is not cool, and
libpq so version is not likely going to be bumped up (if that happens
I have code I could wipe out).  Could we reconsider this decision?  It
seems to me that we should not silently break things.
--
Michael

  4. Re: pgsql: Remove pqsignal() from libpq's official exports list.

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-08T20:25:05Z

    Christoph Berg <myon@debian.org> writes:
> Re: Tom Lane 2018-09-28 <E1g5vmT-0003K1-6S@gemulon.postgresql.org>
>> Remove pqsignal() from libpq's official exports list.

> This is starting to hurt in several places:
> 04 11:41 <magnush> mha@xindi:~$ psql
> 04 11:41 <magnush> /usr/lib/postgresql/9.2/bin/psql: symbol lookup error:
>                    /usr/lib/postgresql/9.2/bin/psql: undefined symbol: pqsignal

I poked into this a little.  Reviewing the commit history, pqsignal()
was a part of libpq (so far as frontend code is concerned) up until
9.3, when commit da5aeccf6 moved it into libpgport.  Since then we've
expected client programs to get it from libpgport not libpq, and indeed
they do so AFAICT --- I can reproduce the described failure with 9.2
and below psql linking to current libpq.so, but not with 9.3 and up.

libpq itself indeed has no need for pqsignal at all, if
--enable-thread-safety is set, which it usually is these days.

I also notice that we've made at least one nontrivial semantics change
to pqsignal: commit 873ab9721 made it use SA_RESTART for SIGALRM
handlers, which it did not do before 9.3.  So really, none of the
post-9.2 versions of libpq have faithfully duplicated what an older
client would expect from pqsignal.  This isn't at all academic, because
I see that pgbench uses pqsignal(SIGALRM,...), and so does pg_test_fsync.
Now, I don't see any indication that we've adjusted either of those
programs for the different behavior, so maybe it's fine.  But we've been
treating this function as strictly internal, and so I'm not pleased with
the idea of putting it back into the exported symbol list.

I'm especially not pleased with doing so to support pre-9.3 client
programs.  Those have been below our support horizon for some time;
notably, they (presumably) haven't been patched for CVE-2018-1058.
Why are you still shipping them in current OS releases?

> pg_repack linked against libpq5 11 breaks with libpq5 12:

This probably means it needs to be linked with libpgport
not only libpq.

Having said all that, if we conclude we can't break compatibility
with this legacy code quite yet, I'd be inclined to put a
separate, clearly-marked-as-legacy-code version of pqsignal()
back into libpq, using the pre-9.3 SA_RESTART semantics.
But I'd like some pretty well-defined sunset time for that,
because it'd just be trouble waiting to happen.  When are
you going to remove 9.2 psql?

			regards, tom lane

  5. Re: pgsql: Remove pqsignal() from libpq's official exports list.

    Christoph Berg <myon@debian.org> — 2019-10-09T07:33:54Z

    Re: Tom Lane 2019-10-08 <9333.1570566305@sss.pgh.pa.us>
> Having said all that, if we conclude we can't break compatibility
> with this legacy code quite yet, I'd be inclined to put a
> separate, clearly-marked-as-legacy-code version of pqsignal()
> back into libpq, using the pre-9.3 SA_RESTART semantics.

That would be nice.

> But I'd like some pretty well-defined sunset time for that,
> because it'd just be trouble waiting to happen.  When are
> you going to remove 9.2 psql?

Note that this change caused breakage on the wiki.postgresql.org
infrastructure which still had an old 9.2 psql running. It wasn't
Debian's fault that it had not been upgraded yet.

But I refuse to buy the argument that I'm doing something wrong here.
Shared libraries have SONAMEs to prevent *exactly* this kind of
breakage. If you are removing symbols, bump the SONAME.

Christoph

  6. Re: pgsql: Remove pqsignal() from libpq's official exports list.

    Stephen Frost <sfrost@snowman.net> — 2019-10-09T13:37:34Z

    Greetings,

* Christoph Berg (myon@debian.org) wrote:
> Re: Tom Lane 2019-10-08 <9333.1570566305@sss.pgh.pa.us>
> > Having said all that, if we conclude we can't break compatibility
> > with this legacy code quite yet, I'd be inclined to put a
> > separate, clearly-marked-as-legacy-code version of pqsignal()
> > back into libpq, using the pre-9.3 SA_RESTART semantics.
> 
> That would be nice.
> 
> > But I'd like some pretty well-defined sunset time for that,
> > because it'd just be trouble waiting to happen.  When are
> > you going to remove 9.2 psql?
> 
> Note that this change caused breakage on the wiki.postgresql.org
> infrastructure which still had an old 9.2 psql running. It wasn't
> Debian's fault that it had not been upgraded yet.
> 
> But I refuse to buy the argument that I'm doing something wrong here.
> Shared libraries have SONAMEs to prevent *exactly* this kind of
> breakage. If you are removing symbols, bump the SONAME.

Yes, this is absolutely the right answer, we shouldn't be removing
symbols without an SONAME bump.  If we don't want to bump the SONAME,
then don't remove the symbol.  This is utterly basic proper library
maintenance and it isn't appropriate to argue that it's about "well,
your old apps shouldn't exist" because it's blatently our fault for not
bumping the SONAME, no matter how old the apps are.

Thanks,

Stephen

  7. Re: pgsql: Remove pqsignal() from libpq's official exports list.

    Michael Paquier <michael@paquier.xyz> — 2019-10-10T03:56:35Z

    On Wed, Oct 09, 2019 at 09:37:34AM -0400, Stephen Frost wrote:
> Yes, this is absolutely the right answer, we shouldn't be removing
> symbols without an SONAME bump.  If we don't want to bump the SONAME,
> then don't remove the symbol.  This is utterly basic proper library
> maintenance and it isn't appropriate to argue that it's about "well,
> your old apps shouldn't exist" because it's blatently our fault for not
> bumping the SONAME, no matter how old the apps are.

+1.  If we were to bump the SONAME, more cleanup could be actually
done, and I got some pushback not long ago regarding some undocumented
APIs in libpq that we still ship for the exact same reasons:
https://www.postgresql.org/message-id/7990.1565550764@sss.pgh.pa.us
--
Michael

  8. Re: pgsql: Remove pqsignal() from libpq's official exports list.

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-10T18:26:09Z

    Stephen Frost <sfrost@snowman.net> writes:
> Yes, this is absolutely the right answer, we shouldn't be removing
> symbols without an SONAME bump.  If we don't want to bump the SONAME,
> then don't remove the symbol.

OK, done.

			regards, tom lane

  9. Re: pgsql: Remove pqsignal() from libpq's official exports list.

    Christoph Berg <myon@debian.org> — 2019-10-11T18:54:20Z

    Re: Tom Lane 2019-10-10 <10247.1570731969@sss.pgh.pa.us>
> OK, done.

Thanks, that made quite a few QA pipeline jobs happy here.

Christoph