Re: Add LZ4 compression in pg_dump

------- 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 another
round of review.

Thank you.

Show quoted text

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 explicit
version check, or else shouldn't depend on that feature (or should define a
safe fallback version if the library header doesn't define it).

https://packages.ubuntu.com/liblz4-1

0003: typo: of legacy => or legacy

There are a large number of ifdefs being added here - it'd be nice to minimize
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_inBuffer,
ZSTD_CStream).

0002: I wonder if you're able to re-use any of the basebackup parsing stuff
from commit ffd53659c. You're passing both the compression method and level.
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=method: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 compression
structs and typedefs. zlib is not idential to gzip, which uses a different
header, so in WriteDataToArchive(), LIBZ is correct, and GZIP is incorrect.

The cf* changes in pg_backup_archiver could be split out into a separate
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 == COMPRESSION_NONE)
|+ fp = cfopen(path, mode, compressionMethod, 0);
| else
| {
| #ifdef HAVE_LIBZ
| char *fname;
|
| fname = psprintf("%s.gz", path);
|- fp = cfopen(fname, mode, compression);
|+ fp = 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 set

Your 0003 patch shouldn't reference LZ4:
+#ifndef HAVE_LIBLZ4
+ if (*compressionMethod == COMPRESSION_LZ4)
+ supports_compression = false;
+#endif

The 0004 patch renames zlibOutSize to outsize - I think the patch series should
be constructed such as to minimize the size of the method-specific patches. I
say this anticipating also adding support for zstd. The preliminary patches
should have all the boring stuff. It would help for reviewing to keep the
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 should
be a structure which includes both. In the future, that can also handle
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.
/messages/by-id/20210104025321.GA9712@telsasoft.com

| fatal("not built with LZ4 support");
| fatal("not built with lz4 support");

Please use consistent capitalization of "lz4" - then the compiler can optimize
away duplicate strings.

0004: The include should use <lz4.h> and not "lz4.h"

Also, use USE_LZ4 rather than HAVE_LIBLZ4, per 75eae0908.

------- 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 ?

Please find a rebased and heavily refactored patchset. Since parts of this
patchset were already committed, I restarted numbering. I am not certain if
this is the preferred way. This makes alignment with previous comments a bit
harder

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

I have updated for "some" of the comments. This is not an unwillingness to
incorporate those specific comments. Simply this patchset had started to divert
heavily already based on comments from Mr. Paquier who had already requested for
the APIs to be refactored to use function pointers. This is happening in 0002 of
the patchset. 0001 of the patchset is using the new compression.h under common.

This patchset should be considered a late draft, as commentary, documentation,
and some finer details are not yet finalized; because I am expecting the proposed
refactor to receive a wealth of comments. It would be helpful to understand if
the proposed direction is something worth to be worked upon, before moving to the
finer details.

For what is worth, I am the sole author of the current patchset.

Cheers,
//Georgios

Show quoted text

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 explicit
version check, or else shouldn't depend on that feature (or should define a
safe fallback version if the library header doesn't define it).

https://packages.ubuntu.com/liblz4-1

0003: typo: of legacy => or legacy

There are a large number of ifdefs being added here - it'd be nice to minimize
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_inBuffer,
ZSTD_CStream).

0002: I wonder if you're able to re-use any of the basebackup parsing stuff
from commit ffd53659c. You're passing both the compression method and level.
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=method: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 compression
structs and typedefs. zlib is not idential to gzip, which uses a different
header, so in WriteDataToArchive(), LIBZ is correct, and GZIP is incorrect.

