------- Original Message -------
On Saturday, February 25th, 2023 at 3:05 PM, Justin Pryzby
<pry...@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
