Re: libpq compression

From: Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru>
To: Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Robbie Harwood <rharwood(at)redhat(dot)com>, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Grigory Smolkin <g(dot)smolkin(at)postgrespro(dot)ru>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: libpq compression
Date: 2018-08-20 15:00:39
Message-ID: 17747c8b-c088-6f92-38e9-98316910202a@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On 13.08.2018 23:06, Andrew Dunstan wrote:
>
>
> 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.
>
New version of the patch is attached: I removed -Z options form pgbench
and psql and add checking that server and client are implementing the
same compression algorithm.

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

Attachment Content-Type Size
libpq-compression-8.patch text/x-patch 36.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2018-08-20 15:02:09 Re: Two proposed modifications to the PostgreSQL FDW
Previous Message Alexander Korotkov 2018-08-20 15:00:09 Re: Truncation failure in autovacuum results in data corruption (duplicate keys)