The cf* changes in pg_backup_archiver could be split out into a separate
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 == COMPRESSION_NONE)
|+ fp = cfopen(path, mode, compressionMethod, 0);
| else
| {
| #ifdef HAVE_LIBZ
| char *fname;
|
| fname = psprintf("%s.gz", path);
|- fp = cfopen(fname, mode, compression);
|+ fp = 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 set

Your 0003 patch shouldn't reference LZ4:
+#ifndef HAVE_LIBLZ4
+ if (*compressionMethod == COMPRESSION_LZ4)
+ supports_compression = false;
+#endif

The 0004 patch renames zlibOutSize to outsize - I think the patch series should
be constructed such as to minimize the size of the method-specific patches. I
say this anticipating also adding support for zstd. The preliminary patches
should have all the boring stuff. It would help for reviewing to keep the
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 should
be a structure which includes both. In the future, that can also handle
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.
/messages/by-id/20210104025321.GA9712@telsasoft.com

| fatal("not built with LZ4 support");
| fatal("not built with lz4 support");

Please use consistent capitalization of "lz4" - then the compiler can optimize
away duplicate strings.

0004: The include should use <lz4.h> and not "lz4.h"

Also, use USE_LZ4 rather than HAVE_LIBLZ4, per 75eae0908.

This is a review of 0001.

On Tue, Jul 05, 2022 at 01:22:47PM +0000, gkokolatos@pm.me wrote:

Simply this patchset had started to divert
heavily already based on comments from Mr. Paquier who had already requested for
the APIs to be refactored to use function pointers. This is happening in 0002 of
the patchset.

I said something about reducing ifdefs, but I'm having trouble finding what
Michael said about this ?

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 explicit
version check, or else shouldn't depend on that feature (or should define a
safe fallback version if the library header doesn't define it).
https://packages.ubuntu.com/liblz4-1

The constant still seems to be used without defining a fallback or a minimum version.

0003: typo: of legacy => or legacy

This is still there

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 compression
structs and typedefs. zlib is not idential to gzip, which uses a different
header, so in WriteDataToArchive(), LIBZ is correct, and GZIP is incorrect.

This comment still applies - zlib's gz* functions are "gzip" but the others are
"zlib". https://zlib.net/manual.html

That affects both the 0001 and 0002 patches.

Actually, I think that "gzip" should not be the name of the user-facing option,
since (except for "plain" format) it isn't using gzip.

+Robert, since this suggests amending parse_compress_algorithm(). Maybe "zlib"
should be parsed the same way as "gzip" - I don't think we ever expose both to
a user, but in some cases (basebackup and pg_dump -Fp -Z1) the output is "gzip"
and in some cases NO it's zlib (pg_dump -Fc -Z1).

The cf* changes in pg_backup_archiver could be split out into a separate
commit. It's strictly a code simplification - not just preparation for more
compression algorithms. The commit message should "See also:
bf9aa490db24b2334b3595ee33653bf2fe39208c".

I still think this could be an early, 0000 patch.

freebsd/cfbot is failing.

This is still failing for bsd, windows and compiler warnings.
Windows also has compiler warnings.
http://cfbot.cputube.org/georgios-kokolatos.html

Please see: src/tools/ci/README, which you can use to run check-world on 4 OS
by pushing a branch to github.

I suggested off-list to add an 0099 patch to change LZ4 to the default, to
exercise it more on CI.

What about this ? I think the patch needs to pass CI on all 4 OS with
default=zlib and default=lz4.

On Sat, Mar 26, 2022 at 01:33:36PM -0500, Justin Pryzby wrote:

@@ -254,7 +251,12 @@ CreateArchive(const char *FileSpec, const ArchiveFormat fmt,
Archive *
OpenArchive(const char *FileSpec, const ArchiveFormat fmt)
{
-	ArchiveHandle *AH = _allocAH(FileSpec, fmt, 0, true, archModeRead, setupRestoreWorker);
+	ArchiveHandle *AH;
+	pg_compress_specification compress_spec;

Should this be initialized to {0} ?

@@ -969,6 +969,8 @@ NewRestoreOptions(void)
opts->format = archUnknown;
opts->cparams.promptPassword = TRI_DEFAULT;
opts->dumpSections = DUMP_UNSECTIONED;
+	opts->compress_spec.algorithm = PG_COMPRESSION_NONE;
+	opts->compress_spec.level = INT_MIN;

Why INT_MIN ?

@@ -1115,23 +1117,28 @@ PrintTOCSummary(Archive *AHX)
ArchiveHandle *AH = (ArchiveHandle *) AHX;
RestoreOptions *ropt = AH->public.ropt;
TocEntry   *te;
+	pg_compress_specification out_compress_spec;

Should have {0} ?
I suggest to write it like my 2020 patch for this, which says:
no_compression = {0};

/* Open stdout with no compression for AH output handle */
-	AH->gzOut = 0;
-	AH->OF = stdout;
+	out_compress_spec.algorithm = PG_COMPRESSION_NONE;
+	AH->OF = cfdopen(dup(fileno(stdout)), PG_BINARY_A, out_compress_spec);

Ideally this should check the success of dup().

@@ -3776,21 +3746,25 @@ ReadHead(ArchiveHandle *AH)
+	if (AH->compress_spec.level != INT_MIN)

Why is it testing the level and not the algorithm ?

--- a/src/bin/pg_dump/pg_backup_custom.c
+++ b/src/bin/pg_dump/pg_backup_custom.c
@@ -298,7 +298,7 @@ _StartData(ArchiveHandle *AH, TocEntry *te)
_WriteByte(AH, BLK_DATA);	/* Block type */
WriteInt(AH, te->dumpId);	/* For sanity check */

-	ctx->cs = AllocateCompressor(AH->compression, _CustomWriteFunc);
+	ctx->cs = AllocateCompressor(AH->compress_spec, _CustomWriteFunc);

Is it necessary to rename the data structure ?
If not, this file can remain unchanged.

--- a/src/bin/pg_dump/pg_backup_directory.c
+++ b/src/bin/pg_dump/pg_backup_directory.c
@@ -573,6 +574,7 @@ _CloseArchive(ArchiveHandle *AH)
if (AH->mode == archModeWrite)
{
cfp		   *tocFH;
+		pg_compress_specification compress_spec;

Should use {0} ?

@@ -639,12 +642,14 @@ static void
_StartBlobs(ArchiveHandle *AH, TocEntry *te)
{
lclContext *ctx = (lclContext *) AH->formatData;
+	pg_compress_specification compress_spec;

Same

+	/*
+	 * Custom and directory formats are compressed by default (zlib), others
+	 * not
+	 */
+	if (user_compression_defined == false)

Should be: !user_compression_defined

Your 0001+0002 patches (without 0003) fail to compile:

pg_backup_directory.c: In function ‘_ReadByte’:
pg_backup_directory.c:519:12: error: ‘CompressFileHandle’ {aka ‘struct CompressFileHandle’} has no member named ‘_IO_getc’
519 | return CFH->getc(CFH);
| ^~
pg_backup_directory.c:520:1: warning: control reaches end of non-void function [-Wreturn-type]
520 | }

--
Justin

On Tue, Jul 05, 2022 at 01:22:47PM +0000, gkokolatos@pm.me wrote:

I have updated for "some" of the comments. This is not an unwillingness to
incorporate those specific comments. Simply this patchset had started to divert
heavily already based on comments from Mr. Paquier who had already requested for
the APIs to be refactored to use function pointers. This is happening in 0002 of
the patchset. 0001 of the patchset is using the new compression.h under common.

This patchset should be considered a late draft, as commentary, documentation,
and some finer details are not yet finalized; because I am expecting the proposed
refactor to receive a wealth of comments. It would be helpful to understand if
the proposed direction is something worth to be worked upon, before moving to the
finer details.

I have read through the patch set, and I like a lot the separation you
are doing here with CompressFileHandle where a compression method has
to specify a full set of callbacks depending on the actions that need
to be taken. One advantage, as you patch shows, is that you reduce
the dependency of each code path depending on the compression method,
with #ifdefs and such located mostly into their own file structure, so
as adding a new compression method becomes really easier. These
callbacks are going to require much more documentation to describe
what anybody using them should expect from them, and perhaps they
could be renamed in a more generic way as the currect names come from
POSIX (say read_char(), read_string()?), even if this patch has just
inherited the names coming from pg_dump itself, but this can be tuned
over and over.

The split into three parts as of 0001 to plug into pg_dump the new
compression option set, 0002 to introduce the callbacks and 0003 to
add LZ4, building on the two first parts, makes sense to me. 0001 and
0002 could be done in a reversed order as they are mostly independent,
this order is fine as-is.

In short, I am fine with the proposed approach.

+#define K_VERS_1_15 MAKE_ARCHIVE_VERSION(1, 15, 0) /* add compressionMethod
+                                                    * in header */
Indeed, the dump format needs a version bump for this information.

+static bool
+parse_compression_option(const char *opt,
+                        pg_compress_specification *compress_spec)
This parsing logic in pg_dump.c looks a lot like what pg_receivewal.c
does with its parse_compress_options() where, for compatibility:
- If only a number is given:
-- Assume no compression if level is 0.
-- Assume gzip with given compression if level > 0.
- If a string is found, assume a full spec, with optionally a level.
So some consolidation could be done between both.

By the way, I can see that GZCLOSE(), etc. are still defined in
compress_io.h but they are not used.
--
Michael

This entry has been waiting on author input for a while (our current
threshold is roughly two weeks), so I've marked it Returned with
Feedback.

Once you think the patchset is ready for review again, you (or any
interested party) can resurrect the patch entry by visiting

https://commitfest.postgresql.org/38/3571/

and changing the status to "Needs Review", and then changing the
status again to "Move to next CF". (Don't forget the second step;
hopefully we will have streamlined this in the near future!)

Thanks,
--Jacob

Thank you for your work during commitfest.

The patch is still in development. Given vacation status, expect the next patches to be ready for the November commitfest.
For now it has moved to the September one. Further action will be taken then as needed.

Enjoy the rest of the summer!

Checking if you'll be able to submit new patches soon ?

On Wed, Nov 2, 2022 at 14:28, Justin Pryzby <pryzby@telsasoft.com> wrote:

Checking if you'll be able to submit new patches soon ?

Thank you for checking up. Expect new versions within this commitfest cycle.