public inbox for [email protected]
help / color / mirror / Atom feedFrom: Justin Pryzby <[email protected]>
To: [email protected]
Cc: Tomas Vondra <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: [email protected]
Cc: Rachel Heaton <[email protected]>
Subject: Re: Add LZ4 compression in pg_dump
Date: Tue, 24 Jan 2023 19:42:03 -0600
Message-ID: <[email protected]> (raw)
In-Reply-To: <YiPrsNILgul-hbeyD6tfNaRUK5JZEhaZE36dLe1YJvKN_i0Tm2GuHd246MqcfNGFsjSY-rNo9n54MFCrRiRHRDkYyjgE4e29V-diAN2EcNU=@pm.me>
References: <IZzs7Z2ny5hRK1EhTynSg3DnCo_z-UvEJN4Q6Hf0CUHWx3BcNwt-2z5vvvvA-Z8PQL-AYiL9YQPlKylATZ_Z6HnJ55YZUHgL66H9MZQFi48=@pm.me>
<[email protected]>
<ljb5G0qPPj_MM5y-C5fRT24uuuj6n5I5RY__MpT7JdQ_B_AIE8vIOgdRO1uiBee6cymDzFLAKbYh3wDFv86jprLhVKOdx70D4g_4JTjqnY0=@pm.me>
<[email protected]>
<7zu6GYFi72jByVk2SEe_nd8r1DHhwA35vSntAE9sAfiMX9Lfk6bwimkwmu7GDgYaEvL27SE4qF1uK4_LHWQM4H-SXOwkMAI-GV8PKGmED_E=@pm.me>
<[email protected]>
<[email protected]>
<MMLKPM28EzV5-NC4Jd3YdWCmPlUvbpbyqrugtklWTHZwF24g5x2IJ1J1wTICSESs8CaIRdj-56eDIqC4cTpahm55PmTVRik2D77X0ZKGeVo=@pm.me>
<[email protected]>
<YiPrsNILgul-hbeyD6tfNaRUK5JZEhaZE36dLe1YJvKN_i0Tm2GuHd246MqcfNGFsjSY-rNo9n54MFCrRiRHRDkYyjgE4e29V-diAN2EcNU=@pm.me>
On Tue, Jan 24, 2023 at 03:56:20PM +0000, [email protected] wrote:
> On Monday, January 23rd, 2023 at 7:00 PM, Justin Pryzby <[email protected]> wrote:
> > On Mon, Jan 23, 2023 at 05:31:55PM +0000, [email protected] 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
view thread (40+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Add LZ4 compression in pg_dump
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox