Re: Add LZ4 compression in pg_dump - Mailing list pgsql-hackers

------- Original Message -------
On Saturday, February 25th, 2023 at 3:05 PM, Justin Pryzby <pryzby@telsasoft.com> wrote:


>
>
> On Fri, Feb 24, 2023 at 11:02:14PM -0600, Justin Pryzby wrote:
>
> > I have some fixes (attached) and questions while polishing the patch for
> > zstd compression. The fixes are small and could be integrated with the
> > patch for zstd, but could be applied independently.


Please find some comments on the rest of the fixes patch that Tomas has not
commented on.

            can be compressed with the <application>gzip</application> or
-           <application>lz4</application>tool.
+           <application>lz4</application> tools.

+1

         The compression method can be set to <literal>gzip</literal> or
-        <literal>lz4</literal> or <literal>none</literal> for no compression.
+        <literal>lz4</literal>, or <literal>none</literal> for no compression.

I am not a native English speaker. Yet I think that if one adds commas
in one of the options, then one should add commas to all the options.
Namely, the aboveis missing a comma between gzip and lz4. However I
think that not having any commas still works grammatically and
syntactically.

-               /*
-                * A level of zero simply copies the input one block at the time. This
-                * is probably not what the user wanted when calling this interface.
-                */
-               if (cs->compression_spec.level == 0)
-                       pg_fatal("requested to compress the archive yet no level was specified");


I disagree with change. WriteDataToArchiveGzip() is far away from
what ever the code in pg_dump.c is doing. Any non valid values for
level will emit an error in when the proper gzip/zlib code is
called. A zero value however, will not emit such error. Having the
extra check there is a future proof guarantee in a very low cost.
Furthermore, it quickly informs the reader of the code about that
specific value helping with readability and comprehension.

If any change is required, something for which I vote strongly
against, I would at least recommend to replace it with an
assertion.

- * Initialize a compress file stream. Deffer the compression algorithm
+ * Initialize a compress file stream. Infer the compression algorithm

:+1:

-       # Skip command-level tests for gzip if there is no support for it.
+       # Skip command-level tests for gzip/lz4 if they're not supported.

We will be back at that again soon. Maybe change to:

Skip command-level test for unsupported compression methods

To include everything.


-               ($pgdump_runs{$run}->{compile_option} eq 'gzip' && !$supports_gzip) ||
-               ($pgdump_runs{$run}->{compile_option} eq 'lz4' && !$supports_lz4))
+               (($pgdump_runs{$run}->{compile_option} eq 'gzip' && !$supports_gzip) ||
+               ($pgdump_runs{$run}->{compile_option} eq 'lz4' && !$supports_lz4)))

Good catch, :+1:

Cheers,
//Georgios

> --
> Justin