From: Petr Jelinek <petr@2ndquadrant.com>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: Vladimir Leskov <vladimirlesk@yandex-team.ru>
Subject: Re: pglz performance
Date: Sun, 4 Aug 2019 17:53:26 +0200
Message-ID: <a5cbd707-e4a4-70e7-e970-75f1cc68c20f@2ndquadrant.com> (raw)
In-Reply-To: <20190804115237.nliwf6hhaqfd62ps@development>
References: <ea57b49a-ecf0-481a-a77b-631833354f7d@postgrespro.ru>
<20190802144345.d62jtiyyx6r2y73f@development>
<DBB2A9E5-29FD-40BF-AC60-BD990FBF142F@yandex-team.ru>
<20190802163948.i6mjypdgujeorrbi@alap3.anarazel.de>
<20190802170039.o4pabnzm4xy3z7uj@development>
<20190802171258.nob5zo2vu7siqo2b@alap3.anarazel.de>
<20190802175239.5ds6rhdso6hwy4vk@development>
<20190802182003.u477q6itwudieqxf@alap3.anarazel.de>
<20190802194859.qxzxq2achzottkup@development>
<d8576096-76ba-487d-515b-44fdedba8bb5@2ndquadrant.com>
<20190804115237.nliwf6hhaqfd62ps@development>
Hi,
On 04/08/2019 13:52, Tomas Vondra wrote:
> On Sun, Aug 04, 2019 at 02:41:24AM +0200, Petr Jelinek wrote:
>> Hi,
>>
>> On 02/08/2019 21:48, Tomas Vondra wrote:
>>> On Fri, Aug 02, 2019 at 11:20:03AM -0700, Andres Freund wrote:
>>>
>>>>
>>>>> Another question is whether we'd actually want to include the code in
>>>>> core directly, or use system libraries (and if some packagers might
>>>>> decide to disable that, for whatever reason).
>>>>
>>>> I'd personally say we should have an included version, and a
>>>> --with-system-... flag that uses the system one.
>>>>
>>>
>>> OK. I'd say to require a system library, but that's a minor detail.
>>>
>>
>> Same here.
>>
>> Just so that we don't idly talk, what do you think about the attached?
>> It:
>> - adds new GUC compression_algorithm with possible values of pglz
>> (default) and lz4 (if lz4 is compiled in), requires SIGHUP
>> - adds --with-lz4 configure option (default yes, so the configure
>> option is actually --without-lz4) that enables the lz4, it's using
>> system library
>> - uses the compression_algorithm for both TOAST and WAL compression
>> (if on)
>> - supports slicing for lz4 as well (pglz was already supported)
>> - supports reading old TOAST values
>> - adds 1 byte header to the compressed data where we currently store
>> the algorithm kind, that leaves us with 254 more to add :) (that's an
>> extra overhead compared to the current state)
>> - changes the rawsize in TOAST header to 31 bits via bit packing
>> - uses the extra bit to differentiate between old and new format
>> - supports reading from table which has different rows stored with
>> different algorithm (so that the GUC itself can be freely changed)
>>
>
> Cool.
>
>> Simple docs and a TAP test included.
>>
>> I did some basic performance testing (it's not really my thing though,
>> so I would appreciate if somebody did more).
>> I get about 7x perf improvement on data load with lz4 compared to pglz
>> on my dataset but strangely only tiny decompression improvement.
>> Perhaps more importantly I also did before patch and after patch tests
>> with pglz and the performance difference with my data set was <1%.
>>
>> Note that this will just link against lz4, it does not add lz4 into
>> PostgreSQL code-base.
>>
>
> WFM, although I think Andres wanted to do both (link against system and
> add lz4 code as a fallback). I think the question is what happens when
> you run with lz4 for a while, and then switch to binaries without lz4
> support. Or when you try to replicate from lz4-enabled instance to an
> instance without it. Especially for physical replication, but I suppose
> it may affect logical replication with binary protocol?
>
I generally prefer having system library, we don't include for example
ICU either.
>
> A very brief review:
>
>
> 1) I wonder what "runstatedir" is about.
>
No idea, that stuff is generated by autoconf from configure.in.
>
> 2) This seems rather suspicious, and obviously the comment is now
> entirely bogus:
>
> /* Check that off_t can represent 2**63 - 1 correctly.
> We can't simply define LARGE_OFF_T to be 9223372036854775807,
> since some C++ compilers masquerading as C compilers
> incorrectly reject 9223372036854775807. */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31)
> << 31))
>
Same as above. TBH I am not sure why we even include configure in git
repo given that different autoconf versions can build different outputs.
>
> 3) I can't really build without lz4:
>
> config.status: linking src/makefiles/Makefile.linux to src/Makefile.port
> pg_lzcompress.c: In function ‘pg_compress_bound’:
> pg_lzcompress.c:892:22: error: ‘SIZEOF_PG_COMPRESS_HEADER’ undeclared
> (first use in this function)
> return slen + 4 + SIZEOF_PG_COMPRESS_HEADER;
> ^~~~~~~~~~~~~~~~~~~~~~~~~
> pg_lzcompress.c:892:22: note: each undeclared identifier is reported
> only once for each function it appears in
>
Okay, that's just problem of SIZEOF_PG_COMPRESS_HEADER being defined
inside the HAVE_LZ4 ifdef while it should be defined above ifdef.
>
> 4) I did a simple test with physical replication, with lz4 enabled on
> both sides (well, can't build without lz4 anyway, per previous point).
> It immediately failed like this:
>
> FATAL: failed to restore block image
> CONTEXT: WAL redo at 0/5000A40 for Btree/INSERT_LEAF: off 138
> LOG: startup process (PID 15937) exited with exit code 1
>
> This is a simple UPDATE on a trivial table:
>
> create table t (a int primary key);
> insert into t select i from generate_series(1,1000) s(i);
> update t set a = a - 100000 where random () < 0.1;
>
> with some checkpoints to force FPW (and wal_compression=on, of course).
>
> I haven't tried `make check-world` but I suppose some of the TAP tests
> should fail because of this. And if not, we need to improve coverage.
>
FWIW I did run check-world without problems, will have to look into this.
>
> 5) I wonder why compression_algorithm is defined as PGC_SIGHUP. Why not
> to allow users to set it per session? I suppose we might have a separate
> option for WAL compression_algorithm.
>
Yeah I was thinking we might want to change wal_compression to enum as
well. Although that complicates the code quite a bit (the caller has to
decide algorithm instead compression system doing it).
>
> 6) It seems a bit strange that pg_compress/pg_decompress are now defined
> in pglz_compress.{c,h}. Maybe we should invent src/common/compress.c?
>
It does not seem worth it to invent new module for like 20 lines of
wrapper code.
--
Petr Jelinek
2ndQuadrant - PostgreSQL Solutions for the Enterprise
https://www.2ndQuadrant.com/
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: petr@2ndquadrant.com, tomas.vondra@2ndquadrant.com, andres@anarazel.de, x4mmm@yandex-team.ru, k.knizhnik@postgrespro.ru, michael@paquier.xyz, vladimirlesk@yandex-team.ru
Subject: Re: pglz performance
In-Reply-To: <a5cbd707-e4a4-70e7-e970-75f1cc68c20f@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox