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 13.08.2018 21:47, 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.

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(at)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