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
