Date: Sun, 4 Aug 2019 13:52:37 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Petr Jelinek <petr@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>,
	Andrey Borodin <x4mmm@yandex-team.ru>,
	Konstantin Knizhnik <k.knizhnik@postgrespro.ru>,
	Michael Paquier <michael@paquier.xyz>,
	pgsql-hackers <pgsql-hackers@postgresql.org>,
	Vladimir Leskov <vladimirlesk@yandex-team.ru>
Subject: Re: pglz performance
Message-ID: <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <d8576096-76ba-487d-515b-44fdedba8bb5@2ndquadrant.com>
User-Agent: NeoMutt/20180716-1444-295967
Precedence: bulk

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?


A very brief review:


1) I wonder what "runstatedir" is about.


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


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


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.


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.


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?


regards

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