Re: Add LZ4 compression in pg_dump

On Tue, Jan 24, 2023 at 03:56:20PM +0000, gkokolatos@pm.me wrote:
> On Monday, January 23rd, 2023 at 7:00 PM, Justin Pryzby <pryzby@telsasoft.com> wrote:
> > On Mon, Jan 23, 2023 at 05:31:55PM +0000, gkokolatos@pm.me wrote:
> > 
> > > Please find attached v23 which reintroduces the split.
> > > 
> > > 0001 is reworked to have a reduced footprint than before. Also in an attempt
> > > to facilitate the readability, 0002 splits the API's and the uncompressed
> > > implementation in separate files.
> > 
> > Thanks for updating the patch. Could you address the review comments I
> > sent here ?
> > https://www.postgresql.org/message-id/20230108194524.GA27637%40telsasoft.com
> 
> Please find v24 attached.

Thanks for updating the patch.

In 001, RestoreArchive() does:

> -#ifndef HAVE_LIBZ
> -       if (AH->compression_spec.algorithm == PG_COMPRESSION_GZIP &&
> -               AH->PrintTocDataPtr != NULL)
> +       supports_compression = false;
> +       if (AH->compression_spec.algorithm == PG_COMPRESSION_NONE ||
> +               AH->compression_spec.algorithm == PG_COMPRESSION_GZIP)
> +               supports_compression = true;
> +
> +       if (AH->PrintTocDataPtr != NULL)
>         {
>                 for (te = AH->toc->next; te != AH->toc; te = te->next)
>                 {
>                         if (te->hadDumper && (te->reqs & REQ_DATA) != 0)
> -                               pg_fatal("cannot restore from compressed archive (compression not supported in this
installation)");
> +                       {
> +#ifndef HAVE_LIBZ
> +                               if (AH->compression_spec.algorithm == PG_COMPRESSION_GZIP)
> +                                       supports_compression = false;
> +#endif
> +                               if (supports_compression == false)
> +                                       pg_fatal("cannot restore from compressed archive (compression not supported
inthis installation)");
 
> +                       }
>                 }
>         }
> -#endif

This first checks if the algorithm is implemented, and then checks if
the algorithm is supported by the current build - that confused me for a
bit.  It seems unnecessary to check for unimplemented algorithms before
looping.  That also requires referencing both GZIP and LZ4 in two
places.

I think it could be written to avoid the need to change for added
compression algorithms:

+                       if (te->hadDumper && (te->reqs & REQ_DATA) != 0)
+                       {
+                               /* Check if the compression algorithm is supported */
+                               pg_compress_specification spec;
+                               parse_compress_specification(AH->compression_spec.algorithm, NULL, &spec);
+                               if (spec->parse_error != NULL)
+                                       pg_fatal(spec->parse_error);
+                       }

Or maybe add a new function to compression.c to indicate whether a given
algorithm is supported.

That would also indicate *which* compression library isn't supported.

Other than that, I think 001 is ready.

002/003 use these names, which I think are too similar - initially I
didn't even realize there were two separate functions (each with a
second stub function to handle the case of unsupported compression):

+extern void InitCompressorGzip(CompressorState *cs, const pg_compress_specification compression_spec);
                                                                                                    
 
+extern void InitCompressGzip(CompressFileHandle *CFH, const pg_compress_specification compression_spec);
                                                                                                        
 

+extern void InitCompressorLZ4(CompressorState *cs, const pg_compress_specification compression_spec);
                                                                                                          
 
+extern void InitCompressLZ4(CompressFileHandle *CFH, const pg_compress_specification compression_spec);
                                                                                                          
 

typo:
s/not build with/not built with/

Should AllocateCompressor() set cs->compression_spec, rather than doing
it in each compressor ?

Thanks for considering.

-- 
Justin