Re: Teach pg_receivewal to use lz4 compression

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

On Thursday, November 4th, 2021 at 9:21 AM, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> > +#ifdef HAVE_LIBLZ4
> > + while (readp < readend)
> > + {
> > + size_t read_size = 1;
> > + size_t out_size = 1;
> > +
> > + status = LZ4F_decompress(ctx, outbuf, &out_size,
> > + readbuf, &read_size, NULL);
>
> And... It happens that the error from v9 is here, where we need to
> read the amount of remaining data from "readp", and not "readbuf" :)
>
> It is already late here, I'll continue on this stuff tomorrow, but
> this looks rather committable overall.

Thank you for v11 of the patch. Please find attached v12 which addresses a few
minor points.

Added an Oxford comma since the list now contains three or more terms:
- <option>--with-lz4</option>) and <literal>none</literal>.
+ <option>--with-lz4</option>), and <literal>none</literal>.

Removed an extra condinional check while switching over compression_method.
Instead of:
+ case COMPRESSION_LZ4:
+#ifdef HAVE_LIBLZ4
+ if (compresslevel != 0)
+ {
+ pg_log_error("cannot use --compress with
--compression-method=%s",
+ "lz4");
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+ progname);
+ exit(1);
+ }
+#else
+ if (compression_method == COMPRESSION_LZ4)
+ {
+ pg_log_error("this build does not support compression with %s",
+ "LZ4");
+ exit(1);
+ }
+ break;
+#endif

I opted for:
+ case COMPRESSION_LZ4:
+#ifdef HAVE_LIBLZ4
+ if (compresslevel != 0)
+ {
+ pg_log_error("cannot use --compress with
--compression-method=%s",
+ "lz4");
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+ progname);
+ exit(1);
+ }
+#else
+ pg_log_error("this build does not support compression with %s",
+ "LZ4");
+ exit(1);
+ #endif

There was an error while trying to find the streaming start. The code read:
+ else if (!ispartial && compression_method == COMPRESSION_LZ4)

where it should be instead:
+ else if (!ispartial && wal_compression_method == COMPRESSION_LZ4)

because compression_method is the global option exposed to the whereas
wal_compression_method is the local variable used to figure out what kind of
file the function is currently working with. This error was existing at least in
v9-0002 of $subject.

The variables readbuf and outbuf, used in the decompression of LZ4 files, are
now heap allocated.

Last, while the following is correct:
+ /*
+ * Once we have read enough data to cover one segment, we are
+ * done, there is no need to do more.
+ */
+ while (uncompressed_size <= WalSegSz)

I felt that converting it a do {} while () loop instead, will help with
readability:
+ do
+ {
<snip>
+ /*
+ * No need to continue reading the file when the uncompressed_size
+ * exceeds WalSegSz, even if there are still data left to read.
+ * However, if uncompressed_size is equal to WalSegSz, it should
+ * verify that there is no more data to read.
+ */
+ } while (r > 0 && uncompressed_size <= WalSegSz);

of course the check:
+ /* Done reading the file */
+ if (r == 0)
+ break;
midway the loop is no longer needed and thus removed.

I would like to have a bit more test coverage in the case for FindStreamingStart().
Specifically for the case that a lz4-compressed segment larger than WalSegSz exists.
The attached patch does not contain such test case. I am not very certain on how to
create such a test case reliably as it would be mostly based on a warning emitted
during the parsing of such a file.

Cheers,
//Georgios

> --
> Michael