Re: libpq compression

Hi hackers,

One of our customers was managed to improve speed about 10 times by
using SSL compression for the system where client and servers are
located in different geographical regions
and query results are very large because of JSON columns. Them actually
do not need encryption, just compression.
I expect that it is not the only case where compression of libpq
protocol can be useful. Please notice that Postgres replication is also
using libpq protocol.

Taken in account that vulnerability was found in SSL compression and so
SSLComppression is considered to be deprecated and insecure
(http://www.postgresql-archive.org/disable-SSL-compression-td6010072.html)
it will be nice to have some alternative mechanism of reducing libpq
traffic.

I have implemented some prototype implementation of it (patch is attached).
To use zstd compression, Postgres should be configured with --with-zstd.
Otherwise compression will use zlib unless it is disabled by
--without-zlib option.
I have added compression=on/off parameter to connection string and -Z
option to psql and pgbench utilities.
Below are some results:

Compression ratio (raw->compressed):

libz (level=1)
libzstd (level=1)
pgbench -i -s 10
16997209->2536330
16997209->268077
pgbench -t 100000 -S
6289036->1523862
6600338<-900293
6288933->1777400
6600338<-1000318

There is no mistyping: libzstd compress COPY data about 10 times better
than libz, with wonderful compression ratio 63.

Influence on execution time is minimal (I have tested local
configuration when client and server are at the same host):

no compression
libz (level=1)
libzstd (level=1)
pgbench -i -s 10
1.552
1.572
1.611
pgbench -t 100000 -S
4.482
4.926
4.877

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

> On 30 March 2018 at 14:53, Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru>
wrote:
> Hi hackers,
> One of our customers was managed to improve speed about 10 times by using
SSL compression for the system where client and servers are located in
different geographical regions
> and query results are very large because of JSON columns. Them actually
do not need encryption, just compression.
> I expect that it is not the only case where compression of libpq protocol
can be useful. Please notice that Postgres replication is also using libpq
protocol.
>
> Taken in account that vulnerability was found in SSL compression and so
SSLComppression is considered to be deprecated and insecure (
http://www.postgresql-archive.org/disable-SSL-compression-td6010072.html)
it will be nice to have some alternative mechanism of reducing libpq
traffic.
>
> I have implemented some prototype implementation of it (patch is
attached).
> To use zstd compression, Postgres should be configured with --with-zstd.
Otherwise compression will use zlib unless it is disabled by --without-zlib
option.
> I have added compression=on/off parameter to connection string and -Z
option to psql and pgbench utilities.

I'm a bit confused why there was no reply to this. I mean, it wasn't sent on
1st April, the patch still can be applied on top of the master branch and
looks
like it even works.

I assume the main concern her is that it's implemented in a rather not
extensible way. Also, if I understand correctly, it compresses the data
stream
in both direction server <-> client, not sure if there is any value in
compressing what a client sends to a server. But still I'm wondering why it
didn't start at least a discussion about how it can be implemented. Do I
miss
something?

On 15.05.2018 13:23, Dmitry Dolgov wrote:
> > On 30 March 2018 at 14:53, Konstantin Knizhnik
> <k(dot)knizhnik(at)postgrespro(dot)ru <mailto:k(dot)knizhnik(at)postgrespro(dot)ru>> wrote:
> > Hi hackers,
> > One of our customers was managed to improve speed about 10 times by
> using SSL compression for the system where client and servers are
> located in different geographical regions
> > and query results are very large because of JSON columns. Them
> actually do not need encryption, just compression.
> > I expect that it is not the only case where compression of libpq
> protocol can be useful. Please notice that Postgres replication is
> also using libpq protocol.
> >
> > Taken in account that vulnerability was found in SSL compression and
> so SSLComppression is considered to be deprecated and insecure
> (http://www.postgresql-archive.org/disable-SSL-compression-td6010072.html)
> it will be nice to have some alternative mechanism of reducing  libpq
> traffic.
> >
> > I have implemented some prototype implementation of it (patch is
> attached).
> > To use zstd compression, Postgres should be configured with
> --with-zstd. Otherwise compression will use zlib unless it is disabled
> by --without-zlib option.
> > I have added compression=on/off parameter to connection string and
> -Z option to psql and pgbench utilities.
>
> I'm a bit confused why there was no reply to this. I mean, it wasn't
> sent on
> 1st April, the patch still can be applied on top of the master branch
> and looks
> like it even works.
>
> I assume the main concern her is that it's implemented in a rather not
> extensible way. Also, if I understand correctly, it compresses the
> data stream
> in both direction server <-> client, not sure if there is any value in
> compressing what a client sends to a server. But still I'm wondering
> why it
> didn't start at least a discussion about how it can be implemented. Do
> I miss
> something?

Implementation of libpq compression will be included in next release of
PgProEE.
Looks like community is not so interested in this patch. Frankly
speaking I do not understand why.
Compression of libpq traffic can significantly increase speed of:
1. COPY
2. Replication (both streaming and logical)
3. Queries returning large results sets (for example JSON) through slow
connections.

It is possible to compress libpq traffic using SSL compression.  But SSL
compression is unsafe and deteriorated feature.

Yes, this patch is not extensible: it can use either zlib either zstd.
Unfortunately internal Postgres compression pglz doesn't provide
streaming API.
May be it is good idea to combine it with Ildus patch (custom
compression methods): https://commitfest.postgresql.org/18/1294/
In this case it will be possible to use any custom compression
algorithm. But we need to design and implement streaming API for pglz
and other compressors.

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

On 05/15/2018 08:53 AM, Konstantin Knizhnik wrote:
>
>
> On 15.05.2018 13:23, Dmitry Dolgov wrote:
>> > On 30 March 2018 at 14:53, Konstantin Knizhnik
>> <k(dot)knizhnik(at)postgrespro(dot)ru <mailto:k(dot)knizhnik(at)postgrespro(dot)ru>> wrote:
>> > Hi hackers,
>> > One of our customers was managed to improve speed about 10 times by
>> using SSL compression for the system where client and servers are
>> located in different geographical regions
>> > and query results are very large because of JSON columns. Them
>> actually do not need encryption, just compression.
>> > I expect that it is not the only case where compression of libpq
>> protocol can be useful. Please notice that Postgres replication is
>> also using libpq protocol.
>> >
>> > Taken in account that vulnerability was found in SSL compression
>> and so SSLComppression is considered to be deprecated and insecure
>> (http://www.postgresql-archive.org/disable-SSL-compression-td6010072.html)
>> it will be nice to have some alternative mechanism of reducing  libpq
>> traffic.
>> >
>> > I have implemented some prototype implementation of it (patch is
>> attached).
>> > To use zstd compression, Postgres should be configured with
>> --with-zstd. Otherwise compression will use zlib unless it is
>> disabled by --without-zlib option.
>> > I have added compression=on/off parameter to connection string and
>> -Z option to psql and pgbench utilities.
>>
>> I'm a bit confused why there was no reply to this. I mean, it wasn't
>> sent on
>> 1st April, the patch still can be applied on top of the master branch
>> and looks
>> like it even works.
>>
>> I assume the main concern her is that it's implemented in a rather not
>> extensible way. Also, if I understand correctly, it compresses the
>> data stream
>> in both direction server <-> client, not sure if there is any value in
>> compressing what a client sends to a server. But still I'm wondering
>> why it
>> didn't start at least a discussion about how it can be implemented.
>> Do I miss
>> something?
>
> Implementation of libpq compression will be included in next release
> of PgProEE.
> Looks like community is not so interested in this patch. Frankly
> speaking I do not understand why.
> Compression of libpq traffic can significantly increase speed of:
> 1. COPY
> 2. Replication (both streaming and logical)
> 3. Queries returning large results sets (for example JSON) through
> slow connections.
>
> It is possible to compress libpq traffic using SSL compression. But
> SSL compression is unsafe and deteriorated feature.
>
> Yes, this patch is not extensible: it can use either zlib either zstd.
> Unfortunately internal Postgres compression pglz doesn't provide
> streaming API.
> May be it is good idea to combine it with Ildus patch (custom
> compression methods): https://commitfest.postgresql.org/18/1294/
> In this case it will be possible to use any custom compression
> algorithm. But we need to design and implement streaming API for pglz
> and other compressors.
>
>

I'm sure there is plenty of interest in this. However, you guys need to
understand where we are in the development cycle. We're trying to wrap
up Postgres 11, which was feature frozen before this patch ever landed.
So it's material for Postgres 12. That means it will probably need to
wait a little while before it gets attention. It doesn't mean nobody is
interested.

I disagree with Dmitry about compressing in both directions - I can
think of plenty of good cases where we would want to compress traffic
from the client.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 15 May 2018 at 21:36, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com>
wrote:

>
>
>>> > To use zstd compression, Postgres should be configured with
>>> --with-zstd. Otherwise compression will use zlib unless it is disabled by
>>> --without-zlib option.
>>> > I have added compression=on/off parameter to connection string and -Z
>>> option to psql and pgbench utilities.
>>>
>>> I'm a bit confused why there was no reply to this. I mean, it wasn't
>>> sent on
>>> 1st April, the patch still can be applied on top of the master branch
>>> and looks
>>> like it even works.
>>>
>>> I assume the main concern her is that it's implemented in a rather not
>>> extensible way. Also, if I understand correctly, it compresses the data
>>> stream
>>> in both direction server <-> client, not sure if there is any value in
>>> compressing what a client sends to a server. But still I'm wondering why
>>> it
>>> didn't start at least a discussion about how it can be implemented. Do I
>>> miss
>>> something?
>>>
>>
>> Implementation of libpq compression will be included in next release of
>> PgProEE.
>> Looks like community is not so interested in this patch. Frankly speaking
>> I do not understand why.
>>
>
I'm definitely very interested, and simply missed the post.

I'll talk with some team mates as we're doing some PG12 planning now.

> Yes, this patch is not extensible: it can use either zlib either zstd.
>> Unfortunately internal Postgres compression pglz doesn't provide streaming
>> API.
>> May be it is good idea to combine it with Ildus patch (custom compression
>> methods): https://commitfest.postgresql.org/18/1294/
>>
>
Given the history of issues with attempting custom/pluggable compression
for toast etc, I really wouldn't want to couple those up.

pglz wouldn't make much sense for protocol compression anyway, except maybe
for fast local links where it was worth a slight compression overhead but
not the cpu needed for gzip. I don't think it's too exciting. zlib/gzip is
likely the sweet spot for the reasonable future for protocol compression,
or a heck of a lot better than what we have, anyway.

We should make sure the protocol part is extensible, but the implementation
doesn't need to be pluggable.

In this case it will be possible to use any custom compression algorithm.
>> But we need to design and implement streaming API for pglz and other
>> compressors.
>>
>
> I'm sure there is plenty of interest in this. However, you guys need to
> understand where we are in the development cycle. We're trying to wrap up
> Postgres 11, which was feature frozen before this patch ever landed. So
> it's material for Postgres 12. That means it will probably need to wait a
> little while before it gets attention. It doesn't mean nobody is interested.
>
> I disagree with Dmitry about compressing in both directions - I can think
> of plenty of good cases where we would want to compress traffic from the
> client.
>

Agreed. The most obvious case being COPY, but there's also big bytea
values, etc.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

2018-05-15 9:53 GMT-03:00 Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru>:
> Looks like community is not so interested in this patch. Frankly speaking I
> do not understand why.
>
AFAICS the lack of replies is due to feature freeze. I'm pretty sure
people are interested in this topic (at least I am). Did you review a
previous discussion [1] about this?

I did a prototype a few years ago. I didn't look at your patch yet.
I'll do in a few weeks. Please add your patch to the next CF [2].

[1] /message-id/4FD9698F.2090407%40timbira.com
[2] https://commitfest.postgresql.org/18/

--
Euler Taveira Timbira -
http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Hello!
I have noticed that psql --help lack -Z|--compression option.
Also it would be nice to have option like --compression-level in psql
and pgbench.

On 03/30/2018 03:53 PM, Konstantin Knizhnik wrote:
> Hi hackers,
>
> One of our customers was managed to improve speed about 10 times by
> using SSL compression for the system where client and servers are
> located in different geographical regions
> and query results are very large because of JSON columns. Them
> actually do not need encryption, just compression.
> I expect that it is not the only case where compression of libpq
> protocol can be useful. Please notice that Postgres replication is
> also using libpq protocol.
>
> Taken in account that vulnerability was found in SSL compression and
> so SSLComppression is considered to be deprecated and insecure
> (http://www.postgresql-archive.org/disable-SSL-compression-td6010072.html)
> it will be nice to have some alternative mechanism of reducing libpq
> traffic.
>
> I have implemented some prototype implementation of it (patch is
> attached).
> To use zstd compression, Postgres should be configured with
> --with-zstd. Otherwise compression will use zlib unless it is disabled
> by --without-zlib option.
> I have added compression=on/off parameter to connection string and -Z
> option to psql and pgbench utilities.
> Below are some results:
>
> Compression ratio (raw->compressed):
>
>
> libz (level=1)
> libzstd (level=1)
> pgbench -i -s 10
> 16997209->2536330
> 16997209->268077
> pgbench -t 100000 -S
> 6289036->1523862
> 6600338<-900293
> 6288933->1777400
> 6600338<-1000318
>
>
> There is no mistyping: libzstd compress COPY data about 10 times
> better than libz, with wonderful compression ratio 63.
>
> Influence on execution time is minimal (I have tested local
> configuration when client and server are at the same host):
>
>
> no compression
> libz (level=1)
> libzstd (level=1)
> pgbench -i -s 10
> 1.552
> 1.572
> 1.611
> pgbench -t 100000 -S
> 4.482
> 4.926
> 4.877
>
> --
> Konstantin Knizhnik
> Postgres Professional:http://www.postgrespro.com
> The Russian Postgres Company

--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 16.05.2018 18:09, Grigory Smolkin wrote:
>
> Hello!
> I have noticed that psql --help lack -Z|--compression option.
> Also it would be nice to have option like --compression-level in psql
> and pgbench.
>
Thank you for this notice.
Updated and rebased patch is attached.
Concerning specification of compression level: I have made many
experiments with different data sets and both zlib/zstd and in both
cases using compression level higher than default doesn't cause some
noticeable increase of compression ratio, but quite significantly reduce
speed. Moreover, for "pgbench -i" zstd provides better compression ratio
(63 times!) with compression level 1 than with with largest recommended
compression level 22! This is why I decided not to allow user to choose
compression level.

On Thu, May 17, 2018 at 3:54 AM, Konstantin Knizhnik
<k(dot)knizhnik(at)postgrespro(dot)ru> wrote:
> Thank you for this notice.
> Updated and rebased patch is attached.

Hi Konstantin,

Seems very useful. +1.

+ rc = inflate(&zs->rx, Z_SYNC_FLUSH);
+ if (rc != Z_OK)
+ {
+ return ZPQ_DECOMPRESS_ERROR;
+ }

Does this actually guarantee that zs->rx.msg is set to a string? I
looked at some documentation here:

https://www.zlib.net/manual.html

It looks like return value Z_DATA_ERROR means that msg is set, but for
the other error codes Z_STREAM_ERROR, Z_BUF_ERROR, Z_MEM_ERROR it
doesn't explicitly say that. From a casual glance at
https://github.com/madler/zlib/blob/master/inflate.c I think it might
be set to Z_NULL and then never set to a string except in the mode =
BAD paths that produce the Z_DATA_ERROR return code. That's
interesting because later we do this:

+ if (r == ZPQ_DECOMPRESS_ERROR)
+ {
+ ereport(COMMERROR,
+ (errcode_for_socket_access(),
+ errmsg("Failed to decompress data: %s", zpq_error(PqStream))));
+ return EOF;

... where zpq_error() returns zs->rx.msg. That might crash or show
"(null)" depending on libc.

Also, message style: s/F/f/

+ssize_t zpq_read(ZpqStream* zs, void* buf, size_t size, size_t* processed)

Code style: We write "Type *foo", not "Type* var". We put the return
type of a function definition on its own line.

It looks like there is at least one place where zpq_stream.o's symbols
are needed but it isn't being linked in, so the build fails in some
ecpg stuff reached by make check-world:

gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -fexcess-precision=standard -g -O2 -pthread -D_REENTRANT
-D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS test1.o
-L../../../../../src/port -L../../../../../src/common -L../../ecpglib
-lecpg -L../../pgtypeslib -lpgtypes
-L../../../../../src/interfaces/libpq -lpq -Wl,--as-needed
-Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags -lpgcommon
-lpgport -lpthread -lz -lreadline -lrt -lcrypt -ldl -lm -o test1
../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_free'
../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_error'
../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_read'
../../../../../src/interfaces/libpq/libpq.so: undefined reference to
`zpq_buffered'
../../../../../src/interfaces/libpq/libpq.so: undefined reference to
`zpq_create'
../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_write'

--
Thomas Munro
http://www.enterprisedb.com

On Thu, May 17, 2018 at 3:54 AM, Konstantin Knizhnik
<k(dot)knizhnik(at)postgrespro(dot)ru> wrote:
> Concerning specification of compression level: I have made many experiments
> with different data sets and both zlib/zstd and in both cases using
> compression level higher than default doesn't cause some noticeable increase
> of compression ratio, but quite significantly reduce speed. Moreover, for
> "pgbench -i" zstd provides better compression ratio (63 times!) with
> compression level 1 than with with largest recommended compression level 22!
> This is why I decided not to allow user to choose compression level.

Speaking of configuration, are you planning to support multiple
compression libraries at the same time? It looks like the current
patch implicitly requires client and server to use the same configure
option, without any attempt to detect or negotiate. Do I guess
correctly that a library mismatch would produce an incomprehensible
corrupt stream message?

--
Thomas Munro
http://www.enterprisedb.com

On Tue, Jun 05, 2018 at 06:04:21PM +1200, Thomas Munro wrote:
> On Thu, May 17, 2018 at 3:54 AM, Konstantin Knizhnik
> Speaking of configuration, are you planning to support multiple
> compression libraries at the same time? It looks like the current
> patch implicitly requires client and server to use the same configure
> option, without any attempt to detect or negotiate. Do I guess
> correctly that a library mismatch would produce an incomprehensible
> corrupt stream message?

I just had a quick look at this patch, lured by the smell of your latest
messages... And it seems to me that this patch needs a heavy amount of
work as presented. There are a couple of things which are not really
nice, like forcing the presentation of the compression option in the
startup packet to begin with. The high-jacking around secure_read() is
not nice either as it is aimed at being a rather high-level API on top
of the method used with the backend. On top of adding some
documentation, I think that you could get some inspiration from the
recent GSSAPI encription patch which has been submitted again for v12
cycle, which has spent a large amount of time designing its set of
options.
--
Michael

On 05.06.2018 08:26, Thomas Munro wrote:
> On Thu, May 17, 2018 at 3:54 AM, Konstantin Knizhnik
> <k(dot)knizhnik(at)postgrespro(dot)ru> wrote:
>> Thank you for this notice.
>> Updated and rebased patch is attached.
> Hi Konstantin,
>
> Seems very useful. +1.
>
> + rc = inflate(&zs->rx, Z_SYNC_FLUSH);
> + if (rc != Z_OK)
> + {
> + return ZPQ_DECOMPRESS_ERROR;
> + }
>
> Does this actually guarantee that zs->rx.msg is set to a string? I
> looked at some documentation here:
>
> https://www.zlib.net/manual.html
>
> It looks like return value Z_DATA_ERROR means that msg is set, but for
> the other error codes Z_STREAM_ERROR, Z_BUF_ERROR, Z_MEM_ERROR it
> doesn't explicitly say that. From a casual glance at
> https://github.com/madler/zlib/blob/master/inflate.c I think it might
> be set to Z_NULL and then never set to a string except in the mode =
> BAD paths that produce the Z_DATA_ERROR return code. That's
> interesting because later we do this:
>
> + if (r == ZPQ_DECOMPRESS_ERROR)
> + {
> + ereport(COMMERROR,
> + (errcode_for_socket_access(),
> + errmsg("Failed to decompress data: %s", zpq_error(PqStream))));
> + return EOF;
>
> ... where zpq_error() returns zs->rx.msg. That might crash or show
> "(null)" depending on libc.
>
> Also, message style: s/F/f/
>
> +ssize_t zpq_read(ZpqStream* zs, void* buf, size_t size, size_t* processed)
>
> Code style: We write "Type *foo", not "Type* var". We put the return
> type of a function definition on its own line.
>
> It looks like there is at least one place where zpq_stream.o's symbols
> are needed but it isn't being linked in, so the build fails in some
> ecpg stuff reached by make check-world:
>
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Wendif-labels
> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
> -fwrapv -fexcess-precision=standard -g -O2 -pthread -D_REENTRANT
> -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS test1.o
> -L../../../../../src/port -L../../../../../src/common -L../../ecpglib
> -lecpg -L../../pgtypeslib -lpgtypes
> -L../../../../../src/interfaces/libpq -lpq -Wl,--as-needed
> -Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags -lpgcommon
> -lpgport -lpthread -lz -lreadline -lrt -lcrypt -ldl -lm -o test1
> ../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_free'
> ../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_error'
> ../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_read'
> ../../../../../src/interfaces/libpq/libpq.so: undefined reference to
> `zpq_buffered'
> ../../../../../src/interfaces/libpq/libpq.so: undefined reference to
> `zpq_create'
> ../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_write'
>

Hi Thomas,
Thank you for review. Updated version of the patch fixing all reported
problems is attached.

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

On 05.06.2018 09:04, Thomas Munro wrote:
> On Thu, May 17, 2018 at 3:54 AM, Konstantin Knizhnik
> <k(dot)knizhnik(at)postgrespro(dot)ru> wrote:
>> Concerning specification of compression level: I have made many experiments
>> with different data sets and both zlib/zstd and in both cases using
>> compression level higher than default doesn't cause some noticeable increase
>> of compression ratio, but quite significantly reduce speed. Moreover, for
>> "pgbench -i" zstd provides better compression ratio (63 times!) with
>> compression level 1 than with with largest recommended compression level 22!
>> This is why I decided not to allow user to choose compression level.
> Speaking of configuration, are you planning to support multiple
> compression libraries at the same time? It looks like the current
> patch implicitly requires client and server to use the same configure
> option, without any attempt to detect or negotiate. Do I guess
> correctly that a library mismatch would produce an incomprehensible
> corrupt stream message?
>
Frankly speaking I am not sure that support of multiple compression
libraries at the same time is actually needed.
If we build Postgres from sources, then both frontend and backend
libraries will use the same compression library.
zlib is available almost everywhere and Postgres in any case is using it.
zstd is faster and provides better compression ratio. So in principle it
may be useful to try first to use zstd and if it is not available then
use zlib.
It will require dynamic loading of this libraries.

libpq stream compression is not the only place where compression is used
in Postgres. So I think that the problem of  choosing compression algorithm
and supporting custom compression methods should be fixed at some upper
level. There is patch for custom compression method at commit fest.
May be it should be combined with this one.

Right now if client and server libpq libraries were built with different
compression libraries, then decompress error will be reported.
Supporting multiple compression methods will require more sophisticated
handshake protocol so that client and server can choose compression
method which is supported by both of them.
But certainly it can be done.

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

On 05.06.2018 10:09, Michael Paquier wrote:
> On Tue, Jun 05, 2018 at 06:04:21PM +1200, Thomas Munro wrote:
>> On Thu, May 17, 2018 at 3:54 AM, Konstantin Knizhnik
>> Speaking of configuration, are you planning to support multiple
>> compression libraries at the same time? It looks like the current
>> patch implicitly requires client and server to use the same configure
>> option, without any attempt to detect or negotiate. Do I guess
>> correctly that a library mismatch would produce an incomprehensible
>> corrupt stream message?
> I just had a quick look at this patch, lured by the smell of your latest
> messages... And it seems to me that this patch needs a heavy amount of
> work as presented. There are a couple of things which are not really
> nice, like forcing the presentation of the compression option in the
> startup packet to begin with. The high-jacking around secure_read() is
> not nice either as it is aimed at being a rather high-level API on top
> of the method used with the backend. On top of adding some
> documentation, I think that you could get some inspiration from the
> recent GSSAPI encription patch which has been submitted again for v12
> cycle, which has spent a large amount of time designing its set of
> options.
> --
> Michael
Thank you for feedback,
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.

But can you please clarify what is wrong with "forcing the presentation
of the compression option in the startup packet to begin with"?
Do you mean that it will be better to be able switch on/off compression
during session?

Also I do not completely understand what do you mean by "high-jacking
around secure_read()".
I looked at GSSAPI patch. It does injection in secure_read:

+#ifdef ENABLE_GSS
+    if (port->gss->enc)
+    {
+        n = be_gssapi_read(port, ptr, len);
+        waitfor = WL_SOCKET_READABLE;
+    }
+    else

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?

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

On 6/5/18 03:09, Michael Paquier wrote:
> I just had a quick look at this patch, lured by the smell of your latest
> messages... And it seems to me that this patch needs a heavy amount of
> work as presented. There are a couple of things which are not really
> nice, like forcing the presentation of the compression option in the
> startup packet to begin with.

Yeah, at this point we will probably need a discussion and explanation
of the protocol behavior this is adding, such as how to negotiate
different compression settings.

Unrelatedly, I suggest skipping the addition of -Z options to various
client-side tools. This is unnecessary, since generic connection
options can already be specified via -d typically, and it creates
confusion because -Z is already used to specify output compression by
some programs.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 5 June 2018 at 13:06, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
wrote:

> On 6/5/18 03:09, Michael Paquier wrote:
> > I just had a quick look at this patch, lured by the smell of your latest
> > messages... And it seems to me that this patch needs a heavy amount of
> > work as presented. There are a couple of things which are not really
> > nice, like forcing the presentation of the compression option in the
> > startup packet to begin with.
>
> Yeah, at this point we will probably need a discussion and explanation
> of the protocol behavior this is adding, such as how to negotiate
> different compression settings.
>
> Unrelatedly, I suggest skipping the addition of -Z options to various
> client-side tools. This is unnecessary, since generic connection
> options can already be specified via -d typically, and it creates
> confusion because -Z is already used to specify output compression by
> some programs.
>
>
As the maintainer of the JDBC driver I would think we would also like to
leverage this as well.

There are a few other drivers that implement the protocol as well and I'm
sure they would want in as well.
I haven't looked at the patch but if we get to the point of negotiating
compression please let me know.

Thanks,

Dave Cramer

davec(at)postgresintl(dot)com
www.postgresintl.com

On Wed, Jun 6, 2018 at 2:06 AM, Konstantin Knizhnik
<k(dot)knizhnik(at)postgrespro(dot)ru> wrote:
> Thank you for review. Updated version of the patch fixing all reported
> problems is attached.

Small problem on Windows[1]:

C:\projects\postgresql\src\include\common/zpq_stream.h(17): error
C2143: syntax error : missing ')' before '*'
[C:\projects\postgresql\libpq.vcxproj]
2395

You used ssize_t in zpq_stream.h, but Windows doesn't have that type.
We have our own typedef in win32_port.h. Perhaps zpq_stream.c should
include postgres.h/postgres_fe.h (depending on FRONTEND) like the
other .c files in src/common, before it includes zpq_stream.h?
Instead of "c.h".

[1] https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.1106

--
Thomas Munro
http://www.enterprisedb.com

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

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

On 06.06.2018 02:03, Thomas Munro wrote:
> On Wed, Jun 6, 2018 at 2:06 AM, Konstantin Knizhnik
> <k(dot)knizhnik(at)postgrespro(dot)ru> wrote:
>> Thank you for review. Updated version of the patch fixing all reported
>> problems is attached.
> Small problem on Windows[1]:
>
> C:\projects\postgresql\src\include\common/zpq_stream.h(17): error
> C2143: syntax error : missing ')' before '*'
> [C:\projects\postgresql\libpq.vcxproj]
> 2395
>
> You used ssize_t in zpq_stream.h, but Windows doesn't have that type.
> We have our own typedef in win32_port.h. Perhaps zpq_stream.c should
> include postgres.h/postgres_fe.h (depending on FRONTEND) like the
> other .c files in src/common, before it includes zpq_stream.h?
> Instead of "c.h".
>
> [1] https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.1106
>
Thank you very much for reporting the problem.
I attached new patch with include of postgres_fe.h added to zpq_stream.c

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

On 05.06.2018 20:06, Peter Eisentraut wrote:
> On 6/5/18 03:09, Michael Paquier wrote:
>> I just had a quick look at this patch, lured by the smell of your latest
>> messages... And it seems to me that this patch needs a heavy amount of
>> work as presented. There are a couple of things which are not really
>> nice, like forcing the presentation of the compression option in the
>> startup packet to begin with.
> Yeah, at this point we will probably need a discussion and explanation
> of the protocol behavior this is adding, such as how to negotiate
> different compression settings.
>
> Unrelatedly, I suggest skipping the addition of -Z options to various
> client-side tools. This is unnecessary, since generic connection
> options can already be specified via -d typically, and it creates
> confusion because -Z is already used to specify output compression by
> some programs.
>

Sorry, psql is using '-d' option for specifying database name and
pgbench is using '-d' option for toggling debug output.
So may be there is some other way to pass generic connection option, but
in any case it seems to be less convenient for users.
Also I do not see any contradiction with using -Z option in some other
tools (pg_basebackup, pg_receivewal, pg_dump)
for enabling output compression. It will be bad if that option has
contradictory meaning in different tools. But if it is used for toggling
compression
(doesn't matter at which level), then I do not see that it can be source
of confusion.

The only problem is with pg_dump which establish connection with server
to fetch data from the database and is able to compress output data.
So here we may need two options: compress input and compress output. 
But I do not think that because of it -Z option should be removed from
psql and pgbench.

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

On 06.06.2018 19:33, Konstantin Knizhnik wrote:
>
>
> On 05.06.2018 20:06, Peter Eisentraut wrote:
>> On 6/5/18 03:09, Michael Paquier wrote:
>>> I just had a quick look at this patch, lured by the smell of your
>>> latest
>>> messages...  And it seems to me that this patch needs a heavy amount of
>>> work as presented.  There are a couple of things which are not really
>>> nice, like forcing the presentation of the compression option in the
>>> startup packet to begin with.
>> Yeah, at this point we will probably need a discussion and explanation
>> of the protocol behavior this is adding, such as how to negotiate
>> different compression settings.
>>
>> Unrelatedly, I suggest skipping the addition of -Z options to various
>> client-side tools.  This is unnecessary, since generic connection
>> options can already be specified via -d typically, and it creates
>> confusion because -Z is already used to specify output compression by
>> some programs.
>>
>
> Sorry, psql is using '-d' option for specifying database name and
> pgbench is using '-d' option for toggling debug output.
> So may be there is some other way to pass generic connection option,
> but in any case it seems to be less convenient for users.
> Also I do not see any contradiction with using -Z option in some other
> tools (pg_basebackup, pg_receivewal, pg_dump)
> for enabling output compression. It will be bad if that option has
> contradictory meaning in different tools. But if it is used for
> toggling compression
> (doesn't matter at which level), then I do not see that it can be
> source of confusion.
>
> The only problem is with pg_dump which establish connection with
> server to fetch data from the database and is able to compress output
> data.
> So here we may need two options: compress input and compress output. 
> But I do not think that because of it -Z option should be removed from
> psql and pgbench.
>
>
Well, psql really allows to specify complete connection string with -d
options (although it is not mentioned in help).
But still I think that it is inconvenient to require user to write
complete connection string to be able to specify compression option,
while everybody prefer to use -h, -U, -p  options to specify
correspondent components of connection string.

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

On 06/06/2018 10:20 AM, Konstantin Knizhnik wrote:

> Well, psql really allows to specify complete connection string with -d
> options (although it is not mentioned in help).
> But still I think that it is inconvenient to require user to write
> complete connection string to be able to specify compression option,
> while everybody prefer to use -h, -U, -p  options to specify
> correspondent components of connection string.

From a barrier to entry and simplicity sake I agree. We should have a
standard flag.

JD

--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc
*** A fault and talent of mine is to tell it exactly how it is. ***
PostgreSQL centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://postgresconf.org
***** Unless otherwise stated, opinions are my own. *****

On 6/6/18 13:20, Konstantin Knizhnik wrote:
> Well, psql really allows to specify complete connection string with -d
> options (although it is not mentioned in help).
> But still I think that it is inconvenient to require user to write
> complete connection string to be able to specify compression option,
> while everybody prefer to use -h, -U, -p  options to specify
> correspondent components of connection string.

I recommend that you avoid derailing your effort by hinging it on this
issue. You can always add command-line options after the libpq support
is in.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wednesday, June 6, 2018, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)
com> wrote:

> On 6/6/18 13:20, Konstantin Knizhnik wrote:
> > Well, psql really allows to specify complete connection string with -d
> > options (although it is not mentioned in help).
> > But still I think that it is inconvenient to require user to write
> > complete connection string to be able to specify compression option,
> > while everybody prefer to use -h, -U, -p options to specify
> > correspondent components of connection string.
>
> I recommend that you avoid derailing your effort by hinging it on this
> issue. You can always add command-line options after the libpq support
> is in.
>
>
It probably requires a long option. It can be debated whether a short
option is warranted (as well as an environment variable).

David J.

On 7 June 2018 at 04:01, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
wrote:

> On 6/6/18 13:20, Konstantin Knizhnik wrote:
> > Well, psql really allows to specify complete connection string with -d
> > options (although it is not mentioned in help).
> > But still I think that it is inconvenient to require user to write
> > complete connection string to be able to specify compression option,
> > while everybody prefer to use -h, -U, -p options to specify
> > correspondent components of connection string.
>
> I recommend that you avoid derailing your effort by hinging it on this
> issue. You can always add command-line options after the libpq support
> is in.
>

Strongly agree. Let libpq handle it first with the core protocol support
and connstr parsing, add convenience flags later.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

t

On 18.06.2018 23:34, Robbie Harwood wrote:
> t
>
>
> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>
>> On 06.06.2018 02:03, Thomas Munro wrote:
>>> On Wed, Jun 6, 2018 at 2:06 AM, Konstantin Knizhnik
>>> <k(dot)knizhnik(at)postgrespro(dot)ru> wrote:
>>>> Thank you for review. Updated version of the patch fixing all reported
>>>> problems is attached.
>>> Small problem on Windows[1]:
>>>
>>> C:\projects\postgresql\src\include\common/zpq_stream.h(17): error
>>> C2143: syntax error : missing ')' before '*'
>>> [C:\projects\postgresql\libpq.vcxproj]
>>> 2395
>>>
>>> You used ssize_t in zpq_stream.h, but Windows doesn't have that type.
>>> We have our own typedef in win32_port.h. Perhaps zpq_stream.c should
>>> include postgres.h/postgres_fe.h (depending on FRONTEND) like the
>>> other .c files in src/common, before it includes zpq_stream.h?
>>> Instead of "c.h".
>>>
>>> [1] https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.1106
>>>
>> Thank you very much for reporting the problem.
>> I attached new patch with include of postgres_fe.h added to zpq_stream.c
> Hello!
>
> Due to being in a similar place, I'm offering some code review. I'm
> excited that you're looking at throughput on the network stack - it's
> not usually what we think of in database performance. Here are some
> first thoughts, which have some overlap with what others have said on
> this thread already:
>
> ###
>
> This build still doesn't pass Windows:
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.2277
>
> You can find more about what the bot is doing here:
> http://cfbot.cputube.org/
Thank you.
Looks like I found the reason: Mkvsbuild.pm has to be patched.

>
> ###
>
> I have a few misgivings about pq_configure(), starting with the name.
> The *only* thing this function does is set up compression, so it's
> mis-named. (There's no point in making something generic unless it's
> needed - it's just confusing.)
Well, my intention was that this function *may* in future perform some
other configuration setting not related with compression.
And it is better to encapsulate this knowledge in pqcomm, rather than
make  postmaster (BackendStartup) worry about it.
But I can rename this function to pq_cofigure_compression() or whatever
your prefer.

> I also don't like that you've injected into the *startup* path - before
> authentication takes place. Fundamentally, authentication (if it
> happens) consists of exchanging some combination of short strings (e.g.,
> usernames) and binary blobs (e.g., keys). None of this will compress
> well, so I don't see it as worth performing this negotiation there - it
> can wait. It's also another message in every startup. I'd leave it to
> connection parameters, personally, but up to you.

From my point of view compression of libpq traffic is similar with SSL
and should be toggled at the same stage.
Definitely authentication parameter are not so large to be efficiently
compressed, by compression (may be in future password protected) can
somehow protect this data.
In any case I do not think that compression of authentication data may
have any influence on negotiation speed.
So I am not 100% sure that toggling compression just after receiving
startup package is the only right solution.
But I am not also convinced in that there is some better place where
compressor should be configured.
Do you have some concrete suggestions for it? In InitPostgres just after
PerformAuthentication ?
Also please notice that compression is useful not only for client-server
communication, but also for replication channels.
Right now it is definitely used in both cases, but if we move
pq_configure somewhere else, we should check that this code is invoked
in both for normal backends and walsender.

>
> ###
>
> Documentation! You're going to need it. There needs to be enough
> around for other people to implement the protocol (or if you prefer,
> enough for us to debug the protocol as it exists).
>
> In conjunction with that, please add information on how to set up
> compressed vs. uncompressed connections - similarly to how we've
> documentation on setting up TLS connection (though presumably compressed
> connection documentation will be shorter).

Sorry, definitely I will add documentation for configuring compression.

>
> ###
>
> Using terminology from https://facebook.github.io/zstd/zstd_manual.html :
>
> Right now you use streaming (ZSTD_{compress,decompress}Stream()) as the
> basis for your API. I think this is probably a mismatch for what we're
> doing here - we're doing one-shot compression/decompression of packets,
> not sending video or something.
>
> I think our use case is better served by the non-streaming interface, or
> what they call the "Simple API" (ZSTD_{decompress,compress}()).
> Documentation suggests it may be worth it to keep an explicit context
> around and use that interface instead (i.e.,
> ZSTD_{compressCCTx,decompressDCtx}()), but that's something you'll have
> to figure out.
>
> You may find making this change helpful for addressing the next issue.

Sorry, but here I completely disagree with you.
What we are doing is really streaming compression, not compression of
individual messages/packages.
Yes, it is not a video, but actually COPY data has the same nature as
video data.
The main benefit of streaming compression is that we can use the same
dictionary for compressing all messages (and adjust this dictionary
based on new data).
We do not need to write dictionary and separate header for each record.
Otherwize compression of libpq messages will be completely useless:
typical size of message is too short to be efficiently compressed. The
main drawback of streaming compression is that you can not decompress
some particular message without decompression of all previous messages.
This is why streaming compression can not be used to compress database
pages  (as it is done in CFS, provided in PostgresPro EE). But for libpq
it is no needed.

> ###
>
> I don't like where you've put the entry points to the compression logic:
> it's a layering violation. A couple people have expressed similar
> reservations I think, so let me see if I can explain using
> `pqsecure_read()` as an example. In pseudocode, `pqsecure_read()` looks
> like this:
>
> if conn->is_tls:
> n = tls_read(conn, ptr, len)
> else:
> n = pqsecure_raw_read(conn, ptr, len)
> return n
>
> I want to see this extended by your code to something like:
>
> if conn->is_tls:
> n = tls_read(conn, ptr, len)
> else:
> n = pqsecure_raw_read(conn, ptr, len)
>
> if conn->is_compressed:
> n = decompress(ptr, n)
>
> return n
>
> In conjunction with the above change, this should also significantly
> reduce the size of the patch (I think).

Yes, it will simplify patch. But make libpq compression completely
useless (see my explanation above).
We need to use streaming compression, and to be able to use streaming
compression I have to pass function for fetching more data to
compression library.

>
> ###
>
> The compression flag has proven somewhat contentious, as you've already
> seen on this thread. I recommend removing it for now and putting it in
> a separate patch to be merged later, since it's separable.
>
> ###
>
> It's not worth flagging style violations in your code right now, but you
> should be aware that there are quite a few style and whitespace
> problems. In particular, please be sure that you're using hard tabs
> when appropriate, and that line lengths are fewer than 80 characters
> (unless they contain error messages), and that pointers are correctly
> declared (`void *arg`, not `void* arg`).
>
> ###

Ok, I will fix it.

>
> Thanks,
> --Robbie

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

Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:

> On 18.06.2018 23:34, Robbie Harwood wrote:
>
>> I also don't like that you've injected into the *startup* path -
>> before authentication takes place. Fundamentally, authentication (if
>> it happens) consists of exchanging some combination of short strings
>> (e.g., usernames) and binary blobs (e.g., keys). None of this will
>> compress well, so I don't see it as worth performing this negotiation
>> there - it can wait. It's also another message in every startup.
>> I'd leave it to connection parameters, personally, but up to you.
>
> From my point of view compression of libpq traffic is similar with SSL
> and should be toggled at the same stage.

But that's not what you're doing. This isn't where TLS gets toggled.

TLS negotiation happens as the very first packet: after completing the
TCP handshake, the client will send a TLS negotiation request. If it
doesn't happen there, it doesn't happen at all.

(You *could* configure it where TLS is toggled. This is, I think, not a
good idea. TLS encryption is a probe: the server can reject it, at
which point the client tears everything down and connects without TLS.
So if you do the same with compression, that's another point of tearing
down an starting over. The scaling on it isn't good either: if we add
another encryption method into the mix, you've doubled the number of
teardowns.)

> Definitely authentication parameter are not so large to be efficiently
> compressed, by compression (may be in future password protected) can
> somehow protect this data.
> In any case I do not think that compression of authentication data may
> have any influence on negotiation speed.
> So I am not 100% sure that toggling compression just after receiving
> startup package is the only right solution.
> But I am not also convinced in that there is some better place where
> compressor should be configured.
> Do you have some concrete suggestions for it? In InitPostgres just after
> PerformAuthentication ?

Hmm, let me try to explain this differently.

pq_configure() (as you've called it) shouldn't send a packet. At its
callsite, we *already know* whether we want to use compression - that's
what the port->use_compression option says. So there's no point in
having a negotiation there - it's already happened.

The other thing you do in pq_configure() is call zpq_create(), which
does a bunch of initialization for you. I am pretty sure that all of
this can be deferred until the first time you want to send a compressed
message - i.e., when compress()/decompress() is called for the first
time from *secure_read() or *secure_write().

> Also please notice that compression is useful not only for client-server
> communication, but also for replication channels.
> Right now it is definitely used in both cases, but if we move
> pq_configure somewhere else, we should check that this code is invoked
> in both for normal backends and walsender.

"We" meaning you, at the moment, since I don't think any of the rest of
us have set up tests with this code :)

If there's common code to be shared around, that's great. But it's not
imperative; in a lot of ways, the network stacks are very different from
each other, as I'm sure you've seen. Let's not have the desire for code
reuse get in the way of good, maintainable design.

>> Using terminology from https://facebook.github.io/zstd/zstd_manual.html :
>>
>> Right now you use streaming (ZSTD_{compress,decompress}Stream()) as the
>> basis for your API. I think this is probably a mismatch for what we're
>> doing here - we're doing one-shot compression/decompression of packets,
>> not sending video or something.
>>
>> I think our use case is better served by the non-streaming interface, or
>> what they call the "Simple API" (ZSTD_{decompress,compress}()).
>> Documentation suggests it may be worth it to keep an explicit context
>> around and use that interface instead (i.e.,
>> ZSTD_{compressCCTx,decompressDCtx}()), but that's something you'll have
>> to figure out.
>>
>> You may find making this change helpful for addressing the next issue.
>
> Sorry, but here I completely disagree with you.
> What we are doing is really streaming compression, not compression of
> individual messages/packages.
> Yes, it is not a video, but actually COPY data has the same nature as
> video data.
> The main benefit of streaming compression is that we can use the same
> dictionary for compressing all messages (and adjust this dictionary
> based on new data).
> We do not need to write dictionary and separate header for each record.
> Otherwize compression of libpq messages will be completely useless:
> typical size of message is too short to be efficiently compressed. The
> main drawback of streaming compression is that you can not decompress
> some particular message without decompression of all previous messages.
> This is why streaming compression can not be used to compress database
> pages  (as it is done in CFS, provided in PostgresPro EE). But for libpq
> it is no needed.

That makes sense, thanks. The zstd documentation doesn't articulate
that at all.

>> I don't like where you've put the entry points to the compression logic:
>> it's a layering violation. A couple people have expressed similar
>> reservations I think, so let me see if I can explain using
>> `pqsecure_read()` as an example. In pseudocode, `pqsecure_read()` looks
>> like this:
>>
>> if conn->is_tls:
>> n = tls_read(conn, ptr, len)
>> else:
>> n = pqsecure_raw_read(conn, ptr, len)
>> return n
>>
>> I want to see this extended by your code to something like:
>>
>> if conn->is_tls:
>> n = tls_read(conn, ptr, len)
>> else:
>> n = pqsecure_raw_read(conn, ptr, len)
>>
>> if conn->is_compressed:
>> n = decompress(ptr, n)
>>
>> return n
>>
>> In conjunction with the above change, this should also significantly
>> reduce the size of the patch (I think).
>
> Yes, it will simplify patch. But make libpq compression completely
> useless (see my explanation above).
> We need to use streaming compression, and to be able to use streaming
> compression I have to pass function for fetching more data to
> compression library.

I don't think you need that, even with the streaming API.

To make this very concrete, let's talk about ZSTD_decompressStream (I'm
pulling information from
https://facebook.github.io/zstd/zstd_manual.html#Chapter7 ).

Using the pseudocode I'm asking for above, the decompress() function
would look vaguely like this:

decompress(ptr, n)
ZSTD_inBuffer in = {0}
ZSTD_outBuffer out = {0}

in.src = ptr
in.size = n

while in.pos < in.size:
ret = ZSTD_decompressStream(out, in)
if ZSTD_isError(ret):
give_up()

memcpy(ptr, out.dst, out.pos)
return out.pos

(and compress() would follow a similar pattern, if we were to talk about
it).

Thanks,
--Robbie

On 20.06.2018 00:04, Robbie Harwood wrote:
> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>
>> On 18.06.2018 23:34, Robbie Harwood wrote:
>>
>>> I also don't like that you've injected into the *startup* path -
>>> before authentication takes place. Fundamentally, authentication (if
>>> it happens) consists of exchanging some combination of short strings
>>> (e.g., usernames) and binary blobs (e.g., keys). None of this will
>>> compress well, so I don't see it as worth performing this negotiation
>>> there - it can wait. It's also another message in every startup.
>>> I'd leave it to connection parameters, personally, but up to you.
>> From my point of view compression of libpq traffic is similar with SSL
>> and should be toggled at the same stage.
> But that's not what you're doing. This isn't where TLS gets toggled.
>
> TLS negotiation happens as the very first packet: after completing the
> TCP handshake, the client will send a TLS negotiation request. If it
> doesn't happen there, it doesn't happen at all.
>
> (You *could* configure it where TLS is toggled. This is, I think, not a
> good idea. TLS encryption is a probe: the server can reject it, at
> which point the client tears everything down and connects without TLS.
> So if you do the same with compression, that's another point of tearing
> down an starting over. The scaling on it isn't good either: if we add
> another encryption method into the mix, you've doubled the number of
> teardowns.)
Yes, you are right. There is special message for enabling TLS procotol.
But I do not think that the same think is needed for compression.
This is why I prefer to specify compression in connectoin options. So
compression may be enabled straight after processing of startup package.
Frankly speaking I still do no see reasons to postpone enabling
compression till some later moment.

>> Definitely authentication parameter are not so large to be efficiently
>> compressed, by compression (may be in future password protected) can
>> somehow protect this data.
>> In any case I do not think that compression of authentication data may
>> have any influence on negotiation speed.
>> So I am not 100% sure that toggling compression just after receiving
>> startup package is the only right solution.
>> But I am not also convinced in that there is some better place where
>> compressor should be configured.
>> Do you have some concrete suggestions for it? In InitPostgres just after
>> PerformAuthentication ?
> Hmm, let me try to explain this differently.
>
> pq_configure() (as you've called it) shouldn't send a packet. At its
> callsite, we *already know* whether we want to use compression - that's
> what the port->use_compression option says. So there's no point in
> having a negotiation there - it's already happened.

My idea was the following: client want to use compression. But server
may reject this attempt (for any reasons: it doesn't support it, has no
proper compression library,
do not want to spend CPU for decompression,...) Right now compression
algorithm is hardcoded. But in future client and server may negotiate to
choose proper compression protocol.
This is why I prefer to perform negotiation between client and server to
enable compression.

>
> The other thing you do in pq_configure() is call zpq_create(), which
> does a bunch of initialization for you. I am pretty sure that all of
> this can be deferred until the first time you want to send a compressed
> message - i.e., when compress()/decompress() is called for the first
> time from *secure_read() or *secure_write().
>
>> Also please notice that compression is useful not only for client-server
>> communication, but also for replication channels.
>> Right now it is definitely used in both cases, but if we move
>> pq_configure somewhere else, we should check that this code is invoked
>> in both for normal backends and walsender.
> "We" meaning you, at the moment, since I don't think any of the rest of
> us have set up tests with this code :)
>
> If there's common code to be shared around, that's great. But it's not
> imperative; in a lot of ways, the network stacks are very different from
> each other, as I'm sure you've seen. Let's not have the desire for code
> reuse get in the way of good, maintainable design.
>
>>> Using terminology from https://facebook.github.io/zstd/zstd_manual.html :
>>>
>>> Right now you use streaming (ZSTD_{compress,decompress}Stream()) as the
>>> basis for your API. I think this is probably a mismatch for what we're
>>> doing here - we're doing one-shot compression/decompression of packets,
>>> not sending video or something.
>>>
>>> I think our use case is better served by the non-streaming interface, or
>>> what they call the "Simple API" (ZSTD_{decompress,compress}()).
>>> Documentation suggests it may be worth it to keep an explicit context
>>> around and use that interface instead (i.e.,
>>> ZSTD_{compressCCTx,decompressDCtx}()), but that's something you'll have
>>> to figure out.
>>>
>>> You may find making this change helpful for addressing the next issue.
>> Sorry, but here I completely disagree with you.
>> What we are doing is really streaming compression, not compression of
>> individual messages/packages.
>> Yes, it is not a video, but actually COPY data has the same nature as
>> video data.
>> The main benefit of streaming compression is that we can use the same
>> dictionary for compressing all messages (and adjust this dictionary
>> based on new data).
>> We do not need to write dictionary and separate header for each record.
>> Otherwize compression of libpq messages will be completely useless:
>> typical size of message is too short to be efficiently compressed. The
>> main drawback of streaming compression is that you can not decompress
>> some particular message without decompression of all previous messages.
>> This is why streaming compression can not be used to compress database
>> pages  (as it is done in CFS, provided in PostgresPro EE). But for libpq
>> it is no needed.
> That makes sense, thanks. The zstd documentation doesn't articulate
> that at all.
>
>>> I don't like where you've put the entry points to the compression logic:
>>> it's a layering violation. A couple people have expressed similar
>>> reservations I think, so let me see if I can explain using
>>> `pqsecure_read()` as an example. In pseudocode, `pqsecure_read()` looks
>>> like this:
>>>
>>> if conn->is_tls:
>>> n = tls_read(conn, ptr, len)
>>> else:
>>> n = pqsecure_raw_read(conn, ptr, len)
>>> return n
>>>
>>> I want to see this extended by your code to something like:
>>>
>>> if conn->is_tls:
>>> n = tls_read(conn, ptr, len)
>>> else:
>>> n = pqsecure_raw_read(conn, ptr, len)
>>>
>>> if conn->is_compressed:
>>> n = decompress(ptr, n)
>>>
>>> return n
>>>
>>> In conjunction with the above change, this should also significantly
>>> reduce the size of the patch (I think).
>> Yes, it will simplify patch. But make libpq compression completely
>> useless (see my explanation above).
>> We need to use streaming compression, and to be able to use streaming
>> compression I have to pass function for fetching more data to
>> compression library.
> I don't think you need that, even with the streaming API.
>
> To make this very concrete, let's talk about ZSTD_decompressStream (I'm
> pulling information from
> https://facebook.github.io/zstd/zstd_manual.html#Chapter7 ).
>
> Using the pseudocode I'm asking for above, the decompress() function
> would look vaguely like this:
>
> decompress(ptr, n)
> ZSTD_inBuffer in = {0}
> ZSTD_outBuffer out = {0}
>
> in.src = ptr
> in.size = n
>
> while in.pos < in.size:
> ret = ZSTD_decompressStream(out, in)
> if ZSTD_isError(ret):
> give_up()
>
> memcpy(ptr, out.dst, out.pos)
> return out.pos
>
> (and compress() would follow a similar pattern, if we were to talk about
> it).
It will not work in this way. We do not know how much input data we need
to be able to decompress message.
So loop should be something like this:

decompress(ptr, n)
ZSTD_inBuffer in = {0}
ZSTD_outBuffer out = {0}

in.src = ptr
in.size = n

while true
ret = ZSTD_decompressStream(out, in)
if ZSTD_isError(ret):
give_up()
if out.pos != 0
// if we deomcpress soemthing
return out.pos;
read_input(in);

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

Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:

> On 20.06.2018 00:04, Robbie Harwood wrote:
>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>> On 18.06.2018 23:34, Robbie Harwood wrote:
>>>
>>>> I also don't like that you've injected into the *startup* path -
>>>> before authentication takes place. Fundamentally, authentication
>>>> (if it happens) consists of exchanging some combination of short
>>>> strings (e.g., usernames) and binary blobs (e.g., keys). None of
>>>> this will compress well, so I don't see it as worth performing this
>>>> negotiation there - it can wait. It's also another message in
>>>> every startup. I'd leave it to connection parameters, personally,
>>>> but up to you.
>>>
>>> From my point of view compression of libpq traffic is similar with
>>> SSL and should be toggled at the same stage.
>>
>> But that's not what you're doing. This isn't where TLS gets toggled.
>>
>> TLS negotiation happens as the very first packet: after completing
>> the TCP handshake, the client will send a TLS negotiation request.
>> If it doesn't happen there, it doesn't happen at all.
>>
>> (You *could* configure it where TLS is toggled. This is, I think,
>> not a good idea. TLS encryption is a probe: the server can reject
>> it, at which point the client tears everything down and connects
>> without TLS. So if you do the same with compression, that's another
>> point of tearing down an starting over. The scaling on it isn't good
>> either: if we add another encryption method into the mix, you've
>> doubled the number of teardowns.)
>
> Yes, you are right. There is special message for enabling TLS
> procotol. But I do not think that the same think is needed for
> compression. This is why I prefer to specify compression in
> connectoin options. So compression may be enabled straight after
> processing of startup package. Frankly speaking I still do no see
> reasons to postpone enabling compression till some later moment.

I'm arguing for connection option only (with no additional negotiation
round-trip). See below.

>>> Definitely authentication parameter are not so large to be
>>> efficiently compressed, by compression (may be in future password
>>> protected) can somehow protect this data. In any case I do not
>>> think that compression of authentication data may have any influence
>>> on negotiation speed. So I am not 100% sure that toggling
>>> compression just after receiving startup package is the only right
>>> solution. But I am not also convinced in that there is some better
>>> place where compressor should be configured. Do you have some
>>> concrete suggestions for it? In InitPostgres just after
>>> PerformAuthentication ?
>>
>> Hmm, let me try to explain this differently.
>>
>> pq_configure() (as you've called it) shouldn't send a packet. At its
>> callsite, we *already know* whether we want to use compression -
>> that's what the port->use_compression option says. So there's no
>> point in having a negotiation there - it's already happened.
>
> My idea was the following: client want to use compression. But server
> may reject this attempt (for any reasons: it doesn't support it, has
> no proper compression library, do not want to spend CPU for
> decompression,...) Right now compression algorithm is hardcoded. But
> in future client and server may negotiate to choose proper compression
> protocol. This is why I prefer to perform negotiation between client
> and server to enable compression.

Well, for negotiation you could put the name of the algorithm you want
in the startup. It doesn't have to be a boolean for compression, and
then you don't need an additional round-trip.

>>>> I don't like where you've put the entry points to the compression
>>>> logic: it's a layering violation. A couple people have expressed
>>>> similar reservations I think, so let me see if I can explain using
>>>> `pqsecure_read()` as an example. In pseudocode, `pqsecure_read()`
>>>> looks like this:
>>>>
>>>> if conn->is_tls:
>>>> n = tls_read(conn, ptr, len)
>>>> else:
>>>> n = pqsecure_raw_read(conn, ptr, len)
>>>> return n
>>>>
>>>> I want to see this extended by your code to something like:
>>>>
>>>> if conn->is_tls:
>>>> n = tls_read(conn, ptr, len)
>>>> else:
>>>> n = pqsecure_raw_read(conn, ptr, len)
>>>>
>>>> if conn->is_compressed:
>>>> n = decompress(ptr, n)
>>>>
>>>> return n
>>>>
>>>> In conjunction with the above change, this should also
>>>> significantly reduce the size of the patch (I think).
>>>
>>> Yes, it will simplify patch. But make libpq compression completely
>>> useless (see my explanation above). We need to use streaming
>>> compression, and to be able to use streaming compression I have to
>>> pass function for fetching more data to compression library.
>>
>> I don't think you need that, even with the streaming API.
>>
>> To make this very concrete, let's talk about ZSTD_decompressStream (I'm
>> pulling information from
>> https://facebook.github.io/zstd/zstd_manual.html#Chapter7 ).
>>
>> Using the pseudocode I'm asking for above, the decompress() function
>> would look vaguely like this:
>>
>> decompress(ptr, n)
>> ZSTD_inBuffer in = {0}
>> ZpSTD_outBuffer out = {0}
>>
>> in.src = ptr
>> in.size = n
>>
>> while in.pos < in.size:
>> ret = ZSTD_decompressStream(out, in)
>> if ZSTD_isError(ret):
>> give_up()
>>
>> memcpy(ptr, out.dst, out.pos)
>> return out.pos
>>
>> (and compress() would follow a similar pattern, if we were to talk
>> about it).
>
> It will not work in this way. We do not know how much input data we
> need to be able to decompress message.

Well, that's a design decision you've made. You could put lengths on
chunks that are sent out - then you'd know exactly how much is needed.
(For instance, 4 bytes of network-order length followed by a complete
payload.) Then you'd absolutely know whether you have enough to
decompress or not.

> So loop should be something like this:
>
> decompress(ptr, n)
> ZSTD_inBuffer in = {0}
> ZSTD_outBuffer out = {0}
>
> in.src = ptr
> in.size = n
>
> while true
> ret = ZSTD_decompressStream(out, in)
> if ZSTD_isError(ret):
> give_up()
> if out.pos != 0
> // if we deomcpress soemthing
> return out.pos;
> read_input(in);

The last line is what I'm taking issue with. The interface we have
already in postgres's network code has a notion of partial reads, or
that reads might not return data. (Same with writing and partial
writes.) So you'd buffer what compressed data you have and return -
this is the preferred idiom so that we don't block or spin on a
nonblocking socket.

This is how the TLS code works already. Look at, for instance,
pgtls_read(). If we get back SSL_ERROR_WANT_READ (i.e., not enough data
to decrypt), we return no data and wait until the socket becomes
readable again.

Thanks,
--Robbie

On 20.06.2018 23:34, Robbie Harwood wrote:
> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>
>
> My idea was the following: client want to use compression. But server
> may reject this attempt (for any reasons: it doesn't support it, has
> no proper compression library, do not want to spend CPU for
> decompression,...) Right now compression algorithm is hardcoded. But
> in future client and server may negotiate to choose proper compression
> protocol. This is why I prefer to perform negotiation between client
> and server to enable compression.
> Well, for negotiation you could put the name of the algorithm you want
> in the startup. It doesn't have to be a boolean for compression, and
> then you don't need an additional round-trip.

Sorry, I can only repeat arguments I already mentioned:
- in future it may be possible to specify compression algorithm
- even with boolean compression option server may have some reasons to
reject client's request to use compression

Extra flexibility is always good thing if it doesn't cost too much. And
extra round of negotiation in case of enabling compression seems to me
not to be a high price for it.

>
> Well, that's a design decision you've made. You could put lengths on
> chunks that are sent out - then you'd know exactly how much is needed.
> (For instance, 4 bytes of network-order length followed by a complete
> payload.) Then you'd absolutely know whether you have enough to
> decompress or not.

Do you really suggest to send extra header for each chunk of data?
Please notice that chunk can be as small as one message: dozen of bytes
because libpq is used for client-server communication with request-reply
pattern.

Frankly speaking I do not completely understand the source of your concern.
My primary idea was to preseve behavior of libpq function as much as
possible, so there is no need to rewrite all places in Postgres code
when them are used.
It seems to me that I succeed in reaching this goal. Incase of enabled
compression zpq_stream functions (zpq-read/write) are used instead of
(pq)secure_read/write and in turn are using them to fetch/send more
data. I do not see any bad flaws, encapsulation violation or some other
problems in such solution.

So before discussing some alternative ways of embedding compression in
libpq, I will want to understand what's wrong with this approach.

>> So loop should be something like this:
>>
>> decompress(ptr, n)
>> ZSTD_inBuffer in = {0}
>> ZSTD_outBuffer out = {0}
>>
>> in.src = ptr
>> in.size = n
>>
>> while true
>> ret = ZSTD_decompressStream(out, in)
>> if ZSTD_isError(ret):
>> give_up()
>> if out.pos != 0
>> // if we deomcpress soemthing
>> return out.pos;
>> read_input(in);
> The last line is what I'm taking issue with. The interface we have
> already in postgres's network code has a notion of partial reads, or
> that reads might not return data. (Same with writing and partial
> writes.) So you'd buffer what compressed data you have and return -
> this is the preferred idiom so that we don't block or spin on a
> nonblocking socket.
If socket is in non-blocking mode and there is available data, then
secure_read  function will also immediately return 0.
The pseudocode above is not quite correct. Let me show the real
implementation of zpq_read:

ssize_t
zpq_read(ZpqStream *zs, void *buf, size_t size, size_t *processed)
{
    ssize_t rc;
    ZSTD_outBuffer out;
    out.dst = buf;
    out.pos = 0;
    out.size = size;

    while (1)
    {
        rc = ZSTD_decompressStream(zs->rx_stream, &out, &zs->rx);
        if (ZSTD_isError(rc))
        {
            zs->rx_error = ZSTD_getErrorName(rc);
            return ZPQ_DECOMPRESS_ERROR;
        }
        /* Return result if we fill requested amount of bytes or read
operation was performed */
        if (out.pos != 0)
        {
            zs->rx_total_raw += out.pos;
            return out.pos;
        }
        if (zs->rx.pos == zs->rx.size)
        {
            zs->rx.pos = zs->rx.size = 0; /* Reset rx buffer */
        }
        rc = zs->rx_func(zs->arg, (char*)zs->rx.src + zs->rx.size,
ZPQ_BUFFER_SIZE - zs->rx.size);
        if (rc > 0) /* read fetches some data */
        {
            zs->rx.size += rc;
            zs->rx_total += rc;
        }
        else /* read failed */
        {
            *processed = out.pos;
            zs->rx_total_raw += out.pos;
            return rc;
        }
    }
}

Sorry, but I have spent quite enough time trying to provide the same
behavior of zpq_read/write as of secure_read/write both for blocking and
non-blocking mode.
And I hope that now it is preserved. And frankly speaking I do not see
much differences of this approach with supporting TLS.
Current implementation allows to combine compression with TLS and in
some cases it may be really useful.

>
> This is how the TLS code works already. Look at, for instance,
> pgtls_read(). If we get back SSL_ERROR_WANT_READ (i.e., not enough data
> to decrypt), we return no data and wait until the socket becomes
> readable again.
>
> Thanks,
> --Robbie

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

Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:

> On 20.06.2018 23:34, Robbie Harwood wrote:
>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>
>>
>> My idea was the following: client want to use compression. But server
>> may reject this attempt (for any reasons: it doesn't support it, has
>> no proper compression library, do not want to spend CPU for
>> decompression,...) Right now compression algorithm is hardcoded. But
>> in future client and server may negotiate to choose proper compression
>> protocol. This is why I prefer to perform negotiation between client
>> and server to enable compression.
>> Well, for negotiation you could put the name of the algorithm you want
>> in the startup. It doesn't have to be a boolean for compression, and
>> then you don't need an additional round-trip.
>
> Sorry, I can only repeat arguments I already mentioned:
> - in future it may be possible to specify compression algorithm
> - even with boolean compression option server may have some reasons to
> reject client's request to use compression
>
> Extra flexibility is always good thing if it doesn't cost too much. And
> extra round of negotiation in case of enabling compression seems to me
> not to be a high price for it.

You already have this flexibility even without negotiation. I don't
want you to lose your flexibility. Protocol looks like this:

- Client sends connection option "compression" with list of algorithms
it wants to use (comma-separated, or something).

- First packet that the server can compress one of those algorithms (or
none, if it doesn't want to turn on compression).

No additional round-trips needed.

>> Well, that's a design decision you've made. You could put lengths on
>> chunks that are sent out - then you'd know exactly how much is needed.
>> (For instance, 4 bytes of network-order length followed by a complete
>> payload.) Then you'd absolutely know whether you have enough to
>> decompress or not.
>
> Do you really suggest to send extra header for each chunk of data?
> Please notice that chunk can be as small as one message: dozen of bytes
> because libpq is used for client-server communication with request-reply
> pattern.

I want you to think critically about your design. I *really* don't want
to design it for you - I have enough stuff to be doing. But again, the
design I gave you doesn't necessarily need that - you just need to
properly buffer incomplete data.

> Frankly speaking I do not completely understand the source of your
> concern. My primary idea was to preseve behavior of libpq function as
> much as possible, so there is no need to rewrite all places in
> Postgres code when them are used. It seems to me that I succeed in
> reaching this goal. Incase of enabled compression zpq_stream functions
> (zpq-read/write) are used instead of (pq)secure_read/write and in turn
> are using them to fetch/send more data. I do not see any bad flaws,
> encapsulation violation or some other problems in such solution.
>
> So before discussing some alternative ways of embedding compression in
> libpq, I will want to understand what's wrong with this approach.

You're destroying the existing model for no reason. If you needed to, I
could understand some argument for the way you've done it, but what I've
tried to tell you is that you don't need to do so. It's longer this
way, and it *significantly* complicates the (already difficult to reason
about) connection state machine.

I get that rewriting code can be obnoxious, and it feels like a waste of
time when we have to do so. (I've been there; I'm on version 19 of my
postgres patchset.)

>>> So loop should be something like this:
>>>
>>> decompress(ptr, n)
>>> ZSTD_inBuffer in = {0}
>>> ZSTD_outBuffer out = {0}
>>>
>>> in.src = ptr
>>> in.size = n
>>>
>>> while true
>>> ret = ZSTD_decompressStream(out, in)
>>> if ZSTD_isError(ret):
>>> give_up()
>>> if out.pos != 0
>>> // if we deomcpress soemthing
>>> return out.pos;
>>> read_input(in);
>>
>> The last line is what I'm taking issue with. The interface we have
>> already in postgres's network code has a notion of partial reads, or
>> that reads might not return data. (Same with writing and partial
>> writes.) So you'd buffer what compressed data you have and return -
>> this is the preferred idiom so that we don't block or spin on a
>> nonblocking socket.
>
> If socket is in non-blocking mode and there is available data, then
> secure_read  function will also immediately return 0.
> The pseudocode above is not quite correct. Let me show the real
> implementation of zpq_read:
>
> ssize_t
> zpq_read(ZpqStream *zs, void *buf, size_t size, size_t *processed)
> {
>     ssize_t rc;
>     ZSTD_outBuffer out;
>     out.dst = buf;
>     out.pos = 0;
>     out.size = size;
>
>     while (1)
>     {
>         rc = ZSTD_decompressStream(zs->rx_stream, &out, &zs->rx);
>         if (ZSTD_isError(rc))
>         {
>             zs->rx_error = ZSTD_getErrorName(rc);
>             return ZPQ_DECOMPRESS_ERROR;
>         }
>         /* Return result if we fill requested amount of bytes or read
> operation was performed */
>         if (out.pos != 0)
>         {
>             zs->rx_total_raw += out.pos;
>             return out.pos;
>         }
>         if (zs->rx.pos == zs->rx.size)
>         {
>             zs->rx.pos = zs->rx.size = 0; /* Reset rx buffer */
>         }
>         rc = zs->rx_func(zs->arg, (char*)zs->rx.src + zs->rx.size,
> ZPQ_BUFFER_SIZE - zs->rx.size);
>         if (rc > 0) /* read fetches some data */
>         {
>             zs->rx.size += rc;
>             zs->rx_total += rc;
>         }
>         else /* read failed */
>         {
>             *processed = out.pos;
>             zs->rx_total_raw += out.pos;
>             return rc;
>         }
>     }
> }
>
> Sorry, but I have spent quite enough time trying to provide the same
> behavior of zpq_read/write as of secure_read/write both for blocking and
> non-blocking mode.
> And I hope that now it is preserved. And frankly speaking I do not see
> much differences of this approach with supporting TLS.
>
> Current implementation allows to combine compression with TLS and in
> some cases it may be really useful.

The bottom line, though, is that I cannot recommend the code for
committer as long you have it plumbed like this. -1.

Thanks,
--Robbie

On 21.06.2018 17:56, Robbie Harwood wrote:
> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>
>> On 20.06.2018 23:34, Robbie Harwood wrote:
>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>
>>>
>>> My idea was the following: client want to use compression. But server
>>> may reject this attempt (for any reasons: it doesn't support it, has
>>> no proper compression library, do not want to spend CPU for
>>> decompression,...) Right now compression algorithm is hardcoded. But
>>> in future client and server may negotiate to choose proper compression
>>> protocol. This is why I prefer to perform negotiation between client
>>> and server to enable compression.
>>> Well, for negotiation you could put the name of the algorithm you want
>>> in the startup. It doesn't have to be a boolean for compression, and
>>> then you don't need an additional round-trip.
>> Sorry, I can only repeat arguments I already mentioned:
>> - in future it may be possible to specify compression algorithm
>> - even with boolean compression option server may have some reasons to
>> reject client's request to use compression
>>
>> Extra flexibility is always good thing if it doesn't cost too much. And
>> extra round of negotiation in case of enabling compression seems to me
>> not to be a high price for it.
> You already have this flexibility even without negotiation. I don't
> want you to lose your flexibility. Protocol looks like this:
>
> - Client sends connection option "compression" with list of algorithms
> it wants to use (comma-separated, or something).
>
> - First packet that the server can compress one of those algorithms (or
> none, if it doesn't want to turn on compression).
>
> No additional round-trips needed.

This is exactly how it works now...
Client includes compression option in connection string and server
replies with special message ('Z') if it accepts request to compress
traffic between this client and server.
I do not know whether sending such message can be considered as
"round-trip" or not, but I do not want to loose possibility to make
decision at server whether to use compression or not.

>
>>> Well, that's a design decision you've made. You could put lengths on
>>> chunks that are sent out - then you'd know exactly how much is needed.
>>> (For instance, 4 bytes of network-order length followed by a complete
>>> payload.) Then you'd absolutely know whether you have enough to
>>> decompress or not.
>> Do you really suggest to send extra header for each chunk of data?
>> Please notice that chunk can be as small as one message: dozen of bytes
>> because libpq is used for client-server communication with request-reply
>> pattern.
> I want you to think critically about your design. I *really* don't want
> to design it for you - I have enough stuff to be doing. But again, the
> design I gave you doesn't necessarily need that - you just need to
> properly buffer incomplete data.

Right now secure_read may return any number of available bytes. But in
case of using streaming compression, it can happen that available number
of bytes is not enough to perform decompression. This is why we may need
to try to fetch additional portion of data. This is how zpq_stream is
working now.

I do not understand how it is possible to implement in different way and
what is wrong with current implementation.
>
>> Frankly speaking I do not completely understand the source of your
>> concern. My primary idea was to preseve behavior of libpq function as
>> much as possible, so there is no need to rewrite all places in
>> Postgres code when them are used. It seems to me that I succeed in
>> reaching this goal. Incase of enabled compression zpq_stream functions
>> (zpq-read/write) are used instead of (pq)secure_read/write and in turn
>> are using them to fetch/send more data. I do not see any bad flaws,
>> encapsulation violation or some other problems in such solution.
>>
>> So before discussing some alternative ways of embedding compression in
>> libpq, I will want to understand what's wrong with this approach.
> You're destroying the existing model for no reason.

Why? Sorry, I really do not understand why adding compression in this
way breaks exited model.
Can you please explain it to me once again.

> If you needed to, I
> could understand some argument for the way you've done it, but what I've
> tried to tell you is that you don't need to do so. It's longer this
> way, and it *significantly* complicates the (already difficult to reason
> about) connection state machine.
>
> I get that rewriting code can be obnoxious, and it feels like a waste of
> time when we have to do so. (I've been there; I'm on version 19 of my
> postgres patchset.)

I am not against rewriting code many times, but first I should
understand the problem which needs to be solved.

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

Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:

> On 21.06.2018 17:56, Robbie Harwood wrote:
>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>> On 20.06.2018 23:34, Robbie Harwood wrote:
>>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>>
>>>> Well, that's a design decision you've made. You could put lengths
>>>> on chunks that are sent out - then you'd know exactly how much is
>>>> needed. (For instance, 4 bytes of network-order length followed by
>>>> a complete payload.) Then you'd absolutely know whether you have
>>>> enough to decompress or not.
>>>
>>> Do you really suggest to send extra header for each chunk of data?
>>> Please notice that chunk can be as small as one message: dozen of
>>> bytes because libpq is used for client-server communication with
>>> request-reply pattern.
>>
>> I want you to think critically about your design. I *really* don't
>> want to design it for you - I have enough stuff to be doing. But
>> again, the design I gave you doesn't necessarily need that - you just
>> need to properly buffer incomplete data.
>
> Right now secure_read may return any number of available bytes. But in
> case of using streaming compression, it can happen that available
> number of bytes is not enough to perform decompression. This is why we
> may need to try to fetch additional portion of data. This is how
> zpq_stream is working now.

No, you need to buffer and wait until you're called again. Which is to
say: decompress() shouldn't call secure_read(). secure_read() should
call decompress().

> I do not understand how it is possible to implement in different way
> and what is wrong with current implementation.

The most succinct thing I can say is: absolutely don't modify
pq_recvbuf(). I gave you pseudocode for how to do that. All of your
logic should be *below* the secure_read/secure_write functions.

I cannot approve code that modifies pq_recvbuf() in the manner you
currently do.

>>>> My idea was the following: client want to use compression. But
>>>> server may reject this attempt (for any reasons: it doesn't support
>>>> it, has no proper compression library, do not want to spend CPU for
>>>> decompression,...) Right now compression algorithm is
>>>> hardcoded. But in future client and server may negotiate to choose
>>>> proper compression protocol. This is why I prefer to perform
>>>> negotiation between client and server to enable compression. Well,
>>>> for negotiation you could put the name of the algorithm you want in
>>>> the startup. It doesn't have to be a boolean for compression, and
>>>> then you don't need an additional round-trip.
>>>
>>> Sorry, I can only repeat arguments I already mentioned:
>>> - in future it may be possible to specify compression algorithm
>>> - even with boolean compression option server may have some reasons to
>>> reject client's request to use compression
>>>
>>> Extra flexibility is always good thing if it doesn't cost too
>>> much. And extra round of negotiation in case of enabling compression
>>> seems to me not to be a high price for it.
>>
>> You already have this flexibility even without negotiation. I don't
>> want you to lose your flexibility. Protocol looks like this:
>>
>> - Client sends connection option "compression" with list of
>> algorithms it wants to use (comma-separated, or something).
>>
>> - First packet that the server can compress one of those algorithms
>> (or none, if it doesn't want to turn on compression).
>>
>> No additional round-trips needed.
>
> This is exactly how it works now... Client includes compression
> option in connection string and server replies with special message
> ('Z') if it accepts request to compress traffic between this client
> and server.

No, it's not. You don't need this message. If the server receives a
compression request, it should just turn compression on (or not), and
then have the client figure out whether it got compression back. This
is of course made harder by your refusal to use packet framing, but
still shouldn't be particularly difficult.

Thanks,
--Robbie

On Thu, Jun 21, 2018 at 10:12:17AM +0300, Konstantin Knizhnik wrote:
> On 20.06.2018 23:34, Robbie Harwood wrote:
> >Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
> >Well, that's a design decision you've made. You could put lengths on
> >chunks that are sent out - then you'd know exactly how much is needed.
> >(For instance, 4 bytes of network-order length followed by a complete
> >payload.) Then you'd absolutely know whether you have enough to
> >decompress or not.
>
> Do you really suggest to send extra header for each chunk of data?
> Please notice that chunk can be as small as one message: dozen of bytes
> because libpq is used for client-server communication with request-reply
> pattern.

You must have lengths, yes, otherwise you're saying that the chosen
compression mechanism must itself provide framing.

I'm not that familiar with compression APIs and formats, but looking at
RFC1950 (zlib) for example I see no framing.

So I think you just have to have lengths.

Now, this being about compression, I understand that you might now want
to have 4-byte lengths, especially given that most messages will be
under 8KB. So use a varint encoding for the lengths.

Nico
--

On 21.06.2018 20:14, Robbie Harwood wrote:
> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>
>> On 21.06.2018 17:56, Robbie Harwood wrote:
>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>> On 20.06.2018 23:34, Robbie Harwood wrote:
>>>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>>>
>>>>> Well, that's a design decision you've made. You could put lengths
>>>>> on chunks that are sent out - then you'd know exactly how much is
>>>>> needed. (For instance, 4 bytes of network-order length followed by
>>>>> a complete payload.) Then you'd absolutely know whether you have
>>>>> enough to decompress or not.
>>>> Do you really suggest to send extra header for each chunk of data?
>>>> Please notice that chunk can be as small as one message: dozen of
>>>> bytes because libpq is used for client-server communication with
>>>> request-reply pattern.
>>> I want you to think critically about your design. I *really* don't
>>> want to design it for you - I have enough stuff to be doing. But
>>> again, the design I gave you doesn't necessarily need that - you just
>>> need to properly buffer incomplete data.
>> Right now secure_read may return any number of available bytes. But in
>> case of using streaming compression, it can happen that available
>> number of bytes is not enough to perform decompression. This is why we
>> may need to try to fetch additional portion of data. This is how
>> zpq_stream is working now.
> No, you need to buffer and wait until you're called again. Which is to
> say: decompress() shouldn't call secure_read(). secure_read() should
> call decompress().
>

I this case I will have to implement this code twice: both for backend
and frontend, i.e. for secure_read/secure_write and
pqsecure_read/pqsecure_write.
Frankly speaking i was very upset by design of libpq communication layer
in Postgres: there are two different implementations of almost the same
stuff for cbackend and frontend.
I do not see any meaningful argument for it except "historical reasons".
The better decision was to encapsulate socket communication layer (and
some other system dependent stuff) in SAL (system abstraction layer) and
use it both in backend and frontend.

By passing secure_read/pqsecure_read functions to zpq_stream I managed
to avoid such code duplication at least for compression.

>> I do not understand how it is possible to implement in different way
>> and what is wrong with current implementation.
> The most succinct thing I can say is: absolutely don't modify
> pq_recvbuf(). I gave you pseudocode for how to do that. All of your
> logic should be *below* the secure_read/secure_write functions.
>
> I cannot approve code that modifies pq_recvbuf() in the manner you
> currently do.

Well. I understand you arguments.
But please also consider my argument above (about avoiding code
duplication).

In any case, secure_read function is called only from pq_recvbuf() as
well as pqsecure_read is called only from pqReadData.
So I do not think principle difference in handling compression in
secure_read or pq_recvbuf functions and do not understand why it is
"destroying the existing model".

Frankly speaking, I will really like to destroy existed model, moving
all system dependent stuff in Postgres to SAL and avoid this awful mix
of code
sharing and duplication between backend and frontend. But it is a
another story and I do not want to discuss it here.

If we are speaking about the "right design", then neither your
suggestion, neither my implementation are good and I do not see
principle differences between them.
The right approach is using "decorator pattern": this is how streams are
designed in .Net/Java. You can construct pipe of "secure", "compressed"
and whatever else streams.
Yes, it is first of all pattern for object-oriented approach and
Postgres is implemented in C. But it is actually possible to use OO
approach in pure C (X-Windows!).
But once again, this discussion may lead other too far away from the
topic of libpq compression.

As far as I already wrote, the main  points of my design were:
1. Minimize changes in Postgres code
2. Avoid code duplication
3. Provide abstract (compression stream) which can be used somewhere
else except libpq itself.

>
>>>>> My idea was the following: client want to use compression. But
>>>>> server may reject this attempt (for any reasons: it doesn't support
>>>>> it, has no proper compression library, do not want to spend CPU for
>>>>> decompression,...) Right now compression algorithm is
>>>>> hardcoded. But in future client and server may negotiate to choose
>>>>> proper compression protocol. This is why I prefer to perform
>>>>> negotiation between client and server to enable compression. Well,
>>>>> for negotiation you could put the name of the algorithm you want in
>>>>> the startup. It doesn't have to be a boolean for compression, and
>>>>> then you don't need an additional round-trip.
>>>> Sorry, I can only repeat arguments I already mentioned:
>>>> - in future it may be possible to specify compression algorithm
>>>> - even with boolean compression option server may have some reasons to
>>>> reject client's request to use compression
>>>>
>>>> Extra flexibility is always good thing if it doesn't cost too
>>>> much. And extra round of negotiation in case of enabling compression
>>>> seems to me not to be a high price for it.
>>> You already have this flexibility even without negotiation. I don't
>>> want you to lose your flexibility. Protocol looks like this:
>>>
>>> - Client sends connection option "compression" with list of
>>> algorithms it wants to use (comma-separated, or something).
>>>
>>> - First packet that the server can compress one of those algorithms
>>> (or none, if it doesn't want to turn on compression).
>>>
>>> No additional round-trips needed.
>> This is exactly how it works now... Client includes compression
>> option in connection string and server replies with special message
>> ('Z') if it accepts request to compress traffic between this client
>> and server.
> No, it's not. You don't need this message. If the server receives a
> compression request, it should just turn compression on (or not), and
> then have the client figure out whether it got compression back.
How it will managed to do it. It receives some reply and first of all it
should know whether it has to be decompressed or not.

> This
> is of course made harder by your refusal to use packet framing, but
> still shouldn't be particularly difficult.
But how?

>
> Thanks,
> --Robbie

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

On 22.06.2018 00:34, Nico Williams wrote:
> On Thu, Jun 21, 2018 at 10:12:17AM +0300, Konstantin Knizhnik wrote:
>> On 20.06.2018 23:34, Robbie Harwood wrote:
>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>> Well, that's a design decision you've made. You could put lengths on
>>> chunks that are sent out - then you'd know exactly how much is needed.
>>> (For instance, 4 bytes of network-order length followed by a complete
>>> payload.) Then you'd absolutely know whether you have enough to
>>> decompress or not.
>> Do you really suggest to send extra header for each chunk of data?
>> Please notice that chunk can be as small as one message: dozen of bytes
>> because libpq is used for client-server communication with request-reply
>> pattern.
> You must have lengths, yes, otherwise you're saying that the chosen
> compression mechanism must itself provide framing.
>
> I'm not that familiar with compression APIs and formats, but looking at
> RFC1950 (zlib) for example I see no framing.
>
> So I think you just have to have lengths.
>
> Now, this being about compression, I understand that you might now want
> to have 4-byte lengths, especially given that most messages will be
> under 8KB. So use a varint encoding for the lengths.
>
> Nico
No explicit framing and lengths are needed in case of using streaming
compression.
There can be certainly some kind of frames inside compression protocol
itself, but it is intrinsic of compression algorithm.

--

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

Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:

> On 21.06.2018 20:14, Robbie Harwood wrote:
>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>> On 21.06.2018 17:56, Robbie Harwood wrote:
>>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>>> On 20.06.2018 23:34, Robbie Harwood wrote:
>>>>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>>>>
>>>>>> Well, that's a design decision you've made. You could put
>>>>>> lengths on chunks that are sent out - then you'd know exactly how
>>>>>> much is needed. (For instance, 4 bytes of network-order length
>>>>>> followed by a complete payload.) Then you'd absolutely know
>>>>>> whether you have enough to decompress or not.
>>>>>
>>>>> Do you really suggest to send extra header for each chunk of data?
>>>>> Please notice that chunk can be as small as one message: dozen of
>>>>> bytes because libpq is used for client-server communication with
>>>>> request-reply pattern.
>>>>
>>>> I want you to think critically about your design. I *really* don't
>>>> want to design it for you - I have enough stuff to be doing. But
>>>> again, the design I gave you doesn't necessarily need that - you
>>>> just need to properly buffer incomplete data.
>>>
>>> Right now secure_read may return any number of available bytes. But
>>> in case of using streaming compression, it can happen that available
>>> number of bytes is not enough to perform decompression. This is why
>>> we may need to try to fetch additional portion of data. This is how
>>> zpq_stream is working now.
>>
>> No, you need to buffer and wait until you're called again. Which is
>> to say: decompress() shouldn't call secure_read(). secure_read()
>> should call decompress().
>
> I this case I will have to implement this code twice: both for backend
> and frontend, i.e. for secure_read/secure_write and
> pqsecure_read/pqsecure_write.

Likely, yes. You can see that this is how TLS does it (which you should
be using as a model, architecture-wise).

> Frankly speaking i was very upset by design of libpq communication
> layer in Postgres: there are two different implementations of almost
> the same stuff for cbackend and frontend.

Changing the codebases so that more could be shared is not necessarily a
bad idea; however, it is a separate change from compression.

>>> I do not understand how it is possible to implement in different way
>>> and what is wrong with current implementation.
>>
>> The most succinct thing I can say is: absolutely don't modify
>> pq_recvbuf(). I gave you pseudocode for how to do that. All of your
>> logic should be *below* the secure_read/secure_write functions.
>>
>> I cannot approve code that modifies pq_recvbuf() in the manner you
>> currently do.
>
> Well. I understand you arguments. But please also consider my
> argument above (about avoiding code duplication).
>
> In any case, secure_read function is called only from pq_recvbuf() as
> well as pqsecure_read is called only from pqReadData. So I do not
> think principle difference in handling compression in secure_read or
> pq_recvbuf functions and do not understand why it is "destroying the
> existing model".
>
> Frankly speaking, I will really like to destroy existed model, moving
> all system dependent stuff in Postgres to SAL and avoid this awful mix
> of code sharing and duplication between backend and frontend. But it
> is a another story and I do not want to discuss it here.

I understand you want to avoid code duplication. I will absolutely
agree that the current setup makes it difficult to share code between
postmaster and libpq clients. But the way I see it, you have two
choices:

1. Modify the code to make code sharing easier. Once this has been
done, *then* build a compression patch on top, with the nice new
architecture.

2. Leave the architecture as-is and add compression support.
(Optionally, you can make code sharing easier at a later point.)

Fundamentally, I think you value code sharing more than I do. So while
I might advocate for (2), you might personally prefer (1).

But right now you're not doing either of those.

> If we are speaking about the "right design", then neither your
> suggestion, neither my implementation are good and I do not see
> principle differences between them.
>
> The right approach is using "decorator pattern": this is how streams
> are designed in .Net/Java. You can construct pipe of "secure",
> "compressed" and whatever else streams.

I *strongly* disagree, but I don't think you're seriously suggesting
this.

>>>>>> My idea was the following: client want to use compression. But
>>>>>> server may reject this attempt (for any reasons: it doesn't support
>>>>>> it, has no proper compression library, do not want to spend CPU for
>>>>>> decompression,...) Right now compression algorithm is
>>>>>> hardcoded. But in future client and server may negotiate to choose
>>>>>> proper compression protocol. This is why I prefer to perform
>>>>>> negotiation between client and server to enable compression. Well,
>>>>>> for negotiation you could put the name of the algorithm you want in
>>>>>> the startup. It doesn't have to be a boolean for compression, and
>>>>>> then you don't need an additional round-trip.
>>>>> Sorry, I can only repeat arguments I already mentioned:
>>>>> - in future it may be possible to specify compression algorithm
>>>>> - even with boolean compression option server may have some reasons to
>>>>> reject client's request to use compression
>>>>>
>>>>> Extra flexibility is always good thing if it doesn't cost too
>>>>> much. And extra round of negotiation in case of enabling compression
>>>>> seems to me not to be a high price for it.
>>>> You already have this flexibility even without negotiation. I don't
>>>> want you to lose your flexibility. Protocol looks like this:
>>>>
>>>> - Client sends connection option "compression" with list of
>>>> algorithms it wants to use (comma-separated, or something).
>>>>
>>>> - First packet that the server can compress one of those algorithms
>>>> (or none, if it doesn't want to turn on compression).
>>>>
>>>> No additional round-trips needed.
>>> This is exactly how it works now... Client includes compression
>>> option in connection string and server replies with special message
>>> ('Z') if it accepts request to compress traffic between this client
>>> and server.
>>
>> No, it's not. You don't need this message. If the server receives a
>> compression request, it should just turn compression on (or not), and
>> then have the client figure out whether it got compression back.
>
> How it will managed to do it. It receives some reply and first of all
> it should know whether it has to be decompressed or not.

You can tell whether a message is compressed by looking at it. The way
the protocol works, every message has a type associated with it: a
single byte, like 'R', that says what kind of message it is.

Thanks,
--Robbie

On Fri, Jun 22, 2018 at 10:18:12AM +0300, Konstantin Knizhnik wrote:
> On 22.06.2018 00:34, Nico Williams wrote:
> >So I think you just have to have lengths.
> >
> >Now, this being about compression, I understand that you might now want
> >to have 4-byte lengths, especially given that most messages will be
> >under 8KB. So use a varint encoding for the lengths.
>
> No explicit framing and lengths are needed in case of using streaming
> compression.
> There can be certainly some kind of frames inside compression protocol
> itself, but it is intrinsic of compression algorithm.

I don't think that's generally true. It may be true of the compression
algorithm you're working with. This is fine, of course, but plugging in
other compression algorithms will require the authors to add framing.

On 22.06.2018 18:59, Robbie Harwood wrote:
> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>
>> On 21.06.2018 20:14, Robbie Harwood wrote:
>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>> On 21.06.2018 17:56, Robbie Harwood wrote:
>>>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>>>> On 20.06.2018 23:34, Robbie Harwood wrote:
>>>>>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>>>>>
>>>>>>> Well, that's a design decision you've made. You could put
>>>>>>> lengths on chunks that are sent out - then you'd know exactly how
>>>>>>> much is needed. (For instance, 4 bytes of network-order length
>>>>>>> followed by a complete payload.) Then you'd absolutely know
>>>>>>> whether you have enough to decompress or not.
>>>>>> Do you really suggest to send extra header for each chunk of data?
>>>>>> Please notice that chunk can be as small as one message: dozen of
>>>>>> bytes because libpq is used for client-server communication with
>>>>>> request-reply pattern.
>>>>> I want you to think critically about your design. I *really* don't
>>>>> want to design it for you - I have enough stuff to be doing. But
>>>>> again, the design I gave you doesn't necessarily need that - you
>>>>> just need to properly buffer incomplete data.
>>>> Right now secure_read may return any number of available bytes. But
>>>> in case of using streaming compression, it can happen that available
>>>> number of bytes is not enough to perform decompression. This is why
>>>> we may need to try to fetch additional portion of data. This is how
>>>> zpq_stream is working now.
>>> No, you need to buffer and wait until you're called again. Which is
>>> to say: decompress() shouldn't call secure_read(). secure_read()
>>> should call decompress().
>> I this case I will have to implement this code twice: both for backend
>> and frontend, i.e. for secure_read/secure_write and
>> pqsecure_read/pqsecure_write.
> Likely, yes. You can see that this is how TLS does it (which you should
> be using as a model, architecture-wise).
>
>> Frankly speaking i was very upset by design of libpq communication
>> layer in Postgres: there are two different implementations of almost
>> the same stuff for cbackend and frontend.
> Changing the codebases so that more could be shared is not necessarily a
> bad idea; however, it is a separate change from compression.
>
>>>> I do not understand how it is possible to implement in different way
>>>> and what is wrong with current implementation.
>>> The most succinct thing I can say is: absolutely don't modify
>>> pq_recvbuf(). I gave you pseudocode for how to do that. All of your
>>> logic should be *below* the secure_read/secure_write functions.
>>>
>>> I cannot approve code that modifies pq_recvbuf() in the manner you
>>> currently do.
>> Well. I understand you arguments. But please also consider my
>> argument above (about avoiding code duplication).
>>
>> In any case, secure_read function is called only from pq_recvbuf() as
>> well as pqsecure_read is called only from pqReadData. So I do not
>> think principle difference in handling compression in secure_read or
>> pq_recvbuf functions and do not understand why it is "destroying the
>> existing model".
>>
>> Frankly speaking, I will really like to destroy existed model, moving
>> all system dependent stuff in Postgres to SAL and avoid this awful mix
>> of code sharing and duplication between backend and frontend. But it
>> is a another story and I do not want to discuss it here.
> I understand you want to avoid code duplication. I will absolutely
> agree that the current setup makes it difficult to share code between
> postmaster and libpq clients. But the way I see it, you have two
> choices:
>
> 1. Modify the code to make code sharing easier. Once this has been
> done, *then* build a compression patch on top, with the nice new
> architecture.
>
> 2. Leave the architecture as-is and add compression support.
> (Optionally, you can make code sharing easier at a later point.)
>
> Fundamentally, I think you value code sharing more than I do. So while
> I might advocate for (2), you might personally prefer (1).
>
> But right now you're not doing either of those.
>
>> If we are speaking about the "right design", then neither your
>> suggestion, neither my implementation are good and I do not see
>> principle differences between them.
>>
>> The right approach is using "decorator pattern": this is how streams
>> are designed in .Net/Java. You can construct pipe of "secure",
>> "compressed" and whatever else streams.
> I *strongly* disagree, but I don't think you're seriously suggesting
> this.
>
>>>>>>> My idea was the following: client want to use compression. But
>>>>>>> server may reject this attempt (for any reasons: it doesn't support
>>>>>>> it, has no proper compression library, do not want to spend CPU for
>>>>>>> decompression,...) Right now compression algorithm is
>>>>>>> hardcoded. But in future client and server may negotiate to choose
>>>>>>> proper compression protocol. This is why I prefer to perform
>>>>>>> negotiation between client and server to enable compression. Well,
>>>>>>> for negotiation you could put the name of the algorithm you want in
>>>>>>> the startup. It doesn't have to be a boolean for compression, and
>>>>>>> then you don't need an additional round-trip.
>>>>>> Sorry, I can only repeat arguments I already mentioned:
>>>>>> - in future it may be possible to specify compression algorithm
>>>>>> - even with boolean compression option server may have some reasons to
>>>>>> reject client's request to use compression
>>>>>>
>>>>>> Extra flexibility is always good thing if it doesn't cost too
>>>>>> much. And extra round of negotiation in case of enabling compression
>>>>>> seems to me not to be a high price for it.
>>>>> You already have this flexibility even without negotiation. I don't
>>>>> want you to lose your flexibility. Protocol looks like this:
>>>>>
>>>>> - Client sends connection option "compression" with list of
>>>>> algorithms it wants to use (comma-separated, or something).
>>>>>
>>>>> - First packet that the server can compress one of those algorithms
>>>>> (or none, if it doesn't want to turn on compression).
>>>>>
>>>>> No additional round-trips needed.
>>>> This is exactly how it works now... Client includes compression
>>>> option in connection string and server replies with special message
>>>> ('Z') if it accepts request to compress traffic between this client
>>>> and server.
>>> No, it's not. You don't need this message. If the server receives a
>>> compression request, it should just turn compression on (or not), and
>>> then have the client figure out whether it got compression back.
>> How it will managed to do it. It receives some reply and first of all
>> it should know whether it has to be decompressed or not.
> You can tell whether a message is compressed by looking at it. The way
> the protocol works, every message has a type associated with it: a
> single byte, like 'R', that says what kind of message it is.

Compressed message can contain any sequence of bytes, including 'R':)

>
> Thanks,
> --Robbie

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

On 22.06.2018 19:05, Nico Williams wrote:
> On Fri, Jun 22, 2018 at 10:18:12AM +0300, Konstantin Knizhnik wrote:
>> On 22.06.2018 00:34, Nico Williams wrote:
>>> So I think you just have to have lengths.
>>>
>>> Now, this being about compression, I understand that you might now want
>>> to have 4-byte lengths, especially given that most messages will be
>>> under 8KB. So use a varint encoding for the lengths.
>> No explicit framing and lengths are needed in case of using streaming
>> compression.
>> There can be certainly some kind of frames inside compression protocol
>> itself, but it is intrinsic of compression algorithm.
> I don't think that's generally true. It may be true of the compression
> algorithm you're working with. This is fine, of course, but plugging in
> other compression algorithms will require the authors to add framing.

If compression algorithm supports streaming mode (and most of them
does), then you should not worry about frames.
And if compression algorithm doesn't support streaming mode, then it
should not be used for libpq traffic compression.

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

Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:

> On 22.06.2018 18:59, Robbie Harwood wrote:
>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>> On 21.06.2018 20:14, Robbie Harwood wrote:
>>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>>> On 21.06.2018 17:56, Robbie Harwood wrote:
>>>>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>>>>> On 20.06.2018 23:34, Robbie Harwood wrote:
>>>>>>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>>>>>>
>>>>>>>>> My idea was the following: client want to use compression. But
>>>>>>>>> server may reject this attempt (for any reasons: it doesn't
>>>>>>>>> support it, has no proper compression library, do not want to
>>>>>>>>> spend CPU for decompression,...) Right now compression
>>>>>>>>> algorithm is hardcoded. But in future client and server may
>>>>>>>>> negotiate to choose proper compression protocol. This is why
>>>>>>>>> I prefer to perform negotiation between client and server to
>>>>>>>>> enable compression.
>>>>>>>>
>>>>>>>> Well, for negotiation you could put the name of the algorithm
>>>>>>>> you want in the startup. It doesn't have to be a boolean for
>>>>>>>> compression, and then you don't need an additional round-trip.
>>>>>>>
>>>>>>> Sorry, I can only repeat arguments I already mentioned:
>>>>>>>
>>>>>>> - in future it may be possible to specify compression algorithm
>>>>>>>
>>>>>>> - even with boolean compression option server may have some
>>>>>>> reasons to reject client's request to use compression
>>>>>>>
>>>>>>> Extra flexibility is always good thing if it doesn't cost too
>>>>>>> much. And extra round of negotiation in case of enabling
>>>>>>> compression seems to me not to be a high price for it.
>>>>>>
>>>>>> You already have this flexibility even without negotiation. I
>>>>>> don't want you to lose your flexibility. Protocol looks like
>>>>>> this:
>>>>>>
>>>>>> - Client sends connection option "compression" with list of
>>>>>> algorithms it wants to use (comma-separated, or something).
>>>>>>
>>>>>> - First packet that the server can compress one of those algorithms
>>>>>> (or none, if it doesn't want to turn on compression).
>>>>>>
>>>>>> No additional round-trips needed.
>>>>>
>>>>> This is exactly how it works now... Client includes compression
>>>>> option in connection string and server replies with special
>>>>> message ('Z') if it accepts request to compress traffic between
>>>>> this client and server.
>>>>
>>>> No, it's not. You don't need this message. If the server receives
>>>> a compression request, it should just turn compression on (or not),
>>>> and then have the client figure out whether it got compression
>>>> back.
>>>
>>> How it will managed to do it. It receives some reply and first of
>>> all it should know whether it has to be decompressed or not.
>>
>> You can tell whether a message is compressed by looking at it. The
>> way the protocol works, every message has a type associated with it:
>> a single byte, like 'R', that says what kind of message it is.
>
> Compressed message can contain any sequence of bytes, including 'R':)

Then tag your messages with a type byte. Or do it the other way around
- look for the zstd framing within a message.

Please, try to work with me on this instead of fighting every design
change.

Thanks,
--Robbie

On 22.06.2018 20:56, Robbie Harwood wrote:
> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>
>> On 22.06.2018 18:59, Robbie Harwood wrote:
>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>> On 21.06.2018 20:14, Robbie Harwood wrote:
>>>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>>>> On 21.06.2018 17:56, Robbie Harwood wrote:
>>>>>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>>>>>> On 20.06.2018 23:34, Robbie Harwood wrote:
>>>>>>>>> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>>>>>>>>>
>>>>>>>>>> My idea was the following: client want to use compression. But
>>>>>>>>>> server may reject this attempt (for any reasons: it doesn't
>>>>>>>>>> support it, has no proper compression library, do not want to
>>>>>>>>>> spend CPU for decompression,...) Right now compression
>>>>>>>>>> algorithm is hardcoded. But in future client and server may
>>>>>>>>>> negotiate to choose proper compression protocol. This is why
>>>>>>>>>> I prefer to perform negotiation between client and server to
>>>>>>>>>> enable compression.
>>>>>>>>> Well, for negotiation you could put the name of the algorithm
>>>>>>>>> you want in the startup. It doesn't have to be a boolean for
>>>>>>>>> compression, and then you don't need an additional round-trip.
>>>>>>>> Sorry, I can only repeat arguments I already mentioned:
>>>>>>>>
>>>>>>>> - in future it may be possible to specify compression algorithm
>>>>>>>>
>>>>>>>> - even with boolean compression option server may have some
>>>>>>>> reasons to reject client's request to use compression
>>>>>>>>
>>>>>>>> Extra flexibility is always good thing if it doesn't cost too
>>>>>>>> much. And extra round of negotiation in case of enabling
>>>>>>>> compression seems to me not to be a high price for it.
>>>>>>> You already have this flexibility even without negotiation. I
>>>>>>> don't want you to lose your flexibility. Protocol looks like
>>>>>>> this:
>>>>>>>
>>>>>>> - Client sends connection option "compression" with list of
>>>>>>> algorithms it wants to use (comma-separated, or something).
>>>>>>>
>>>>>>> - First packet that the server can compress one of those algorithms
>>>>>>> (or none, if it doesn't want to turn on compression).
>>>>>>>
>>>>>>> No additional round-trips needed.
>>>>>> This is exactly how it works now... Client includes compression
>>>>>> option in connection string and server replies with special
>>>>>> message ('Z') if it accepts request to compress traffic between
>>>>>> this client and server.
>>>>> No, it's not. You don't need this message. If the server receives
>>>>> a compression request, it should just turn compression on (or not),
>>>>> and then have the client figure out whether it got compression
>>>>> back.
>>>> How it will managed to do it. It receives some reply and first of
>>>> all it should know whether it has to be decompressed or not.
>>> You can tell whether a message is compressed by looking at it. The
>>> way the protocol works, every message has a type associated with it:
>>> a single byte, like 'R', that says what kind of message it is.
>> Compressed message can contain any sequence of bytes, including 'R':)
> Then tag your messages with a type byte. Or do it the other way around
> - look for the zstd framing within a message.
>
> Please, try to work with me on this instead of fighting every design
> change.
Sorry, I do not want fighting.
I am always vote for peace and constructive dialog.

But it is hard to me to understand and accept your arguments.

I do not understand why secure_read function is better place for
handling compression than pq_recvbuf.
And why it is destroying existed model.
I already mentioned my arguments:
1. I want to use the same code for frontend and backend.
2. I think that streaming compression can be used not only for libpq.
This is why I tried to make zpq_stream independent from
communication layer and pass here callbacks for sending/receiving data.

If pq_recvbuf is not right place for performing decommpression, I can
introduce some other function,  like read_raw or something like that and
do decompression here. But I do not see much sense in it.

Concerning necessity of special message for acknowledging compression by
server: I also do not understand why you do not like idea to send some
message and what is wrong with it. What you are suggesting "then tag
your message" actually is the same as sending new message.
Because what is the difference between tag 'Z' and message with code 'Z'?

Sorry, but  I do not understand problems you are going to solve and do
not see any arguments except "I can not accept it".

> Thanks,
> --Robbie

On 18.06.2018 23:34, Robbie Harwood wrote:
>
> ###
>
> Documentation! You're going to need it. There needs to be enough
> around for other people to implement the protocol (or if you prefer,
> enough for us to debug the protocol as it exists).
>
> In conjunction with that, please add information on how to set up
> compressed vs. uncompressed connections - similarly to how we've
> documentation on setting up TLS connection (though presumably compressed
> connection documentation will be shorter).
>

Document protocol changes needed for libpq compression.

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

On 06/25/2018 05:32 AM, Konstantin Knizhnik wrote:
>
>
> On 18.06.2018 23:34, Robbie Harwood wrote:
>>
>> ###
>>
>> Documentation!  You're going to need it.  There needs to be enough
>> around for other people to implement the protocol (or if you prefer,
>> enough for us to debug the protocol as it exists).
>>
>> In conjunction with that, please add information on how to set up
>> compressed vs. uncompressed connections - similarly to how we've
>> documentation on setting up TLS connection (though presumably compressed
>> connection documentation will be shorter).
>>
>
> Document protocol changes needed for libpq compression.
>

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.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

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

On Mon, Aug 20, 2018 at 06:00:39PM +0300, Konstantin Knizhnik wrote:
> 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.

The patch had no reviews, and does not apply anymore, so it is moved to
next CF with waiting on author as status.
--
Michael

On 01.10.2018 09:49, Michael Paquier wrote:
> On Mon, Aug 20, 2018 at 06:00:39PM +0300, Konstantin Knizhnik wrote:
>> 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.
> The patch had no reviews, and does not apply anymore, so it is moved to
> next CF with waiting on author as status.
> --
> Michael

Rebased version of the patch is attached.

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

> On Mon, Aug 13, 2018 at 8:48 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> 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.

After some time spend reading this patch and investigating different points,
mentioned in the discussion, I tend to agree with that. As far as I see it's
probably the biggest disagreement here, that keeps things from progressing. I'm
interested in this feature, so if Konstantin doesn't mind, I'll post in the
near future (after I'll wrap up the current CF) an updated patch I'm working on
right now to propose another way of incorporating compression. For now I'm
moving patch to the next CF.

Hi,

> > 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.
>
> After some time spend reading this patch and investigating different points,
> mentioned in the discussion, I tend to agree with that. As far as I see it's
> probably the biggest disagreement here, that keeps things from progressing.
> I'm interested in this feature, so if Konstantin doesn't mind, I'll post in
> the near future (after I'll wrap up the current CF) an updated patch I'm working
> on right now to propose another way of incorporating compression. For now
> I'm moving patch to the next CF.

This thread seems to be stopped.
In last e-mail, Dmitry suggest to update the patch that implements the function in another way, and as far as I saw, he has not updated patch yet. (It may be because author has not responded.)
I understand big disagreement is here, however the status is "Needs review".
There is no review after author update the patch to v9. So I will do.

About the patch, Please update your patch to attach current master. I could not test.

About Documentation, there are typos. Please check it. I am waiting for the reviewer of the sentence because I am not so good at English.

When you add new protocol message, it needs the information of "Length of message contents in bytes, including self.".
It provides supported compression algorithm as a Byte1. I think it better to provide it as a list like the NegotiateProtocolVersion protocol.

I quickly saw code changes.

+ nread = conn->zstream
+ ? zpq_read(conn->zstream, conn->inBuffer + conn->inEnd,
+ conn->inBufSize - conn->inEnd, &processed)
+ : pqsecure_read(conn, conn->inBuffer + conn->inEnd,
+ conn->inBufSize - conn->inEnd);

How about combine as a #define macro? Because there are same logic in two place.

Do you consider anything about memory control?
Typically compression algorithm keeps dictionary in memory. I think it needs reset or some method.

Regards,
Aya Iwata

> On Wed, Jan 9, 2019 at 11:25 AM Iwata, Aya <iwata(dot)aya(at)jp(dot)fujitsu(dot)com> wrote:
>

> This thread seems to be stopped.
> In last e-mail, Dmitry suggest to update the patch that implements the
> function in another way, and as far as I saw, he has not updated patch yet.

Yep, I'm still working on it, hopefully I can submit something rather soon. But
it doesn't cancel in any way all this work, done by Konstantin, so anyway thanks
for the review!

On 09.01.2019 13:25, Iwata, Aya wrote:
> Hi,
>
>>> 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.
>> After some time spend reading this patch and investigating different points,
>> mentioned in the discussion, I tend to agree with that. As far as I see it's
>> probably the biggest disagreement here, that keeps things from progressing.
>> I'm interested in this feature, so if Konstantin doesn't mind, I'll post in
>> the near future (after I'll wrap up the current CF) an updated patch I'm working
>> on right now to propose another way of incorporating compression. For now
>> I'm moving patch to the next CF.
> This thread seems to be stopped.
> In last e-mail, Dmitry suggest to update the patch that implements the function in another way, and as far as I saw, he has not updated patch yet. (It may be because author has not responded.)
> I understand big disagreement is here, however the status is "Needs review".
> There is no review after author update the patch to v9. So I will do.
>
> About the patch, Please update your patch to attach current master. I could not test.
>
> About Documentation, there are typos. Please check it. I am waiting for the reviewer of the sentence because I am not so good at English.
>
> When you add new protocol message, it needs the information of "Length of message contents in bytes, including self.".
> It provides supported compression algorithm as a Byte1. I think it better to provide it as a list like the NegotiateProtocolVersion protocol.
>
> I quickly saw code changes.
>
> + nread = conn->zstream
> + ? zpq_read(conn->zstream, conn->inBuffer + conn->inEnd,
> + conn->inBufSize - conn->inEnd, &processed)
> + : pqsecure_read(conn, conn->inBuffer + conn->inEnd,
> + conn->inBufSize - conn->inEnd);
>
> How about combine as a #define macro? Because there are same logic in two place.
>
> Do you consider anything about memory control?
> Typically compression algorithm keeps dictionary in memory. I think it needs reset or some method.

Thank you for review.
Attached please find rebased version of the patch.
I fixed all issues you have reported except using list of supported
compression algorithms.
It will require extra round of communication between client and server
to make a decision about used compression algorithm.
I still not sure whether it is good idea to make it possible to user to
explicitly specify compression algorithm.
Right now used streaming compression algorithm is hardcoded and depends
on --use-zstd ort --use-zlib configuration options.
If client and server were built with the same options, then they are
able to use compression.

Concerning memory control: there is a call of zpq_free(PqStream) in
socket_close() function which should deallocate all memory used by
compressor:

void
zpq_free(ZpqStream *zs)
{
    if (zs != NULL)
    {
        ZSTD_freeCStream(zs->tx_stream);
        ZSTD_freeDStream(zs->rx_stream);
        free(zs);
    }
}

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

Hi,

I am sorry for my late reply.

> I fixed all issues you have reported except using list of supported compression
> algorithms.
Sure. I confirmed that.

> It will require extra round of communication between client and server to
> make a decision about used compression algorithm.
In beginning of this thread, Robbie Harwood said that no extra communication needed.
I think so, too.

> I still not sure whether it is good idea to make it possible to user to
> explicitly specify compression algorithm.
> Right now used streaming compression algorithm is hardcoded and depends on
> --use-zstd ort --use-zlib configuration options.
> If client and server were built with the same options, then they are able
> to use compression.
I understand that compression algorithm is hardcoded in your proposal.
However given the possibility of future implementation, I think
it would be better for it to have a flexibility to choose compression library.

src/backend/libpq/pqcomm.c :
In current Postgres source code, pq_recvbuf() calls secure_read()
and pq_getbyte_if_available() also calls secure_read().
It means these functions are on the same level.
However in your change, pq_getbyte_if_available() calls pq_recvbuf(),
and pq_recvbuf() calls secure_read(). The level of these functions is different.

I think the purpose of pq_getbyte_if_available() is to get a character if it exists and
the purpose of pq_recvbuf() is to acquire data up to the expected length.
In your change, pq_getbyte_if_available() may have to do unnecessary process waiting or something.

So how about changing your code like this?
The part that checks whether it is compressed is implemented as a #define macro(like fe_misc.c). And pq_recvbuf() and pq_getbyte_if_available() modify little, like this;

- r = secure_read(MyProcPort, PqRecvBuffer + PqRecvLength,
- PQ_RECV_BUFFER_SIZE - PqRecvLength);
+ r = SOME_DEFINE_NAME_();

configure:
Adding following message to the top of zlib in configure
```
{$as_echo "$as_me:${as_lineno-$LINENO}:checking whethere to build with zstd support">&5
$as_echo_n "checking whether to build with zstd suppor... ">&6;}
```

Regards,
Aya Iwata

Hi,

On 2019-02-08 07:01:01 +0000, Iwata, Aya wrote:
> > I still not sure whether it is good idea to make it possible to user to
> > explicitly specify compression algorithm.
> > Right now used streaming compression algorithm is hardcoded and depends on
> > --use-zstd ort --use-zlib configuration options.
> > If client and server were built with the same options, then they are able
> > to use compression.
> I understand that compression algorithm is hardcoded in your proposal.
> However given the possibility of future implementation, I think
> it would be better for it to have a flexibility to choose compression library.

Agreed. I think that's a hard requirement. Making explicitly not forward
compatible interfaces is a bad idea.

Greetings,

Andres Freund

Hi,

On 2018-03-30 15:53:39 +0300, Konstantin Knizhnik wrote:
> Taken in account that vulnerability was found in SSL compression and so
> SSLComppression is considered to be deprecated and insecure
> (http://www.postgresql-archive.org/disable-SSL-compression-td6010072.html)
> it will be nice to have some alternative mechanism of reducing libpq
> traffic.
>
> I have implemented some prototype implementation of it (patch is attached).
> To use zstd compression, Postgres should be configured with --with-zstd.
> Otherwise compression will use zlib unless it is disabled by --without-zlib
> option.
> I have added compression=on/off parameter to connection string and -Z option
> to psql and pgbench utilities.
> Below are some results:

I think compression is pretty useful, and I'm not convinced that the
threat model underlying the attacks on SSL really apply to postgres. But
having said that, have you done any analysis of whether your
implementation has the same issues?

Greetings,

Andres Freund

On 08.02.2019 10:14, Andres Freund wrote:
> Hi,
>
> On 2018-03-30 15:53:39 +0300, Konstantin Knizhnik wrote:
>> Taken in account that vulnerability was found in SSL compression and so
>> SSLComppression is considered to be deprecated and insecure
>> (http://www.postgresql-archive.org/disable-SSL-compression-td6010072.html)
>> it will be nice to have some alternative mechanism of reducing libpq
>> traffic.
>>
>> I have implemented some prototype implementation of it (patch is attached).
>> To use zstd compression, Postgres should be configured with --with-zstd.
>> Otherwise compression will use zlib unless it is disabled by --without-zlib
>> option.
>> I have added compression=on/off parameter to connection string and -Z option
>> to psql and pgbench utilities.
>> Below are some results:
> I think compression is pretty useful, and I'm not convinced that the
> threat model underlying the attacks on SSL really apply to postgres. But
> having said that, have you done any analysis of whether your
> implementation has the same issues?

Sorry, I am not an expert in security area, so I cannot perform analysis
whether using compression in SSL protocol
is vulnerable and is it really applicable to libpq communication between
Postgres client and server.
The main idea of compression implementation at libpq level was not to
solve this possible vulnerability
(I am also not convinced that such kind of attack is applicable to
postgres client-server communication)
but reduce traffic without requirement to use SSL (which may not be
possible or convenient because of many other reasons
not only related with potential vulnerability). Also I believe (although
I have not performed this test yet)
that zstd compression is much more efficient than one used in SSL both
in speed and compression ratio.

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

On 08.02.2019 10:01, Iwata, Aya wrote:
> Hi,
>
> I am sorry for my late reply.
>
>> I fixed all issues you have reported except using list of supported compression
>> algorithms.
> Sure. I confirmed that.
>
>> It will require extra round of communication between client and server to
>> make a decision about used compression algorithm.
> In beginning of this thread, Robbie Harwood said that no extra communication needed.
> I think so, too.

Well, I think that this problem is more complex and requires more
discussion.
There are three places determining choice of compression algorithm:
1. Specification of compression algorithm by client. Right now it is
just boolean "compression" parameter in connection string,
but it is obviously possible to specify concrete algorithm here.
2. List of compression algorithms supported by client.
3. List of compression algorithms supported by server.

Concerning first option I have very serious doubt that it is good idea
to let client choose compression protocol.
Without extra round-trip it can be only implemented in this way:
if client toggles compression option in connection string, then libpq
includes in startup packet list of supported compression algorithms.
Then server intersects this list with its own set of supported
compression algorithms and if result is not empty, then
somehow choose  one of the commonly supported algorithms and sends it to
the client with 'z' command.

One more question: should we allow custom defined compression methods
and if so, how them can be handled at client side (at server we can use
standard extension
dynamic loading mechanism).

Frankly speaking, I do not think that such flexibility in choosing
compression algorithms is really needed.
I do not expect that there will be many situations where old client has
to communicate with new server or visa versa.
In most cases both client and server belongs to the same postgres
distributive and so implements the same compression algorithm.
As far as we are compressing only temporary data (traffic), the problem
of providing backward compatibility seems to be not so important.

>
>> I still not sure whether it is good idea to make it possible to user to
>> explicitly specify compression algorithm.
>> Right now used streaming compression algorithm is hardcoded and depends on
>> --use-zstd ort --use-zlib configuration options.
>> If client and server were built with the same options, then they are able
>> to use compression.
> I understand that compression algorithm is hardcoded in your proposal.
> However given the possibility of future implementation, I think
> it would be better for it to have a flexibility to choose compression library.
>
> src/backend/libpq/pqcomm.c :
> In current Postgres source code, pq_recvbuf() calls secure_read()
> and pq_getbyte_if_available() also calls secure_read().
> It means these functions are on the same level.
> However in your change, pq_getbyte_if_available() calls pq_recvbuf(),
> and pq_recvbuf() calls secure_read(). The level of these functions is different.
>
> I think the purpose of pq_getbyte_if_available() is to get a character if it exists and
> the purpose of pq_recvbuf() is to acquire data up to the expected length.
> In your change, pq_getbyte_if_available() may have to do unnecessary process waiting or something.

Sorry, but this change is essential. We can have some available data in
compression buffer and we need to try to fetch it in
pq_getbyte_if_available()
instead of just returning EOF.
>
> So how about changing your code like this?
> The part that checks whether it is compressed is implemented as a #define macro(like fe_misc.c). And pq_recvbuf() and pq_getbyte_if_available() modify little, like this;
>
> - r = secure_read(MyProcPort, PqRecvBuffer + PqRecvLength,
> - PQ_RECV_BUFFER_SIZE - PqRecvLength);
> + r = SOME_DEFINE_NAME_();
>
> configure:
> Adding following message to the top of zlib in configure
> ```
> {$as_echo "$as_me:${as_lineno-$LINENO}:checking whethere to build with zstd support">&5
> $as_echo_n "checking whether to build with zstd suppor... ">&6;}
> ```

Sorry, but it seems to me that the following fragment of configure is
doing it:

+
+if test "$with_zstd" = yes ; then
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for ZSTD_compress
in -lzstd" >&5
+$as_echo_n "checking for ZSTD_compress in -lzstd... " >&6; }
+if ${ac_cv_lib_zstd_ZSTD_compress+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  ac_check_lib_save_LIBS=$LIBS
+LIBS="-lzstd  $LIBS"
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+/* Override any GCC internal prototype to avoid an error.
+   Use char because int might match the return type of a GCC
+   builtin and then its argument prototype would still apply.  */
+#ifdef __cplusplus
+extern "C"
+#endif
+char ZSTD_compress ();
+int
+main ()
+{
+return ZSTD_compress ();
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+  ac_cv_lib_zstd_ZSTD_compress=yes
+else
+  ac_cv_lib_zstd_ZSTD_compress=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+    conftest$ac_exeext conftest.$ac_ext
+LIBS=$ac_check_lib_save_LIBS
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result:
$ac_cv_lib_zstd_ZSTD_compress" >&5
+$as_echo "$ac_cv_lib_zstd_ZSTD_compress" >&6; }
+if test "x$ac_cv_lib_zstd_ZSTD_compress" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_LIBZSTD 1
+_ACEOF
+
+  LIBS="-lzstd $LIBS"
+
+else
+  as_fn_error $? "library 'zstd' is required for ZSTD support" "$LINENO" 5
+fi
+
+fi

> Regards,
> Aya Iwata

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

> On 8 Feb 2019, at 10:15, Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> wrote:

> Frankly speaking, I do not think that such flexibility in choosing compression algorithms is really needed.
> I do not expect that there will be many situations where old client has to communicate with new server or visa versa.
> In most cases both client and server belongs to the same postgres distributive and so implements the same compression algorithm.
> As far as we are compressing only temporary data (traffic), the problem of providing backward compatibility seems to be not so important.

I don’t think this assumption is entirely valid, and would risk unnecessary
breakage.

cheers ./daniel

Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:

> On 08.02.2019 10:01, Iwata, Aya wrote:
>
>>> I fixed all issues you have reported except using list of supported
>>> compression algorithms.
>>
>> Sure. I confirmed that.
>>
>>> It will require extra round of communication between client and
>>> server to make a decision about used compression algorithm.
>>
>> In beginning of this thread, Robbie Harwood said that no extra
>> communication needed. I think so, too.
>
> Well, I think that this problem is more complex and requires more
> discussion.
> There are three places determining choice of compression algorithm:
> 1. Specification of compression algorithm by client. Right now it is
> just boolean "compression" parameter in connection string,
> but it is obviously possible to specify concrete algorithm here.
> 2. List of compression algorithms supported by client.
> 3. List of compression algorithms supported by server.
>
> Concerning first option I have very serious doubt that it is good idea
> to let client choose compression protocol.
> Without extra round-trip it can be only implemented in this way:
> if client toggles compression option in connection string, then libpq
> includes in startup packet list of supported compression algorithms.
> Then server intersects this list with its own set of supported
> compression algorithms and if result is not empty, then
> somehow choose  one of the commonly supported algorithms and sends it to
> the client with 'z' command.

The easiest way, which I laid out earlier in
id:jlgfu1gqjbk(dot)fsf(at)redhat(dot)com, is to have the server perform selection.
The client sends a list of supported algorithms in startup. Startup has
a reply, so if the server wishes to use compression, then its startup
reply contains which algorithm to use. Compression then begins after
startup.

If you really wanted to compress half the startup for some reason, you
can even have the server send a packet which consists of the choice of
compression algorithm and everything else in it subsequently
compressed. I don't see this being useful. However, you can use a
similar approach to let the client choose the algorithm if there were
some compelling reason for that (there is none I'm aware of to prefer
one to the other) - startup from client requests compression, reply from
server lists supported algorithms, next packet from client indicates
which one is in use along with compressed payload.

It may help to keep in mind that you are defining your own message type
here.

> Frankly speaking, I do not think that such flexibility in choosing
> compression algorithms is really needed.
> I do not expect that there will be many situations where old client has
> to communicate with new server or visa versa.
> In most cases both client and server belongs to the same postgres
> distributive and so implements the same compression algorithm.
> As far as we are compressing only temporary data (traffic), the problem
> of providing backward compatibility seems to be not so important.

Your comments have been heard, but this is the model that numerous folks
from project has told you we have. Your code will not pass review
without algorithm agility.

>> src/backend/libpq/pqcomm.c :
>> In current Postgres source code, pq_recvbuf() calls secure_read()
>> and pq_getbyte_if_available() also calls secure_read().
>> It means these functions are on the same level.
>> However in your change, pq_getbyte_if_available() calls pq_recvbuf(),
>> and pq_recvbuf() calls secure_read(). The level of these functions is different.
>>
>> I think the purpose of pq_getbyte_if_available() is to get a
>> character if it exists and the purpose of pq_recvbuf() is to acquire
>> data up to the expected length. In your change,
>> pq_getbyte_if_available() may have to do unnecessary process waiting
>> or something.
>
> Sorry, but this change is essential. We can have some available data
> in compression buffer and we need to try to fetch it in
> pq_getbyte_if_available() instead of just returning EOF.

Aya is correct about the purposes of these functions. Take a look at
how the buffering in TLS or GSSAPI works for an example of how to do
this correctly.

As with agility, this is what multiple folks from the project have told
you is a hard requirement. None of us will be okaying your code without
proper transport layering.

Thanks,
--Robbie

On 08.02.2019 12:33, Daniel Gustafsson wrote:
>> On 8 Feb 2019, at 10:15, Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> wrote:
>> Frankly speaking, I do not think that such flexibility in choosing compression algorithms is really needed.
>> I do not expect that there will be many situations where old client has to communicate with new server or visa versa.
>> In most cases both client and server belongs to the same postgres distributive and so implements the same compression algorithm.
>> As far as we are compressing only temporary data (traffic), the problem of providing backward compatibility seems to be not so important.
> I don’t think this assumption is entirely valid, and would risk unnecessary
> breakage.

I am also not sure in this assumption. We (PostgresPro) are having some
issues now with CFS (file level compression of Postgres database).
Some build are using zstd, some are using zlib (default)...
zstd is faster and provides better compression ratio and is available at
most platforms.
But zlib is available almost everywhere and is used by Postgres by
default...
The only thing I know for sure: if we implement several algorithms and
make it possible for database user to make a choice, then
we will get much more problems.

Right now Postgres is using zlib as the only supported compression
algorithm in many places.
So may be libpq compression should also use only zlib and provide no
other choices?

Concerning backward compatibility. Assume that we allow use zstd, but
then Facebook change zstd license or some critical bug in it is found.
So we will have to exclude dependency on zstd. So there will be no
backward compatibility in any case, even if we support more
sophisticated negotiation between client and server in choosing
compression algorithm.

What can be more interesting - is to support custom compression
algorithms (optimized for the particular data flow).
But it seems to be completely different and much more sophisticated
story. We have to provide some mechanism for loading foreign libraries
at client side!
IMHO it is overkill.

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

On 08.02.2019 19:26, Robbie Harwood wrote:
> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>
>> On 08.02.2019 10:01, Iwata, Aya wrote:
>>
>>>> I fixed all issues you have reported except using list of supported
>>>> compression algorithms.
>>> Sure. I confirmed that.
>>>
>>>> It will require extra round of communication between client and
>>>> server to make a decision about used compression algorithm.
>>> In beginning of this thread, Robbie Harwood said that no extra
>>> communication needed. I think so, too.
>> Well, I think that this problem is more complex and requires more
>> discussion.
>> There are three places determining choice of compression algorithm:
>> 1. Specification of compression algorithm by client. Right now it is
>> just boolean "compression" parameter in connection string,
>> but it is obviously possible to specify concrete algorithm here.
>> 2. List of compression algorithms supported by client.
>> 3. List of compression algorithms supported by server.
>>
>> Concerning first option I have very serious doubt that it is good idea
>> to let client choose compression protocol.
>> Without extra round-trip it can be only implemented in this way:
>> if client toggles compression option in connection string, then libpq
>> includes in startup packet list of supported compression algorithms.
>> Then server intersects this list with its own set of supported
>> compression algorithms and if result is not empty, then
>> somehow choose  one of the commonly supported algorithms and sends it to
>> the client with 'z' command.
> The easiest way, which I laid out earlier in
> id:jlgfu1gqjbk(dot)fsf(at)redhat(dot)com, is to have the server perform selection.
> The client sends a list of supported algorithms in startup. Startup has
> a reply, so if the server wishes to use compression, then its startup
> reply contains which algorithm to use. Compression then begins after
> startup.
>
> If you really wanted to compress half the startup for some reason, you
> can even have the server send a packet which consists of the choice of
> compression algorithm and everything else in it subsequently
> compressed. I don't see this being useful. However, you can use a
> similar approach to let the client choose the algorithm if there were
> some compelling reason for that (there is none I'm aware of to prefer
> one to the other) - startup from client requests compression, reply from
> server lists supported algorithms, next packet from client indicates
> which one is in use along with compressed payload.
I already replied you that that next package cannot indicate which
algorithm the client has choosen.
Using magics or some other tricks are not reliable and acceptable solution/
It can be done only by introducing extra message type.

Actually it is not really needed, because if client sends to the server
list of supported algorithms in startup packet,
then server can made a decision and inform client about it (using
special message type as it is done now).
Then client doesn't need to make a choice. I can do it if everybody
think that choice of compression algorithm is so important feature.
Just wonder: Postgres now is living good with hardcoded zlib and
built-in LZ compression algorithm implementation.

> It may help to keep in mind that you are defining your own message type
> here.
>
>> Frankly speaking, I do not think that such flexibility in choosing
>> compression algorithms is really needed.
>> I do not expect that there will be many situations where old client has
>> to communicate with new server or visa versa.
>> In most cases both client and server belongs to the same postgres
>> distributive and so implements the same compression algorithm.
>> As far as we are compressing only temporary data (traffic), the problem
>> of providing backward compatibility seems to be not so important.
> Your comments have been heard, but this is the model that numerous folks
> from project has told you we have. Your code will not pass review
> without algorithm agility.
>
>>> src/backend/libpq/pqcomm.c :
>>> In current Postgres source code, pq_recvbuf() calls secure_read()
>>> and pq_getbyte_if_available() also calls secure_read().
>>> It means these functions are on the same level.
>>> However in your change, pq_getbyte_if_available() calls pq_recvbuf(),
>>> and pq_recvbuf() calls secure_read(). The level of these functions is different.
>>>
>>> I think the purpose of pq_getbyte_if_available() is to get a
>>> character if it exists and the purpose of pq_recvbuf() is to acquire
>>> data up to the expected length. In your change,
>>> pq_getbyte_if_available() may have to do unnecessary process waiting
>>> or something.
>> Sorry, but this change is essential. We can have some available data
>> in compression buffer and we need to try to fetch it in
>> pq_getbyte_if_available() instead of just returning EOF.
> Aya is correct about the purposes of these functions. Take a look at
> how the buffering in TLS or GSSAPI works for an example of how to do
> this correctly.
>
> As with agility, this is what multiple folks from the project have told
> you is a hard requirement. None of us will be okaying your code without
> proper transport layering.
Guys, I wondering which layering violation you are talking about?
Right now there are two cut&pasted peace of almost the same code in
pqcomm.c:

static int
pq_recvbuf(void)
...
    /* Ensure that we're in blocking mode */
    socket_set_nonblocking(false);

    /* Can fill buffer from PqRecvLength and upwards */
    for (;;)
    {
        int            r;

        r = secure_read(MyProcPort, PqRecvBuffer + PqRecvLength,
                        PQ_RECV_BUFFER_SIZE - PqRecvLength);

        if (r < 0)
        {
            if (errno == EINTR)
                continue;        /* Ok if interrupted */

            /*
             * Careful: an ereport() that tries to write to the client
would
             * cause recursion to here, leading to stack overflow and core
             * dump!  This message must go *only* to the postmaster log.
             */
            ereport(COMMERROR,
                    (errcode_for_socket_access(),
                     errmsg("could not receive data from client: %m")));
            return EOF;
        }
        if (r == 0)
        {
            /*
             * EOF detected.  We used to write a log message here, but it's
             * better to expect the ultimate caller to do that.
             */
            return EOF;
        }
        /* r contains number of bytes read, so just incr length */
        PqRecvLength += r;
        return 0;
    }
}

int
pq_getbyte_if_available(unsigned char *c)
{
...
    /* Put the socket into non-blocking mode */
    socket_set_nonblocking(true);

    r = secure_read(MyProcPort, c, 1);
    if (r < 0)
    {
        /*
         * Ok if no data available without blocking or interrupted (though
         * EINTR really shouldn't happen with a non-blocking socket).
Report
         * other errors.
         */
        if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR)
            r = 0;
        else
        {
            /*
             * Careful: an ereport() that tries to write to the client
would
             * cause recursion to here, leading to stack overflow and core
             * dump!  This message must go *only* to the postmaster log.
             */
            ereport(COMMERROR,
                    (errcode_for_socket_access(),
                     errmsg("could not receive data from client: %m")));
            r = EOF;
        }
    }
    else if (r == 0)
    {
        /* EOF detected */
        r = EOF;
    }

    return r;
}

The only difference between them is that in first case we are using
block mode and in the second case non-blocking mode.
Also there is loop in first case handling ENITR.

I have added nowait parameter to  pq_recvbuf(bool nowait)
and remove second fragment of code so it is changed just to one line:

if (PqRecvPointer < PqRecvLength || (r = pq_recvbuf(true)) > 0)

Both functions (pq_recvbuf and pq_getbyte_if_available) are defined in
the same file.
So there is not any layering violation here. It is just elimination of
duplicated code.

If the only objection is that pq_getbyte_if_available is calling
pq_recvbuf, then I can add some other function compress_read (as analog
of secure read) and call it from both functions. But frankly speaking I
do not see any advantages of such approach.
It just introduce extra function call and gives no extra
encapsulation.modularity, flexibility or whatever else.

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

On 2019-02-08 12:15:58 +0300, Konstantin Knizhnik wrote:
> Frankly speaking, I do not think that such flexibility in choosing
> compression algorithms is really needed.
> I do not expect that there will be many situations where old client has to
> communicate with new server or visa versa.
> In most cases both client and server belongs to the same postgres
> distributive and so implements the same compression algorithm.
> As far as we are compressing only temporary data (traffic), the problem of
> providing backward compatibility seems to be not so important.

I think we should outright reject any patch without compression type
negotiation.

On 08.02.2019 21:57, Andres Freund wrote:
> On 2019-02-08 12:15:58 +0300, Konstantin Knizhnik wrote:
>> Frankly speaking, I do not think that such flexibility in choosing
>> compression algorithms is really needed.
>> I do not expect that there will be many situations where old client has to
>> communicate with new server or visa versa.
>> In most cases both client and server belongs to the same postgres
>> distributive and so implements the same compression algorithm.
>> As far as we are compressing only temporary data (traffic), the problem of
>> providing backward compatibility seems to be not so important.
> I think we should outright reject any patch without compression type
> negotiation.
Does it mean that it is necessary to support multiple compression
algorithms and make it possible to perform switch between them at
runtime? Right now compression algorithm is linked statically.
Negotiation of compression type is currently performed but it only
checks that server and client are implementing the same algorithm and
disables compression if it is not true.

If we are going to support multiple compression algorithms, do we need
dynamic loading of correspondent compression libraries or static linking
is ok? In case of dynamic linking we need to somehow specify information
about available compression algorithms.
Some special subdirectory for them so that I can traverse this directory
and try to load correspondent libraries?

Only I find it too complicated for the addressed problem?

On 2/8/19 11:10 PM, Konstantin Knizhnik wrote:
>
>
> On 08.02.2019 21:57, Andres Freund wrote:
>> On 2019-02-08 12:15:58 +0300, Konstantin Knizhnik wrote:
>>> Frankly speaking, I do not think that such flexibility in choosing
>>> compression algorithms is really needed.
>>> I do not expect that there will be many situations where old client
>>> has to
>>> communicate with new server or visa versa.
>>> In most cases both client and server belongs to the same postgres
>>> distributive and so implements the same compression algorithm.
>>> As far as we are compressing only temporary data (traffic), the
>>> problem of
>>> providing backward compatibility seems to be not so important.
>> I think we should outright reject any patch without compression type
>> negotiation.
> Does it mean that it is necessary to support multiple compression
> algorithms and make it possible to perform switch between them at
> runtime?

IMHO the negotiation should happen at connection time, i.e. the server
should support connections compressed by different algorithms. Not sure
if that's what you mean by runtime.

AFAICS this is quite close to how negotiation of encryption algorithms
works, in TLS and so on. Client specifies supported algorithms, server
compares that to list of supported algorithms, deduces the encryption
algorithm and notifies the client.

To allow fall-back to uncompressed connection, use "none" as algorithm.
If there's no common algorithm, fail.

> Right now compression algorithm is linked statically.
> Negotiation of compression type is currently performed but it only
> checks that server and client are implementing the same algorithm and
> disables compression if it is not true.
>

I don't think we should automatically fall-back to disabled compression,
when a client specifies compression algorithm.

> If we are going to support multiple compression algorithms, do we need
> dynamic loading of correspondent compression libraries or static linking
> is ok? In case of dynamic linking we need to somehow specify information
> about available compression algorithms.
> Some special subdirectory for them so that I can traverse this directory
> and try to load correspondent libraries?
>
> Only I find it too complicated for the addressed problem?
>

I don't think we need dynamic algorithms v1, but IMHO it'd be pretty
simple to do - just add a shared_preload_library which registers it in a
list in memory.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 09.02.2019 1:38, Tomas Vondra wrote:
> On 2/8/19 11:10 PM, Konstantin Knizhnik wrote:
>>
>> On 08.02.2019 21:57, Andres Freund wrote:
>>> On 2019-02-08 12:15:58 +0300, Konstantin Knizhnik wrote:
>>>> Frankly speaking, I do not think that such flexibility in choosing
>>>> compression algorithms is really needed.
>>>> I do not expect that there will be many situations where old client
>>>> has to
>>>> communicate with new server or visa versa.
>>>> In most cases both client and server belongs to the same postgres
>>>> distributive and so implements the same compression algorithm.
>>>> As far as we are compressing only temporary data (traffic), the
>>>> problem of
>>>> providing backward compatibility seems to be not so important.
>>> I think we should outright reject any patch without compression type
>>> negotiation.
>> Does it mean that it is necessary to support multiple compression
>> algorithms and make it possible to perform switch between them at
>> runtime?
> IMHO the negotiation should happen at connection time, i.e. the server
> should support connections compressed by different algorithms. Not sure
> if that's what you mean by runtime.
>
> AFAICS this is quite close to how negotiation of encryption algorithms
> works, in TLS and so on. Client specifies supported algorithms, server
> compares that to list of supported algorithms, deduces the encryption
> algorithm and notifies the client.
>
> To allow fall-back to uncompressed connection, use "none" as algorithm.
> If there's no common algorithm, fail.

It is good analogue with SSL.
Yes, SSL protocol provides several ways of authentication, encryption,...
And there are several different libraries implementing SSL.
But Postgres is using only one of them: OpenSSL.
If I want to use some other library (for example to make it possible to
serialize and pass SSL session state to other
process), then there is no way to achieve it.

Actually zstd also includes implementations of several compression
algorithms and it choose one of them best fitting particular data
stream. As in case of SSL, choice of algorithm is performed internally
inside zstd - not at libpq level.

Sorry, if my explanation about static and dynamic (at runtime) choice
were not correct.
This is how compression is toggled now:

#if HAVE_LIBZSTD
ZpqStream*
zpq_create(zpq_tx_func tx_func, zpq_rx_func rx_func, void *arg)
{
...
}
#endif

So if Postgres was configured with zstd, then this implementation is
included inclient and server Postgres libraries.
If postgres is configures with zlib, them  zlib implementation will be used.
This is similar with using compression and most of other configurable
features in Postgres.

If we want to provide dynamic choice at runtime, then we need to have
array with available compression algorithms:

#if HAVE_LIBZSTD
static ZpqStream*
zstd_create(zpq_tx_func tx_func, zpq_rx_func rx_func, void *arg)
{
...
}
#endif

ZpqCompressorImpl compressorImpl[] =
{
#if HAVE_LIBZSTD
{zstd_create, zstd_read,zstd_write,...},
#endif
#if HAVE_ZLIB
{zlib_create, zlib_read,zslib_write,...},
#endif
...
}

And the most interesting case is that if we load library dynamically.
Each implementation is generated in separate library (for  example
libpztd.so).
In this case we need to somehow specify available libraries.
For example by placing them in separate directory, or specifying list of
libraries in postgresql.conf.
Then we try to load this library using dlopen.  Such library has
external dependencies of correspondent compressor library (for example
-lz). The library can be successfully loaded if there correspond
compressor implementation was install at the system.
This is most flexible approach allowing to provide custom implementation
of compressors.
Compression implementation can be organized as Postgres extension and
its PG_init function registers this implementation in some list.

This is what I am asking about.
Right now approach 1) is implemented: compression algorithm is defined
by configure.
It is no so difficult to extend it to support multiple algorithms.
And the most flexible but more sophisticated is to load libraries
dynamically.

>
>> Right now compression algorithm is linked statically.
>> Negotiation of compression type is currently performed but it only
>> checks that server and client are implementing the same algorithm and
>> disables compression if it is not true.
>>
> I don't think we should automatically fall-back to disabled compression,
> when a client specifies compression algorithm.

Compression is disabled only when client and server were configured with
different compression algorithms (i.e. zstd and zlib).

>
>> If we are going to support multiple compression algorithms, do we need
>> dynamic loading of correspondent compression libraries or static linking
>> is ok? In case of dynamic linking we need to somehow specify information
>> about available compression algorithms.
>> Some special subdirectory for them so that I can traverse this directory
>> and try to load correspondent libraries?
>>
>> Only I find it too complicated for the addressed problem?
>>
> I don't think we need dynamic algorithms v1, but IMHO it'd be pretty
> simple to do - just add a shared_preload_library which registers it in a
> list in memory.

I do not think that it is necessary to include such libraries in
preload_shared_libraries list.
It can be done lazily only of compression is requested by client.
Also please notice that we need to load compression library both at
server and client sides.
preload_shared_libraries works only for postmaster.

Hi,

On 2019-02-08 23:38:12 +0100, Tomas Vondra wrote:
> On 2/8/19 11:10 PM, Konstantin Knizhnik wrote:
> > Does it mean that it is necessary to support multiple compression
> > algorithms and make it possible to perform switch between them at
> > runtime?
>
> IMHO the negotiation should happen at connection time, i.e. the server
> should support connections compressed by different algorithms.

Exactly. And support client libraries with support for different
compression algorithms. We have large forward and backward compatibility
with libpq and even moreso with the wire protocol, we therefore
shouldn't require exactly matching client / server versions or
compilation options.

> Not sure if that's what you mean by runtime.

Same.

> AFAICS this is quite close to how negotiation of encryption algorithms
> works, in TLS and so on. Client specifies supported algorithms, server
> compares that to list of supported algorithms, deduces the encryption
> algorithm and notifies the client.
>
> To allow fall-back to uncompressed connection, use "none" as algorithm.
> If there's no common algorithm, fail.

I'm somewhat inclined to think that not compressing is
preferrable. There's some reason to think that there's some
cryptographical issues around compression, therefore it could make a ton
of sense for specific servers to disable compression centrally.

> > Right now compression algorithm is linked statically.
> > Negotiation of compression type is currently performed but it only
> > checks that server and client are implementing the same algorithm and
> > disables compression if it is not true.

> I don't think we should automatically fall-back to disabled compression,
> when a client specifies compression algorithm.

Why?

> > If we are going to support multiple compression algorithms, do we need
> > dynamic loading of correspondent compression libraries or static linking
> > is ok? In case of dynamic linking we need to somehow specify information
> > about available compression algorithms.
> > Some special subdirectory for them so that I can traverse this directory
> > and try to load correspondent libraries?
> >
> > Only I find it too complicated for the addressed problem?
> >
>
> I don't think we need dynamic algorithms v1, but IMHO it'd be pretty
> simple to do - just add a shared_preload_library which registers it in a
> list in memory.

I personally don't think that's something we need to support. There's a
lot of issues around naming registries that need to synchronized between
client/server.

Greetings,

Andres Freund

On 2/9/19 3:14 PM, Andres Freund wrote:
> Hi,
>
> On 2019-02-08 23:38:12 +0100, Tomas Vondra wrote:
>> On 2/8/19 11:10 PM, Konstantin Knizhnik wrote:
>>> Does it mean that it is necessary to support multiple compression
>>> algorithms and make it possible to perform switch between them at
>>> runtime?
>>
>> IMHO the negotiation should happen at connection time, i.e. the server
>> should support connections compressed by different algorithms.
>
> Exactly. And support client libraries with support for different
> compression algorithms. We have large forward and backward compatibility
> with libpq and even moreso with the wire protocol, we therefore
> shouldn't require exactly matching client / server versions or
> compilation options.
>

IMHO the main reason to want/need this is not as much backward/forward
compatibility (in the sense of us adding/removing supported algorithms).
A much bigger problem (IMHO) is that different systems may not have some
of the libraries needed, or maybe the packager decided not to enable a
particular library, etc. That's likely far more dynamic.

>
>> AFAICS this is quite close to how negotiation of encryption algorithms
>> works, in TLS and so on. Client specifies supported algorithms, server
>> compares that to list of supported algorithms, deduces the encryption
>> algorithm and notifies the client.
>>
>> To allow fall-back to uncompressed connection, use "none" as algorithm.
>> If there's no common algorithm, fail.
>
> I'm somewhat inclined to think that not compressing is
> preferrable. There's some reason to think that there's some
> cryptographical issues around compression, therefore it could make a ton
> of sense for specific servers to disable compression centrally.
>

I agree compression is not the same as crypto in this case, so fallback
to uncompressed connection might be sensible in some cases. But in other
cases I probably want to be notified ASAP that the compression does not
work, before pushing 10x the amount of data through the network. That's
why I think why I proposed to also have "none" as compression algorithm,
to allow/disallow the fallback.

>
>>> Right now compression algorithm is linked statically.
>>> Negotiation of compression type is currently performed but it only
>>> checks that server and client are implementing the same algorithm and
>>> disables compression if it is not true.
>
>> I don't think we should automatically fall-back to disabled compression,
>> when a client specifies compression algorithm.
>
> Why?
>

Because it's pretty difficult to realize it happened, particularly for
non-interactive connections. Imagine you have machines where transfer
between them is not free - surely you want to know when you suddenly get
10x the traffic, right?

But as I said above, this should be configurable. IMHO having "none"
algorithm to explicitly enable this seems like a reasonable solution.

>
>>> If we are going to support multiple compression algorithms, do we need
>>> dynamic loading of correspondent compression libraries or static linking
>>> is ok? In case of dynamic linking we need to somehow specify information
>>> about available compression algorithms.
>>> Some special subdirectory for them so that I can traverse this directory
>>> and try to load correspondent libraries?
>>>
>>> Only I find it too complicated for the addressed problem?
>>>
>>
>> I don't think we need dynamic algorithms v1, but IMHO it'd be pretty
>> simple to do - just add a shared_preload_library which registers it in a
>> list in memory.
>
> I personally don't think that's something we need to support. There's a
> lot of issues around naming registries that need to synchronized between
> client/server.
>

Don't we have that issue even without the dynamic registration? Let's
say you have custom driver implementing the protocol (but not based on
libpq). Surely that assumes the algorithm names match what we have?

In the worst case the decompression fails (which may happen already
anyway) and the connection dies. Not a big deal, no?

But I agree v1 should not include this dynamic registration.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2/9/19 3:02 PM, Konstantin Knizhnik wrote:
>
>
> On 09.02.2019 1:38, Tomas Vondra wrote:
>> On 2/8/19 11:10 PM, Konstantin Knizhnik wrote:
>>>
>>> On 08.02.2019 21:57, Andres Freund wrote:
>>>> On 2019-02-08 12:15:58 +0300, Konstantin Knizhnik wrote:
>>>>> Frankly speaking, I do not think that such flexibility in choosing
>>>>> compression algorithms is really needed.
>>>>> I do not expect that there will be many situations where old client
>>>>> has to
>>>>> communicate with new server or visa versa.
>>>>> In most cases both client and server belongs to the same postgres
>>>>> distributive and so implements the same compression algorithm.
>>>>> As far as we are compressing only temporary data (traffic), the
>>>>> problem of
>>>>> providing backward compatibility seems to be not so important.
>>>> I think we should outright reject any patch without compression type
>>>> negotiation.
>>> Does it mean that it is necessary to support multiple compression
>>> algorithms and make it possible to perform switch between them at
>>> runtime?
>> IMHO the negotiation should happen at connection time, i.e. the server
>> should support connections compressed by different algorithms. Not sure
>> if that's what you mean by runtime.
>>
>> AFAICS this is quite close to how negotiation of encryption algorithms
>> works, in TLS and so on. Client specifies supported algorithms, server
>> compares that to list of supported algorithms, deduces the encryption
>> algorithm and notifies the client.
>>
>> To allow fall-back to uncompressed connection, use "none" as algorithm.
>> If there's no common algorithm, fail.
>
> It is good analogue with SSL.
> Yes, SSL protocol provides several ways of authentication, encryption,...
> And there are several different libraries implementing SSL.
> But Postgres is using only one of them: OpenSSL.
> If I want to use some other library (for example to make it possible to
> serialize and pass SSL session state to other
> process), then there is no way to achieve it.
>

That's rather misleading. Firstly, it's true we only support OpenSSL at
the moment, but I do remember we've been working on adding support to a
bunch of other TLS libraries.

But more importantly, it's not the TLS library that's negotiated. It's
the encryption algorithms that is negotiated. The server is oblivious
which TLS library is used by the client (and vice versa), because the
messages are the same - what matters is that they agree on keys,
ciphers, etc. And those can differ/change between libraries or even
versions of the same library.

For us, the situation is the same - we have the messages specified by
the FE/BE protocol, and it's the algorithms that are negotiated.

> Actually zstd also includes implementations of several compression
> algorithms and it choose one of them best fitting particular data
> stream. As in case of SSL, choice of algorithm is performed internally
> inside zstd - not at libpq level.
>

Really? I always thought zstd is a separate compression algorithm.
There's adaptive compression feature, but AFAIK that essentially tweaks
compression level based on network connection. Can you point me to the
sources or docs explaining this?

Anyway, this does not really change anything - it's internal zstd stuff.

> Sorry, if my explanation about static and dynamic (at runtime) choice
> were not correct.
> This is how compression is toggled now:
>
> #if HAVE_LIBZSTD
> ZpqStream*
> zpq_create(zpq_tx_func tx_func, zpq_rx_func rx_func, void *arg)
> {
> ...
> }
> #endif
>
> So if Postgres was configured with zstd, then this implementation is
> included inclient and server Postgres libraries.
> If postgres is configures with zlib, them  zlib implementation will be
> used.
> This is similar with using compression and most of other configurable
> features in Postgres.
>
> If we want to provide dynamic choice at runtime, then we need to have
> array with available compression algorithms:
>
> #if HAVE_LIBZSTD
> static ZpqStream*
> zstd_create(zpq_tx_func tx_func, zpq_rx_func rx_func, void *arg)
> {
> ...
> }
> #endif
>
> ZpqCompressorImpl compressorImpl[] =
> {
> #if HAVE_LIBZSTD
> {zstd_create, zstd_read,zstd_write,...},
> #endif
> #if HAVE_ZLIB
> {zlib_create, zlib_read,zslib_write,...},
> #endif
> ...
> }
>

Yes, that's mostly what I've been imagining, except that you also need
some sort of identifier for the algorithm - a cstring at the beginning
of the struct should be enough, I guess.

> And the most interesting case is that if we load library dynamically.
> Each implementation is generated in separate library (for  example
> libpztd.so).
> In this case we need to somehow specify available libraries.
> For example by placing them in separate directory, or specifying list of
> libraries in postgresql.conf.
> Then we try to load this library using dlopen.  Such library has
> external dependencies of correspondent compressor library (for example
> -lz). The library can be successfully loaded if there correspond
> compressor implementation was install at the system.
> This is most flexible approach allowing to provide custom implementation
> of compressors.
> Compression implementation can be organized as Postgres extension and
> its PG_init function registers this implementation in some list.
>

How you could make them as extensions? Those are database-specific and
the authentication happens before you have access to the database.

As I said before, I think adding them using shared_preload_libraries and
registering them in _PG_init should be sufficient.

> This is what I am asking about.
> Right now approach 1) is implemented: compression algorithm is defined
> by configure.
> It is no so difficult to extend it to support multiple algorithms.
> And the most flexible but more sophisticated is to load libraries
> dynamically.
>

Well, there's nothing stopping you from implementing the dynamic
loading, but IMHO it makes v1 unnecessarily complex.

>>
>>> Right now compression algorithm is linked statically.
>>> Negotiation of compression type is currently performed but it only
>>> checks that server and client are implementing the same algorithm and
>>> disables compression if it is not true.
>>>
>> I don't think we should automatically fall-back to disabled compression,
>> when a client specifies compression algorithm.
>
> Compression is disabled only when client and server were configured with
> different compression algorithms (i.e. zstd and zlib).
>

Yes, and I'm of the opinion we shouldn't do that, unless unless both
sides explicitly enable that in some way.

>>
>>> If we are going to support multiple compression algorithms, do we need
>>> dynamic loading of correspondent compression libraries or static linking
>>> is ok? In case of dynamic linking we need to somehow specify information
>>> about available compression algorithms.
>>> Some special subdirectory for them so that I can traverse this directory
>>> and try to load correspondent libraries?
>>>
>>> Only I find it too complicated for the addressed problem?
>>>
>> I don't think we need dynamic algorithms v1, but IMHO it'd be pretty
>> simple to do - just add a shared_preload_library which registers it in a
>> list in memory.
>
> I do not think that it is necessary to include such libraries in
> preload_shared_libraries list.
> It can be done lazily only of compression is requested by client.
> Also please notice that we need to load compression library both at
> server and client sides.
> preload_shared_libraries works only for postmaster.
>

How would you know which libraries to load for a given compression
algorithm? Surely, loading all available libraries just because they
might happen to implement the requested algorithm seems bad? IMHO the
shared_preload_libraries is a much safer (and working) approach.

But I'd just leave this aside, because trying to pack all of this into
v1 just increases the likelihood of it not getting committed in time.
And the fact that we don't have any such infrastructure in the client
just increases the risk.

+1 to go with hard-coded list of supported algorithms in v1

regars

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 10.02.2019 3:25, Tomas Vondra wrote:
>
> On 2/9/19 3:02 PM, Konstantin Knizhnik wrote:
>>
>> On 09.02.2019 1:38, Tomas Vondra wrote:
>>> On 2/8/19 11:10 PM, Konstantin Knizhnik wrote:
>>>> On 08.02.2019 21:57, Andres Freund wrote:
>>>>> On 2019-02-08 12:15:58 +0300, Konstantin Knizhnik wrote:
>>>>>> Frankly speaking, I do not think that such flexibility in choosing
>>>>>> compression algorithms is really needed.
>>>>>> I do not expect that there will be many situations where old client
>>>>>> has to
>>>>>> communicate with new server or visa versa.
>>>>>> In most cases both client and server belongs to the same postgres
>>>>>> distributive and so implements the same compression algorithm.
>>>>>> As far as we are compressing only temporary data (traffic), the
>>>>>> problem of
>>>>>> providing backward compatibility seems to be not so important.
>>>>> I think we should outright reject any patch without compression type
>>>>> negotiation.
>>>> Does it mean that it is necessary to support multiple compression
>>>> algorithms and make it possible to perform switch between them at
>>>> runtime?
>>> IMHO the negotiation should happen at connection time, i.e. the server
>>> should support connections compressed by different algorithms. Not sure
>>> if that's what you mean by runtime.
>>>
>>> AFAICS this is quite close to how negotiation of encryption algorithms
>>> works, in TLS and so on. Client specifies supported algorithms, server
>>> compares that to list of supported algorithms, deduces the encryption
>>> algorithm and notifies the client.
>>>
>>> To allow fall-back to uncompressed connection, use "none" as algorithm.
>>> If there's no common algorithm, fail.
>> It is good analogue with SSL.
>> Yes, SSL protocol provides several ways of authentication, encryption,...
>> And there are several different libraries implementing SSL.
>> But Postgres is using only one of them: OpenSSL.
>> If I want to use some other library (for example to make it possible to
>> serialize and pass SSL session state to other
>> process), then there is no way to achieve it.
>>
> That's rather misleading. Firstly, it's true we only support OpenSSL at
> the moment, but I do remember we've been working on adding support to a
> bunch of other TLS libraries.
>
> But more importantly, it's not the TLS library that's negotiated. It's
> the encryption algorithms that is negotiated. The server is oblivious
> which TLS library is used by the client (and vice versa), because the
> messages are the same - what matters is that they agree on keys,
> ciphers, etc. And those can differ/change between libraries or even
> versions of the same library.
>
> For us, the situation is the same - we have the messages specified by
> the FE/BE protocol, and it's the algorithms that are negotiated.
>
>> Actually zstd also includes implementations of several compression
>> algorithms and it choose one of them best fitting particular data
>> stream. As in case of SSL, choice of algorithm is performed internally
>> inside zstd - not at libpq level.
>>
> Really? I always thought zstd is a separate compression algorithm.
> There's adaptive compression feature, but AFAIK that essentially tweaks
> compression level based on network connection. Can you point me to the
> sources or docs explaining this?
>
> Anyway, this does not really change anything - it's internal zstd stuff.
>
>> Sorry, if my explanation about static and dynamic (at runtime) choice
>> were not correct.
>> This is how compression is toggled now:
>>
>> #if HAVE_LIBZSTD
>> ZpqStream*
>> zpq_create(zpq_tx_func tx_func, zpq_rx_func rx_func, void *arg)
>> {
>> ...
>> }
>> #endif
>>
>> So if Postgres was configured with zstd, then this implementation is
>> included inclient and server Postgres libraries.
>> If postgres is configures with zlib, them  zlib implementation will be
>> used.
>> This is similar with using compression and most of other configurable
>> features in Postgres.
>>
>> If we want to provide dynamic choice at runtime, then we need to have
>> array with available compression algorithms:
>>
>> #if HAVE_LIBZSTD
>> static ZpqStream*
>> zstd_create(zpq_tx_func tx_func, zpq_rx_func rx_func, void *arg)
>> {
>> ...
>> }
>> #endif
>>
>> ZpqCompressorImpl compressorImpl[] =
>> {
>> #if HAVE_LIBZSTD
>> {zstd_create, zstd_read,zstd_write,...},
>> #endif
>> #if HAVE_ZLIB
>> {zlib_create, zlib_read,zslib_write,...},
>> #endif
>> ...
>> }
>>
> Yes, that's mostly what I've been imagining, except that you also need
> some sort of identifier for the algorithm - a cstring at the beginning
> of the struct should be enough, I guess.
>
>> And the most interesting case is that if we load library dynamically.
>> Each implementation is generated in separate library (for  example
>> libpztd.so).
>> In this case we need to somehow specify available libraries.
>> For example by placing them in separate directory, or specifying list of
>> libraries in postgresql.conf.
>> Then we try to load this library using dlopen.  Such library has
>> external dependencies of correspondent compressor library (for example
>> -lz). The library can be successfully loaded if there correspond
>> compressor implementation was install at the system.
>> This is most flexible approach allowing to provide custom implementation
>> of compressors.
>> Compression implementation can be organized as Postgres extension and
>> its PG_init function registers this implementation in some list.
>>
> How you could make them as extensions? Those are database-specific and
> the authentication happens before you have access to the database.
>
> As I said before, I think adding them using shared_preload_libraries and
> registering them in _PG_init should be sufficient.
>
>> This is what I am asking about.
>> Right now approach 1) is implemented: compression algorithm is defined
>> by configure.
>> It is no so difficult to extend it to support multiple algorithms.
>> And the most flexible but more sophisticated is to load libraries
>> dynamically.
>>
> Well, there's nothing stopping you from implementing the dynamic
> loading, but IMHO it makes v1 unnecessarily complex.
>
>>>> Right now compression algorithm is linked statically.
>>>> Negotiation of compression type is currently performed but it only
>>>> checks that server and client are implementing the same algorithm and
>>>> disables compression if it is not true.
>>>>
>>> I don't think we should automatically fall-back to disabled compression,
>>> when a client specifies compression algorithm.
>> Compression is disabled only when client and server were configured with
>> different compression algorithms (i.e. zstd and zlib).
>>
> Yes, and I'm of the opinion we shouldn't do that, unless unless both
> sides explicitly enable that in some way.
>
>>>> If we are going to support multiple compression algorithms, do we need
>>>> dynamic loading of correspondent compression libraries or static linking
>>>> is ok? In case of dynamic linking we need to somehow specify information
>>>> about available compression algorithms.
>>>> Some special subdirectory for them so that I can traverse this directory
>>>> and try to load correspondent libraries?
>>>>
>>>> Only I find it too complicated for the addressed problem?
>>>>
>>> I don't think we need dynamic algorithms v1, but IMHO it'd be pretty
>>> simple to do - just add a shared_preload_library which registers it in a
>>> list in memory.
>> I do not think that it is necessary to include such libraries in
>> preload_shared_libraries list.
>> It can be done lazily only of compression is requested by client.
>> Also please notice that we need to load compression library both at
>> server and client sides.
>> preload_shared_libraries works only for postmaster.
>>
> How would you know which libraries to load for a given compression
> algorithm? Surely, loading all available libraries just because they
> might happen to implement the requested algorithm seems bad? IMHO the
> shared_preload_libraries is a much safer (and working) approach.
>
> But I'd just leave this aside, because trying to pack all of this into
> v1 just increases the likelihood of it not getting committed in time.
> And the fact that we don't have any such infrastructure in the client
> just increases the risk.
>
> +1 to go with hard-coded list of supported algorithms in v1
>
> regars
>
Ok, I will implement support of multiple configured compression algorithms.
Concerning usage of several different compression algorithms in zsd - I
was not correct.
It combines LZ77 with entropy encoding stageand can adaptively adjust
the compression ratio according to the load.

I will preface this with that I am not a security guy and that also do
not know how the Zstd vompression works, so take any of what I say with
a grain of salt.

On 2/8/19 8:14 AM, Andres Freund wrote:> I think compression is pretty
useful, and I'm not convinced that the
> threat model underlying the attacks on SSL really apply to postgres.
I think only because it is usually harder to intercept traffic between
the application server and the database than between the we bbrowser and
the web server.

Imagine the following query which uses the session ID from the cookie to
check if the logged in user has access to a file.

SELECT may_download_file(session_id => $1, path => $2);

When the query with its parameters is compressed the compressed size
will depend on the similarity between the session ID and the requested
path (assuming Zstd works similar to DEFLATE), so by tricking the web
browser into making requests with specifically crafted paths while
monitoring the traffic between the web server and the database the
compressed request size can be use to hone in the session ID and steal
people's login sessions, just like the CRIME attack[1].

So while compression is a very useful feature I am worried that it also
opens application developers to a new set of security vulnerabilities
which they previously were protected from when compression was removed
from SSL.

1. https://en.wikipedia.org/wiki/CRIME

Andreas

On 11.02.2019 2:36, Andreas Karlsson wrote:
> I will preface this with that I am not a security guy and that also do
> not know how the Zstd vompression works, so take any of what I say
> with a grain of salt.
>
> On 2/8/19 8:14 AM, Andres Freund wrote:> I think compression is pretty
> useful, and I'm not convinced that the
>> threat model underlying the attacks on SSL really apply to postgres.
> I think only because it is usually harder to intercept traffic between
> the application server and the database than between the we bbrowser
> and the web server.
>
> Imagine the following query which uses the session ID from the cookie
> to check if the logged in user has access to a file.
>
> SELECT may_download_file(session_id => $1, path => $2);
>
> When the query with its parameters is compressed the compressed size
> will depend on the similarity between the session ID and the requested
> path (assuming Zstd works similar to DEFLATE), so by tricking the web
> browser into making requests with specifically crafted paths while
> monitoring the traffic between the web server and the database the
> compressed request size can be use to hone in the session ID and steal
> people's login sessions, just like the CRIME attack[1].
>
> So while compression is a very useful feature I am worried that it
> also opens application developers to a new set of security
> vulnerabilities which they previously were protected from when
> compression was removed from SSL.
>
> 1. https://en.wikipedia.org/wiki/CRIME
>
> Andreas

Andreas? thank you for clarification. Such kind of attack is really
possible.
But as far as I understand  such attack requires injection between
server and database (to be able to analyze traffic between them).
Also such attack is possible only if session_id can be somehow
"guessed". If it is just big random number, then it is very unlikely
that it can be hacked in in this way.

But once again - I am not expert in cryptography.
And this patch is not addressing SSL vulnerabilities when using
compression - I agree, that compression at libpq level is not safer than
SSL level compression.
The goal was to support compression without using SSL. It seems to me
that there are many cases when security is not requires, but reducing
network traffic is desired.
The best example is replication between node in local network.

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

I attach new version of the patch which support choice between different
compression algorithms.
Right now only zstd and zlib are supported. If postgres is configured
with both of them, then zstd will be used.

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

On Mon, Feb 11, 2019 at 05:56:24PM +0300, Konstantin Knizhnik wrote:
>
> Also such attack is possible only if session_id can be somehow "guessed". If
> it is just big random number, then it is very unlikely that it can be hacked
> in in this way.

I am not arguing against compression, but this point isn't exactly true.
The _uniformity_ of the key makes a big difference in the practicality of
the attack, not the total entropy.

For example, if the session_id was a 128 bit hex string and I knew or
guessed the characters before the secret part and could send data that ended
up near the secret then I can guess one character at a time and infer
the guess is correct when the size of the packet gets smaller. IOW, I
really only have to guess with 1/16 odds each digit (because its a hex
string in this example).

In the case, the 128 bit secret only provides the effective protection
of an 8-bit secret because it can be guessed left to right 4 bits at a
time.

Garick

On 2019-Feb-11, Andreas Karlsson wrote:

> Imagine the following query which uses the session ID from the cookie to
> check if the logged in user has access to a file.
>
> SELECT may_download_file(session_id => $1, path => $2);
>
> When the query with its parameters is compressed the compressed size will
> depend on the similarity between the session ID and the requested path
> (assuming Zstd works similar to DEFLATE), so by tricking the web browser
> into making requests with specifically crafted paths while monitoring the
> traffic between the web server and the database the compressed request size
> can be use to hone in the session ID and steal people's login sessions, just
> like the CRIME attack[1].

I would have said that you'd never let the attacker eavesdrop into the
traffic between webserver and DB, but then that's precisely the scenario
you'd use SSL for, so I suppose that even though this attack is probably
just a theoretical risk at this point, it should definitely be
considered. Now, does this mean that we should forbid libpq compression
completely? I'm not sure -- maybe it's still usable if you encapsulate
that traffic so that the attackers cannot get at it, and there's no
reason to deprive those users of the usefulness of the feature. But
then we need documentation warnings pointing out the risks.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services