Date: Tue, 28 Jun 2022 08:14:25 +0000
To: Justin Pryzby <pryzby@telsasoft.com>
From: gkokolatos@pm.me
Cc: pgsql-hackers@lists.postgresql.org,
 Rachel Heaton <rachelmheaton@gmail.com>
Reply-To: gkokolatos@pm.me
Subject: Re: Add LZ4 compression in pg_dump
Message-ID: 
 <UsTnh6pNlF-Kichjme-H98KcAIycxLqV_BNBh4OvGLinGadL2KlsRlW2LHzB8qDbABTrLJ6CSTtENifm7mXOinRUXftSwZ3_rWj7vsPYVAc=@pm.me>
In-Reply-To: <20220626155532.GA9311@telsasoft.com>
References: <20220626155532.GA9311@telsasoft.com>
Feedback-ID: 6383013:user:proton
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/UsTnh6pNlF-Kichjme-H98KcAIycxLqV_BNBh4OvGLinGadL2KlsRlW2LHzB8qDbABTrLJ6CSTtENifm7mXOinRUXftSwZ3_rWj7vsPYVAc%3D%40pm.me>
Precedence: bulk


------- Original Message -------
On Sunday, June 26th, 2022 at 5:55 PM, Justin Pryzby <pryzby@telsasoft.com>=
 wrote:


>
>
> Hi,
>
> Will you be able to send a rebased patch for the next CF ?

Thank you for taking an interest in the PR. The plan is indeed to sent
a new version.

> If you update for the review comments I sent in March, I'll plan to do an=
other
> round of review.

Thank you.

>
> On Sat, Mar 26, 2022 at 11:21:56AM -0500, Justin Pryzby wrote:
>
> > LZ4F_HEADER_SIZE_MAX isn't defined in old LZ4.
> >
> > I ran into that on an ubuntu LTS, so I don't think it's so old that it
> > shouldn't be handled more gracefully. LZ4 should either have an explici=
t
> > version check, or else shouldn't depend on that feature (or should defi=
ne a
> > safe fallback version if the library header doesn't define it).
> >
> > https://packages.ubuntu.com/liblz4-1
> >
> > 0003: typo: of legacy =3D> or legacy
> >
> > There are a large number of ifdefs being added here - it'd be nice to m=
inimize
> > that. basebackup was organized to use separate files, which is one way.
> >
> > $ git grep -c 'ifdef .*LZ4' src/bin/pg_dump/compress_io.c
> > src/bin/pg_dump/compress_io.c:19
> >
> > In last year's CF entry, I had made a union within CompressorState. LZ4
> > doesn't need z_streamp (and ztsd will need ZSTD_outBuffer, ZSTD_inBuffe=
r,
> > ZSTD_CStream).
> >
> > 0002: I wonder if you're able to re-use any of the basebackup parsing s=
tuff
> > from commit ffd53659c. You're passing both the compression method and l=
evel.
> > I think there should be a structure which includes both. In the future,=
 that
> > can also handle additional options. I hope to re-use these same things =
for
> > wal_compression=3Dmethod:level.
> >
> > You renamed this:
> >
> > |- COMPR_ALG_LIBZ
> > |-} CompressionAlgorithm;
> > |+ COMPRESSION_GZIP,
> > |+} CompressionMethod;
> >
> > ..But I don't think that's an improvement. If you were to change it, it=
 should
> > say something like PGDUMP_COMPRESS_ZLIB, since there are other compress=
ion
> > structs and typedefs. zlib is not idential to gzip, which uses a differ=
ent
> > header, so in WriteDataToArchive(), LIBZ is correct, and GZIP is incorr=
ect.
> >
> > The cf* changes in pg_backup_archiver could be split out into a separat=
e
> > commit. It's strictly a code simplification - not just preparation for =
more
> > compression algorithms. The commit message should "See also:
> > bf9aa490db24b2334b3595ee33653bf2fe39208c".
> >
> > The changes in 0002 for cfopen_write seem insufficient:
> > |+ if (compressionMethod =3D=3D COMPRESSION_NONE)
> > |+ fp =3D cfopen(path, mode, compressionMethod, 0);
> > | else
> > | {
> > | #ifdef HAVE_LIBZ
> > | char *fname;
> > |
> > | fname =3D psprintf("%s.gz", path);
> > |- fp =3D cfopen(fname, mode, compression);
> > |+ fp =3D cfopen(fname, mode, compressionMethod, compressionLevel);
> > | free_keep_errno(fname);
> > | #else
> >
> > The only difference between the LIBZ and uncompressed case is the file
> > extension, and it'll be the only difference with LZ4 too. So I suggest =
to
> > first handle the file extension, and the rest of the code path is not
> > conditional on the compression method. I don't think cfopen_write even =
needs
> > HAVE_LIBZ - can't you handle that in cfopen_internal() ?
> >
> > This patch rejects -Z0, which ought to be accepted:
> > ./src/bin/pg_dump/pg_dump -h /tmp regression -Fc -Z0 |wc
> > pg_dump: error: can only specify -Z/--compress [LEVEL] when method is s=
et
> >
> > Your 0003 patch shouldn't reference LZ4:
> > +#ifndef HAVE_LIBLZ4
> > + if (*compressionMethod =3D=3D COMPRESSION_LZ4)
> > + supports_compression =3D false;
> > +#endif
> >
> > The 0004 patch renames zlibOutSize to outsize - I think the patch serie=
s should
> > be constructed such as to minimize the size of the method-specific patc=
hes. I
> > say this anticipating also adding support for zstd. The preliminary pat=
ches
> > should have all the boring stuff. It would help for reviewing to keep t=
he
> > patches split up, or to enumerate all the boring things that are being =
renamed
> > (like change OutputContext to cfp, rename zlibOutSize, ...).
> >
> > 0004: The include should use <lz4.h> and not "lz4.h"
> >
> > freebsd/cfbot is failing.
> >
> > I suggested off-list to add an 0099 patch to change LZ4 to the default,=
 to
> > exercise it more on CI.
>
>
> On Sat, Mar 26, 2022 at 01:33:36PM -0500, Justin Pryzby wrote:
>
> > On Sat, Mar 26, 2022 at 11:21:56AM -0500, Justin Pryzby wrote:
> >
> > > You're passing both the compression method and level. I think there s=
hould
> > > be a structure which includes both. In the future, that can also hand=
le
> > > additional options.
> >
> > I'm not sure if there's anything worth saving, but I did that last year=
 with
> > 0003-Support-multiple-compression-algs-levels-opts.patch
> > I sent a rebased copy off-list.
> > https://www.postgresql.org/message-id/flat/20210104025321.GA9712@telsas=
oft.com#ca1b9f9d3552c87fa874731cad9d8391
> >
> > | fatal("not built with LZ4 support");
> > | fatal("not built with lz4 support");
> >
> > Please use consistent capitalization of "lz4" - then the compiler can o=
ptimize
> > away duplicate strings.
> >
> > > 0004: The include should use <lz4.h> and not "lz4.h"
> >
> > Also, use USE_LZ4 rather than HAVE_LIBLZ4, per 75eae0908.
>
>