Re: Teach pg_receivewal to use lz4 compression

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

On Thursday, November 4th, 2021 at 9:21 AM, Michael Paquier <michael@paquier.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