Re: libpq compression

Hi Robert,
First of all thank you for review and your time spent on analyzing this patch.
My comments are inside.

On Fri, Aug 10, 2018 at 5:55 PM, Andrew Dunstan
<andrew.dunstan@2ndquadrant.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. 

Sorry, I know that it is too impertinently to ask you or somebody else to suggest better way of integration compression in libpq
frontend/backend. My primary intention was to use the same code both for backend and frontend. This is why I pass pointer to receive/send function
to compression module. I am passing secure_read/secure_write function for backend and pqsecure_read/pqsecure_write functions for frontend.
And change pq_recvbuf/pqReadData, internal_flush/pqSendSome functions to call zpq_read/zpq_write functions when compression is enabled.
Robbie thinks that compression should be toggled in secure_write/secure_write/pqsecure_read/pqsecure_write.
I do not understand why it is better then my implementation. From my point of view it just introduce extra code redundancy and has no advantages.
Just name of this functions (secure_*) will be confusing if this function will handle compression as well. May be it is better to introduce some other set of function
which in turn will  secuire_* functions, but I also do no think that it is good idea.

 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.  

It is not a big deal to remove command line options.
My only concern was that if somebody want to upload data using psql+COPY command,
it will be more difficult to completely change the way of invoking psql in this case rather than just adding one option
 (most of user are using -h -D -U options rather than passing complete connection string).

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. 

I will add checking of supported compression method.
Right now Postgres can be configured to use either zlib, either zstd streaming compression, but not both of them.
So choosing appreciate compression algorithm is not possible, I can only check that client and server are supporting the same compression method.
Certainly it is possible to implement support of different compression method and make it possible to chose on at runtime, but I do not think that it is really good idea
unless we want to support custom compression methods.

 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.

If I tried to login with new client with _pq_.compression option to old server (9.6.8) then I got the following error:

knizhnik@knizhnik:~/dtm-data$ psql -d "port=5432 _pq_.compression=zlib dbname=postgres"
psql: expected authentication request from server, but received v

Frankly speaking I do not see big problem here: the error will happen only if we use NEW client to connect OLD server and EXPLICITLY specify compression=on option in connection string.
There will be no problem if we use old client to access new server or use new client to access old server without switching on  compression.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company