Re: pgsql: Remove pqsignal() from libpq's official exports list.
Re: Tom Lane 2018-09-28 > 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 mha@xindi:~$ psql 04 11:41 /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.0 + +++ /tmp/autopkgtest-lxc.evololie/downtmp/build.XvF/src/regress/results/repack-run.out 2019-10-03 21:24:29.049576631 + @@ -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
Re: pgsql: Remove pqsignal() from libpq's official exports list.
On Fri, Oct 04, 2019 at 11:56:31AM +0200, Christoph Berg wrote: > This is starting to hurt in several places: > > 04 11:41 mha@xindi:~$ psql > 04 11:41 /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 signature.asc Description: PGP signature
Re: pgsql: Remove pqsignal() from libpq's official exports list.
Christoph Berg writes: > Re: Tom Lane 2018-09-28 >> Remove pqsignal() from libpq's official exports list. > This is starting to hurt in several places: > 04 11:41 mha@xindi:~$ psql > 04 11:41 /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
Re: pgsql: Remove pqsignal() from libpq's official exports list.
Re: Tom Lane 2019-10-08 <9333.1570566...@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
Re: pgsql: Remove pqsignal() from libpq's official exports list.
Greetings, * Christoph Berg (m...@debian.org) wrote: > Re: Tom Lane 2019-10-08 <9333.1570566...@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 signature.asc Description: PGP signature
Re: pgsql: Remove pqsignal() from libpq's official exports list.
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.1565550...@sss.pgh.pa.us -- Michael signature.asc Description: PGP signature
Re: pgsql: Remove pqsignal() from libpq's official exports list.
Stephen Frost 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
Re: pgsql: Remove pqsignal() from libpq's official exports list.
Re: Tom Lane 2019-10-10 <10247.1570731...@sss.pgh.pa.us> > OK, done. Thanks, that made quite a few QA pipeline jobs happy here. Christoph