Re: Teach pg_receivewal to use lz4 compression

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

On Friday, July 9th, 2021 at 04:49, Michael Paquier <michael(at)paquier(dot)xyz> wrote:

Hi,

please find v3 of the patch attached, rebased to the current head.

> > Michael Paquier wrote:
> >
>
> * http://www.zlib.org/rfc-gzip.html.
>
> - - For lz4 compressed segments
> */
> This comment is incomplete.

Fixed.

> +#define IsLZ4CompressXLogFileName(fname) \
> - (strlen(fname) == XLOG_FNAME_LEN + strlen(".lz4") && \
> - strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN && \
> - strcmp((fname) + XLOG_FNAME_LEN, ".lz4") == 0)
>
> +#define IsLZ4PartialCompressXLogFileName(fname) \
> - (strlen(fname) == XLOG_FNAME_LEN + strlen(".lz4.partial") && \
> - strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN && \
> - strcmp((fname) + XLOG_FNAME_LEN, ".lz4.partial") == 0)
>
> This is getting complicated. Would it be better to change this stuff
> and switch to a routine that checks if a segment has a valid name, is
> partial, and the type of compression that applied to it? It seems to
> me that we should group iszlibcompress and islz4compress together with
> the options available through compression_method.

Agreed and done.

> - if (compresslevel != 0)
> - {
> - if (compression_method == COMPRESSION_NONE)
> - {
> - compression_method = COMPRESSION_ZLIB;
> - }
> - if (compression_method != COMPRESSION_ZLIB)
> - {
> - pg_log_error("cannot use --compress when "
> - "--compression-method is not gzip");
> - fprintf(stderr, _("Try \\"%s --help\\" for more information.\\n"),
> - progname);
> - exit(1);
> - }
> - }
>
> For compatibility where --compress enforces the use of zlib that would
> work, but this needs a comment explaining the goal of this block. I
> am wondering if it would be better to break the will and just complain
> when specifying --compress without --compression-method though. That
> would cause compatibility issues, but this would make folks aware of
> the presence of LZ4, which does not sound bad to me either as ZLIB is
> slower than LZ4 on all fronts.

Fair point. In v3 of the patch --compress requires --compression-method. Passing
0 as value errors out.

> - else if (compression_method == COMPRESSION_ZLIB)
> - {
> - pg_log_error("cannot use --compression-method gzip when "
> - "--compression is 0");
> - fprintf(stderr, _("Try \\"%s --help\\" for more information.\\n"),
> - progname);
> - exit(1);
> - }
>
> Hmm. It would be more natural to enforce a default compression level
> in this case? The user is asking for a compression with zlib here.

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.

Cheers,
//Georgios

> ---------------------------------------------------------------
>
> Michael