pgsql: Remove support for SSL compression

On Tue, Mar 09, 2021 at 02:18:52AM +0000, Michael Paquier wrote:

Remove support for SSL compression

PostgreSQL disabled compression as of e3bdb2d and the documentation
recommends against using it since. Additionally, SSL compression has
been disabled in OpenSSL since version 1.1.0, and was disabled in many
distributions long before that. The most recent TLS version, TLSv1.3,
disallows compression at the protocol level.

(Adding Daniel in CC)

So, this is breaking a test case in crake where an upgrade from 9.3 is
done with a FDW server using this option that gets restored:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2021-03-09%2002%3A32%3A29
pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 1541; 1417 55431 SERVER testserver1
buildfarm
pg_restore: error: could not execute query: ERROR: invalid option
"sslcompression"

It looks like it is not that much a good idea to define it as a debug
option after all. So I guess that the attached would fix the failure,
where FDW servers can still pass down the parameter at will for
backward-compatibility, and where libpq keeps ignoring its value. Any
thoughts?
--
Michael

On 9 Mar 2021, at 04:26, Michael Paquier <michael@paquier.xyz> wrote:

It looks like it is not that much a good idea to define it as a debug
option after all. So I guess that the attached would fix the failure,
where FDW servers can still pass down the parameter at will for
backward-compatibility, and where libpq keeps ignoring its value. Any
thoughts?

While not pretty, I think that might be the least invasive option right now.

--
Daniel Gustafsson https://vmware.com/

On Tue, Mar 09, 2021 at 12:26:15PM +0900, Michael Paquier wrote:

It looks like it is not that much a good idea to define it as a debug
option after all. So I guess that the attached would fix the failure,
where FDW servers can still pass down the parameter at will for
backward-compatibility, and where libpq keeps ignoring its value. Any
thoughts?

So, I have been thinking more about this one, and applied the previous
patch to bring back crake to green. There are a couple of options
that could be considered to make the option non-visible, but nothing
looks really appealing to me:
- Tweak the regression tests on the back-branches to bypass the
failure, but this won't change the potential impact of anybody using
sslcompression in a FDW server, or even dblink.
- Create specific exceptions in postgres_fdw and perhaps dblink.
- Extend libpq with a special status to mark a parameter as
to-be-removed, but that feels like a mix of a normal option and a
debug option.
- Make pg_upgrade complain if such a server is detected (?), bringing
in the ring the first point of this list.
--
Michael

On Tue, Mar 09, 2021 at 12:19:41PM +0100, Daniel Gustafsson wrote:

While not pretty, I think that might be the least invasive option right now.

crake has turned back to green just now. Let's discuss that a bit
more, even if it does not feel like an issue we absolutely have to
address in this release cycle.
--
Michael

On 9 Mar 2021, at 12:19, Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Mar 09, 2021 at 12:26:15PM +0900, Michael Paquier wrote:

It looks like it is not that much a good idea to define it as a debug
option after all. So I guess that the attached would fix the failure,
where FDW servers can still pass down the parameter at will for
backward-compatibility, and where libpq keeps ignoring its value. Any
thoughts?

So, I have been thinking more about this one, and applied the previous
patch to bring back crake to green. There are a couple of options
that could be considered to make the option non-visible, but nothing
looks really appealing to me:
- Tweak the regression tests on the back-branches to bypass the
failure, but this won't change the potential impact of anybody using
sslcompression in a FDW server, or even dblink.
- Create specific exceptions in postgres_fdw and perhaps dblink.
- Extend libpq with a special status to mark a parameter as
to-be-removed, but that feels like a mix of a normal option and a
debug option.
- Make pg_upgrade complain if such a server is detected (?), bringing
in the ring the first point of this list.

Having tried a number of different ways to fix this I think the least intrusive
fix is to teach pg_upgrade and dblink about sslcompression, like how've already
taught it about other options. Building any infrastructure to handle
deprecated options and extending the dispchar with a classification for
deprecation for just this one option seems overkill. Deprecating options is
also not something frequently done, so the cost/benefit for something
complicated seems off.

The attached is a version of the simple fix.

--
Daniel Gustafsson https://vmware.com/

Daniel Gustafsson <daniel@yesql.se> writes:

