libpq compression
Locked
1234
... 10
1234
... 10
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):
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):
-- Konstantin Knizhnik Postgres Professional: http://www.postgrespro.com The Russian Postgres Company |
libpq-compression-2.patch (35K)
Re: libpq compression
|
|
Re: libpq compression
|
|
Re: libpq compression
|
|
Re: libpq compression
|
|
Re: libpq compression
|
|
Re: libpq compression
|
|
Re: libpq compression
|
|
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. |
Re: libpq compression
|
|
Re: libpq compression
|
|
Re: libpq compression
|
|
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 |
Re: libpq compression
|
|
In reply to this post by Thomas Munro-3
On 05.06.2018 08:26, Thomas Munro wrote:
> On Thu, May 17, 2018 at 3:54 AM, Konstantin Knizhnik > <[hidden email]> 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' > 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 |
Re: libpq compression
|
|
Re: libpq compression
|
|
Re: libpq compression
|
|
Re: libpq compression
|
|
Re: libpq compression
|
|
Re: libpq compression
|
|
In reply to this post by konstantin knizhnik
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 |
Re: libpq compression
|
|
Re: libpq compression
|
|
In reply to this post by Thomas Munro-3
On 06.06.2018 02:03, Thomas Munro wrote: > On Wed, Jun 6, 2018 at 2:06 AM, Konstantin Knizhnik > <[hidden email]> 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 > -- Konstantin Knizhnik Postgres Professional: http://www.postgrespro.com The Russian Postgres Company |
| Free forum by Nabble | Edit this page |