Re: libpq compression

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.

> 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.

Just my 2c.
--
Michael