From: Michael Paquier <michael@paquier.xyz>
To: gkokolatos@pm.me
Cc: Magnus Hagander <magnus@hagander.net>
Cc: Dilip Kumar <dilipbalaut@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Teach pg_receivewal to use lz4 compression
Date: Wed, 15 Sep 2021 15:46:50 +0900
Message-ID: <YUGW2mwx0eNnffoR@paquier.xyz> (raw)
In-Reply-To: <rYyZ3Fj9qayyY9-egNsV_kkLbL_BSWcOEdi3Mb6M9eQRTkcA2jrqFEHglLUEYnzWR_wttCqn7VI94MZ2p7mwNje51lHTvWYnJ1jHdOceen4=@pm.me>
References: <CAFiTN-urjKKCwu+YA5VYLG40_RLst-B=BxZNKXGpuTgh8hoFtA@mail.gmail.com>
<CABUevEyTDC1uEbF2PccLdA6hVGuqFuYemYimrBToLupu2usmjw@mail.gmail.com>
<A6gxTGqoplZLSOzN0MS2zsutUZdFFEMhWmcrcb9MLvJ3MJ_ylD4X-g0iqHW9cB6D-dTAgf4Ep1aQoZ9UuFf9k3DBI2gZoYmZk_BFBFX2_ow=@pm.me>
<CABUevEzuc02iacteSq78A7ws6gP7k7TSSG4FHum6uBq8+xTNig@mail.gmail.com>
<S2rA_l89FgB0CtpkaBoPe0DTjvkvo46zUzlZxilMGzlDCICb5rwmrKXg1bzpp5ZO75GF-b7ayvG9dihbzRwOKVawLLreJbWWGfY6OdhSpho=@pm.me>
<YN5nePsQJhMLhznM@paquier.xyz>
<J4hpev8z-ElCT_rFLwiDypcSk3fgq9llb0FbEzH74eg7YPVQPSn4KaAi_Rd63ndvY8Bcv-zoo0VOZbfCZSRfOobRazlCLuOl8S7pUC1mRlE=@pm.me>
<PZnyf3PE6s8jQBf-CTQjAvjSRvw8dWWxYrLWR2NdqTrhm7wHfrQ2gUgwdlmGevfIxindJ3KooyuWQ5MRU9NTyMRiNMfRBkyVnqkyPH7nP_k=@pm.me>
<YOe5LouYuxXOrL0H@paquier.xyz>
<rYyZ3Fj9qayyY9-egNsV_kkLbL_BSWcOEdi3Mb6M9eQRTkcA2jrqFEHglLUEYnzWR_wttCqn7VI94MZ2p7mwNje51lHTvWYnJ1jHdOceen4=@pm.me>
On Fri, Sep 10, 2021 at 08:21:51AM +0000, gkokolatos@pm.me wrote:
> Agreed. A default value of 5, which is in the middle point of options, has been
> defined and used.
>
> In addition, the tests have been adjusted to mimic the newly added gzip tests.
Looking at lz4frame.h, there is LZ4F_flush() that allows to compress
immediately any data buffered in the frame context but not compressed
yet. It seems to me that dir_sync() should be extended to support
LZ4.
export GZIP_PROGRAM=$(GZIP)
+export LZ4
[...]
+PGAC_PATH_PROGS(LZ4, lz4)
+
PGAC_PATH_BISON
The part of the test assigning LZ4 is fine, but I'd rather switch to a
logic à-la-gzip, where we just save "lz4" in Makefile.global.in,
saving cycles in ./configure.
+static bool
+is_xlogfilename(const char *filename, bool *ispartial,
+ WalCompressionMethod *wal_compression_method)
I like the set of simplifications you have done here to detection if a
segment is partial and which compression method applies to it.
+ if (compression_method != COMPRESSION_ZLIB && compresslevel != 0)
+ {
+ pg_log_error("can only use --compress together with "
+ "--compression-method=gzip");
+#ifndef HAVE_LIBLZ4
+ pg_log_error("this build does not support compression via gzip");
+#endif
s/HAVE_LIBLZ4/HAVE_LIBZ/.
+$primary->command_fails(
+ [
+ 'pg_receivewal', '-D', $stream_dir, '--compression-method', 'lz4',
+ '--compress', '1'
+ ],
+ 'failure if --compression-method=lz4 specified with --compress');
This would fail when the code is not built with LZ4 with a non-zero
error code but with an error that is not what we expect. I think that
you should use $primary->command_fails_like() instead. That's quite
new, as of de1d4fe. The matching error pattern will need to change
depending on if we build the code with LZ4 or not. A simpler method
is to use --compression-method=none, to bypass the first round of
errors and make that build-independent, but that feels incomplete if
you want to tie that to LZ4.
+ pg_log_warning("compressed segment file \"%s\" has incorrect header size %lu, skipping",
+ dirent->d_name, consumed_len);
+ LZ4F_freeDecompressionContext(ctx);
I agree that skipping all those cases when calculating the streaming
start point is more consistent.
+ if (r < 0)
+ pg_log_error("could not read compressed file \"%s\": %m",
+ fullpath);
+ else
+ pg_log_error("could not read compressed file \"%s\": read %d of %lu",
+ fullpath, r, sizeof(buf));
Let's same in translation effort here by just using "could not read",
etc. by removing the term "compressed".
+ pg_log_error("can only use --compress together with "
+ "--compression-method=gzip");
Better to keep these in a single line to ease grepping. We don't care
if error strings are longer than the 72-80 character limit.
+/* Size of lz4 input chunk for .lz4 */
+#define LZ4_IN_SIZE 4096
Why this choice? Does it need to use LZ4_COMPRESSBOUND?
- if (dir_data->compression > 0)
+ if (dir_data->compression_method == COMPRESSION_ZLIB)
gzclose(gzfp);
else
Hm. The addition of the header in dir_open_for_write() uses
LZ4F_compressBegin. Shouldn't we use LZ4F_compressEnd() if
fsync_fname() or fsync_parent_path() fail on top of closing the fd?
That would be more consistent IMO to do so. The patch does that in
dir_close(). You should do that additionally if there is a failure
when writing the header.
+ pg_log_error("invalid compression-method \"%s\"", optarg);
+ exit(1);
This could be "invalid value \"%s\" for option %s", see
option_parse_int() in fe_utils/option_utils.c.
After running the TAP tests, the LZ4 section is failing as follows:
pg_receivewal: stopped log streaming at 0/4001950 (timeline 1)
pg_receivewal: not renaming "000000010000000000000004.partial", segment is not complete
pg_receivewal: error: could not close file "000000010000000000000004": Undefined error: 0
ok 26 - streaming some WAL using --compression-method=lz4
The third log line I am quoting here looks unexpected to me. Saying
that, the tests integrate nicely with the existing code.
As mentioned upthread, LZ4-compressed files don't store the file size
by default. I think that we should document that better in the code
and the documentation, in two ways at least:
- Add some comments mentioning lz4 --content-size, with at least one
in FindStreamingStart().
- Add a new paragraph in the documentation of --compression-method.
The name of the compression method is "LZ4" with upper-case
characters. Some comments in the code and the tests, as well as the
docs, are not careful about that.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
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: michael@paquier.xyz, gkokolatos@pm.me, magnus@hagander.net, dilipbalaut@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Teach pg_receivewal to use lz4 compression
In-Reply-To: <YUGW2mwx0eNnffoR@paquier.xyz>
* 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