Re: libpq compression

On 08/13/2018 02:47 PM, Robert Haas wrote:
> 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.
>

Excellent summary, and well argued recommendations, thanks. I've changed
the status to waiting on author.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services