pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Remove pqsignal() from libpq's official exports list.
9+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2019-10-10 06a367c382 7ed1bcaed6 Put back pqsignal() as an exported libpq symbol.
* pgsql: Remove pqsignal() from libpq's official exports list.
@ 2018-09-28 16:38 Tom Lane <tgl@sss.pgh.pa.us>
2019-10-04 09:56 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Christoph Berg <myon@debian.org>
0 siblings, 1 reply; 9+ messages in thread
From: Tom Lane @ 2018-09-28 16:38 UTC (permalink / raw)
To: pgsql-committers@lists.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.
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(-)
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pgsql: Remove pqsignal() from libpq's official exports list.
2018-09-28 16:38 pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
@ 2019-10-04 09:56 ` Christoph Berg <myon@debian.org>
2019-10-07 06:59 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Michael Paquier <michael@paquier.xyz>
2019-10-08 20:25 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 9+ messages in thread
From: Christoph Berg @ 2019-10-04 09:56 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
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:
inline patch 1: /tmp/autopkgtest-lxc.evololie/downtmp/build.XvF/src/regress/results/repack-run.out (+1/-1 lines) download inline-1.patch
--- /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
* Re: pgsql: Remove pqsignal() from libpq's official exports list.
2018-09-28 16:38 pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
2019-10-04 09:56 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Christoph Berg <myon@debian.org>
@ 2019-10-07 06:59 ` Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 9+ messages in thread
From: Michael Paquier @ 2019-10-07 06:59 UTC (permalink / raw)
To: Christoph Berg <myon@debian.org>; Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pgsql: Remove pqsignal() from libpq's official exports list.
2018-09-28 16:38 pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
2019-10-04 09:56 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Christoph Berg <myon@debian.org>
@ 2019-10-08 20:25 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-10-09 07:33 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Christoph Berg <myon@debian.org>
1 sibling, 1 reply; 9+ messages in thread
From: Tom Lane @ 2019-10-08 20:25 UTC (permalink / raw)
To: Christoph Berg <myon@debian.org>; +Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pgsql: Remove pqsignal() from libpq's official exports list.
2018-09-28 16:38 pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
2019-10-04 09:56 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Christoph Berg <myon@debian.org>
2019-10-08 20:25 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
@ 2019-10-09 07:33 ` Christoph Berg <myon@debian.org>
2019-10-09 13:37 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Stephen Frost <sfrost@snowman.net>
0 siblings, 1 reply; 9+ messages in thread
From: Christoph Berg @ 2019-10-09 07:33 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pgsql: Remove pqsignal() from libpq's official exports list.
2018-09-28 16:38 pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
2019-10-04 09:56 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Christoph Berg <myon@debian.org>
2019-10-08 20:25 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
2019-10-09 07:33 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Christoph Berg <myon@debian.org>
@ 2019-10-09 13:37 ` Stephen Frost <sfrost@snowman.net>
2019-10-10 03:56 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Michael Paquier <michael@paquier.xyz>
2019-10-10 18:26 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 9+ messages in thread
From: Stephen Frost @ 2019-10-09 13:37 UTC (permalink / raw)
To: Christoph Berg <myon@debian.org>; Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
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
Attachments:
signature.asc (application/pgp-signature, 818B) download
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pgsql: Remove pqsignal() from libpq's official exports list.
2018-09-28 16:38 pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
2019-10-04 09:56 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Christoph Berg <myon@debian.org>
2019-10-08 20:25 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
2019-10-09 07:33 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Christoph Berg <myon@debian.org>
2019-10-09 13:37 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Stephen Frost <sfrost@snowman.net>
@ 2019-10-10 03:56 ` Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 9+ messages in thread
From: Michael Paquier @ 2019-10-10 03:56 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Christoph Berg <myon@debian.org>; Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pgsql: Remove pqsignal() from libpq's official exports list.
2018-09-28 16:38 pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
2019-10-04 09:56 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Christoph Berg <myon@debian.org>
2019-10-08 20:25 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
2019-10-09 07:33 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Christoph Berg <myon@debian.org>
2019-10-09 13:37 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Stephen Frost <sfrost@snowman.net>
@ 2019-10-10 18:26 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-10-11 18:54 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Christoph Berg <myon@debian.org>
1 sibling, 1 reply; 9+ messages in thread
From: Tom Lane @ 2019-10-10 18:26 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Christoph Berg <myon@debian.org>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pgsql: Remove pqsignal() from libpq's official exports list.
2018-09-28 16:38 pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
2019-10-04 09:56 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Christoph Berg <myon@debian.org>
2019-10-08 20:25 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
2019-10-09 07:33 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Christoph Berg <myon@debian.org>
2019-10-09 13:37 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Stephen Frost <sfrost@snowman.net>
2019-10-10 18:26 ` Re: pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
@ 2019-10-11 18:54 ` Christoph Berg <myon@debian.org>
0 siblings, 0 replies; 9+ messages in thread
From: Christoph Berg @ 2019-10-11 18:54 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Stephen Frost <sfrost@snowman.net>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2019-10-11 18:54 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-09-28 16:38 pgsql: Remove pqsignal() from libpq's official exports list. Tom Lane <tgl@sss.pgh.pa.us>
2019-10-04 09:56 ` Christoph Berg <myon@debian.org>
2019-10-07 06:59 ` Michael Paquier <michael@paquier.xyz>
2019-10-08 20:25 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-10-09 07:33 ` Christoph Berg <myon@debian.org>
2019-10-09 13:37 ` Stephen Frost <sfrost@snowman.net>
2019-10-10 03:56 ` Michael Paquier <michael@paquier.xyz>
2019-10-10 18:26 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-10-11 18:54 ` Christoph Berg <myon@debian.org>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox