Date: Tue, 24 Jan 2023 19:42:03 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: gkokolatos@pm.me
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>,
	Michael Paquier <michael@paquier.xyz>,
	pgsql-hackers@lists.postgresql.org,
	Rachel Heaton <rachelmheaton@gmail.com>
Subject: Re: Add LZ4 compression in pg_dump
Message-ID: <20230125014203.GL13860@telsasoft.com>
References: <IZzs7Z2ny5hRK1EhTynSg3DnCo_z-UvEJN4Q6Hf0CUHWx3BcNwt-2z5vvvvA-Z8PQL-AYiL9YQPlKylATZ_Z6HnJ55YZUHgL66H9MZQFi48=@pm.me>
 <41036470-041b-08a3-469d-fe07de2bc58e@enterprisedb.com>
 <ljb5G0qPPj_MM5y-C5fRT24uuuj6n5I5RY__MpT7JdQ_B_AIE8vIOgdRO1uiBee6cymDzFLAKbYh3wDFv86jprLhVKOdx70D4g_4JTjqnY0=@pm.me>
 <6ab89d42-0345-b96e-02d4-68e171cdbe03@enterprisedb.com>
 <7zu6GYFi72jByVk2SEe_nd8r1DHhwA35vSntAE9sAfiMX9Lfk6bwimkwmu7GDgYaEvL27SE4qF1uK4_LHWQM4H-SXOwkMAI-GV8PKGmED_E=@pm.me>
 <d8cbc586-a8e3-1662-c158-9022d5c8de83@enterprisedb.com>
 <ce49a4f6-f7cc-28d7-3761-ddee7ef732af@enterprisedb.com>
 <MMLKPM28EzV5-NC4Jd3YdWCmPlUvbpbyqrugtklWTHZwF24g5x2IJ1J1wTICSESs8CaIRdj-56eDIqC4cTpahm55PmTVRik2D77X0ZKGeVo=@pm.me>
 <20230123180014.GX13860@telsasoft.com>
 <YiPrsNILgul-hbeyD6tfNaRUK5JZEhaZE36dLe1YJvKN_i0Tm2GuHd246MqcfNGFsjSY-rNo9n54MFCrRiRHRDkYyjgE4e29V-diAN2EcNU=@pm.me>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YiPrsNILgul-hbeyD6tfNaRUK5JZEhaZE36dLe1YJvKN_i0Tm2GuHd246MqcfNGFsjSY-rNo9n54MFCrRiRHRDkYyjgE4e29V-diAN2EcNU=@pm.me>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: <https://www.postgresql.org/message-id/20230125014203.GL13860%40telsasoft.com>
Precedence: bulk

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 in this 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