Having tried a number of different ways to fix this I think the least intrusive
fix is to teach pg_upgrade and dblink about sslcompression, like how've already
taught it about other options. Building any infrastructure to handle
deprecated options and extending the dispchar with a classification for
deprecation for just this one option seems overkill. Deprecating options is
also not something frequently done, so the cost/benefit for something
complicated seems off.

I'm fairly distressed by this patch series, and this proposed change in
particular, because it seems to be going in very much the wrong direction.
The objective ought to be simplification, but you're doing exactly the
opposite.

Also, the changes in postgres_fdw.out remind me way too much of this
problem my successors at Red Hat are fighting:

https://bugzilla.redhat.com/show_bug.cgi?id=1915179

The context there is that they are now shipping concurrently-installable
packages for a couple of different PG versions, but they only install
the latest available version of libpq.so into /usr/lib, figuring that
the back-rev versions can rely on that one. Maybe that's a foolish idea
on their part (but I have a vague idea that Debian does something
similar?). Any such setup is going to have pain whenever we change
the externally-visible behavior of libpq.

I'm inclined to think that we should leave this parameter completely
alone, and at most choose to ignore its value internally to libpq.
But really, why is there urgency to do anything at all with it?
It's not our business whether OpenSSL chooses to ignore it.

In short, I think I'm a vote for reverting to the v13 behavior.

regards, tom lane

On 9 Mar 2021, at 16:58, Tom Lane <tgl@sss.pgh.pa.us> wrote:

In short, I think I'm a vote for reverting to the v13 behavior.

Ok, then let us just go ahead and do that.

--
Daniel Gustafsson https://vmware.com/

On Tue, Mar 09, 2021 at 10:58:21AM -0500, Tom Lane wrote:

I'm fairly distressed by this patch series, and this proposed change in
particular, because it seems to be going in very much the wrong direction.
The objective ought to be simplification, but you're doing exactly the
opposite.

Yeah. I'd rather simplify things more.

Also, the changes in postgres_fdw.out remind me way too much of this
problem my successors at Red Hat are fighting:

https://bugzilla.redhat.com/show_bug.cgi?id=1915179

The context there is that they are now shipping concurrently-installable
packages for a couple of different PG versions, but they only install
the latest available version of libpq.so into /usr/lib, figuring that
the back-rev versions can rely on that one. Maybe that's a foolish idea
on their part (but I have a vague idea that Debian does something
similar?). Any such setup is going to have pain whenever we change
the externally-visible behavior of libpq.

Err. That's indeed a weird choice as I would have though that any
past version of postgres_fdw.so or dblink.so would link to their
respective libpq, but that makes LD_LIBRARY_PATH a no-brainer.

I'm inclined to think that we should leave this parameter completely
alone, and at most choose to ignore its value internally to libpq.
But really, why is there urgency to do anything at all with it?
It's not our business whether OpenSSL chooses to ignore it.

In short, I think I'm a vote for reverting to the v13 behavior.

The libpq part is a good point, as it could be annoying for people to
get a changing behavior once the libpq version changes for anything
using the older major version binaries once the newer libpq is out. I
am still in favor of keeping the simplifications in the backend though
as it simplifies some catalogs and PgBackendSSLStatus, and we know
that SSL compression does not really matter these days.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Tue, Mar 09, 2021 at 10:58:21AM -0500, Tom Lane wrote:

In short, I think I'm a vote for reverting to the v13 behavior.

The libpq part is a good point, as it could be annoying for people to
get a changing behavior once the libpq version changes for anything
using the older major version binaries once the newer libpq is out. I
am still in favor of keeping the simplifications in the backend though
as it simplifies some catalogs and PgBackendSSLStatus, and we know
that SSL compression does not really matter these days.

I have no objection to the backend-side changes, since we can be sure
those won't affect people until they do a major-version upgrade to v14.
But the situation is squishier on the client side, and I don't really
see that pulling out libpq's support is buying us much.

regards, tom lane

On Tue, Mar 09, 2021 at 05:45:56PM -0500, Tom Lane wrote:

I have no objection to the backend-side changes, since we can be sure
those won't affect people until they do a major-version upgrade to v14.
But the situation is squishier on the client side, and I don't really
see that pulling out libpq's support is buying us much.

Agreed. So done this way and applied a fix that pushes back libpq in
its original state while letting the backend alone.
--
Michael