Re: libpq compression

On 06.06.2018 10:53, Michael Paquier wrote:
> On Tue, Jun 05, 2018 at 06:58:42PM +0300, Konstantin Knizhnik wrote:
>> I have considered this patch mostly as prototype to estimate efficiency of
>> libpq protocol compression and compare it with SSL compression.
>> So I agree with you that there are a lot of things which should be
>> improved.
> Cool.  It seems that there is some meaning for such a feature with
> environments with spare CPU and network limitations.
>
>> But can you please clarify what is wrong with "forcing the presentation of
>> the compression option in the startup packet to begin with"?
> Sure, I am referring to that in your v4:
>      if (conn->replication && conn->replication[0])
>         ADD_STARTUP_OPTION("replication", conn->replication);
> +   if (conn->compression && conn->compression[0])
> +       ADD_STARTUP_OPTION("compression", conn->compression);
> There is no point in adding that as a mandatory field of the startup
> packet.

Sorry, but ADD_STARTUP_OPTION is not adding manatory field of startup 
package. This option any be omitted.
There are a lto of other options registered using ADD_STARTUP_OPTION, 
for example all environment-driven GUCs:

     /* Add any environment-driven GUC settings needed */
     for (next_eo = options; next_eo->envName; next_eo++)
     {
         if ((val = getenv(next_eo->envName)) != NULL)
         {
             if (pg_strcasecmp(val, "default") != 0)
                 ADD_STARTUP_OPTION(next_eo->pgName, val);
         }
     }


So I do not understand what is wrong here registering "compression" as 
option of startup package and what is the alternative for it...

>> Do you mean that it will be better to be able switch on/off compression
>> during session?
> Not really, I get that this should be defined when the session is
> established and remain until the session finishes.  You have a couple of
> restrictions like what to do with the first set of messages exchanged
> but that could be delayed until the negotiation is done.
>
>> But the main difference between encryption and compression is that
>> encryption is not changing data size, while compression does.
>> To be able to use streaming compression, I need to specify some function for
>> reading data from the stream. I am using secure_read for this purpose:
>>
>>         PqStream = zpq_create((zpq_tx_func)secure_write,
>> (zpq_rx_func)secure_read, MyProcPort);
>>
>> Can you please explain what is the problem with it?
> Likely I have not looked at your patch sufficiently, but the point I am
> trying to make is that secure_read or pqsecure_read are entry points
> which switch method depending on the connection details.  The GSSAPI
> encryption patch does that.  Yours does not with stuff like that:
>
>   retry4:
> -   nread = pqsecure_read(conn, conn->inBuffer + conn->inEnd,
> -                         conn->inBufSize - conn->inEnd);
>
> This makes the whole error handling more consistent, and the new option
> layer as well more consistent with what happens in SSL, except that you
> want to be able to combine SSL and compression as well so you need an
> extra process which decompresses/compresses the data after doing a "raw"
> or "ssl" read/write.  I have not actually looked much at your patch, but
> I am wondering if it could not be possible to make the whole footprint
> less invasive which really worries me as now shaped.  As you need to
> register functions with say zpq_create(), it would be instinctively
> nicer to do the handling directly in secure_read() and such.

Once again sorry, but i still do not understand the problem here.
If compression is anabled, then I am using zpq_read instead of 
secure_read/pqsecure_read. But  zpq_read is in turn calling 
secure_read/pqsecure_read
to fetch more raw data. So if "ecure_read or pqsecure_read are entry 
points which switch method depending on the connection details", then  
compression is not preventing them from making this choice. Compression 
should be done prior to encryption otherwise compression will have no sense.

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