Re: libpq compression

On Fri, Aug 10, 2018 at 5:55 PM, Andrew Dunstan
<andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:
> This thread appears to have gone quiet. What concerns me is that there
> appears to be substantial disagreement between the author and the reviewers.
> Since the last thing was this new patch it should really have been put back
> into "needs review" (my fault to some extent - I missed that). So rather
> than return the patch with feedfack I'm going to set it to "needs review"
> and move it to the next CF. However, if we can't arrive at a consensus about
> the direction during the next CF it should be returned with feedback.

I agree with the critiques from Robbie Harwood and Michael Paquier
that the way in that compression is being hooked into the existing
architecture looks like a kludge. I'm not sure I know exactly how it
should be done, but the current approach doesn't look natural; it
looks like it was bolted on. I agree with the critique from Peter
Eisentraut and others that we should not go around and add -Z as a
command-line option to all of our tools; this feature hasn't yet
proved itself to be useful enough to justify that. Better to let
people specify it via a connection string option if they want it. I
think Thomas Munro was right to ask about what will happen when
different compression libraries are in use, and I think failing
uncleanly is quite unacceptable. I think there needs to be some
method for negotiating the compression type; the client can, for
example, provide a comma-separated list of methods it supports in
preference order, and the server can pick the first one it likes. In
short, I think that a number of people have provided really good
feedback on this patch, and I suggest to Konstantin that he should
consider accepting all of those suggestions.

Commit ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed tried to introduce
some facilities that can be used for protocol version negotiation as
new features are added, but this patch doesn't use them. It looks to
me like it instead just breaks backward compatibility. The new
'compression' option won't be recognized by older servers. If it were
spelled '_pq_.compression' then the facilities in that commit would
cause a NegotiateProtocolVersion message to be sent by servers which
have that commit but don't support the compression option. I'm not
exactly sure what will happen on even-older servers that don't have
that commit either, but I hope they'll treat it as a GUC name; note
that setting an unknown GUC name with a namespace prefix is not an
error, but setting one without such a prefix IS an ERROR. Servers
which do support compression would respond with a message indicating
that compression had been enabled or, maybe, just start sending back
compressed-packet messages, if we go with including some framing in
the libpq protocol.

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