I have some fixes (attached) and questions while polishing the patch for zstd compression. The fixes are small and could be integrated with the patch for zstd, but could be applied independently.
- I'm unclear about get_error_func(). That's called in three places from pg_backup_directory.c, after failures from write_func(), to supply an compression-specific error message to pg_fatal(). But it's not being used outside of directory format, nor for errors for other function pointers, or even for all errors in write_func(). Is there some reason why each compression method's write_func() shouldn't call pg_fatal() directly, with its compression-specific message ? - I still think supports_compression() should be renamed, or made into a static function in the necessary file. The main reason is that it's more clear what it indicates - whether compression is "implemented by pgdump" and not whether compression is "supported by this postgres build". It also seems possible that we'd want to add a function called something like supports_compression(), indicating whether the algorithm is supported by the current build. It'd be better if pgdump didn't subjugate that name. - Finally, the "Nothing to do in the default case" comment comes from Michael's commit 5e73a6048: + /* + * Custom and directory formats are compressed by default with gzip when + * available, not the others. + */ + if ((archiveFormat == archCustom || archiveFormat == archDirectory) && + !user_compression_defined) { #ifdef HAVE_LIBZ - if (archiveFormat == archCustom || archiveFormat == archDirectory) - compressLevel = Z_DEFAULT_COMPRESSION; - else + parse_compress_specification(PG_COMPRESSION_GZIP, NULL, + &compression_spec); +#else + /* Nothing to do in the default case */ #endif - compressLevel = 0; } As the comment says: for -Fc and -Fd, the compression is set to zlib, if enabled, and when not otherwise specified by the user. Before 5e73a6048, this set compressLevel=0 for -Fp and -Ft, *and* when zlib was unavailable. But I'm not sure why there's now an empty "#else". I also don't know what "the default case" refers to. Maybe the best thing here is to move the preprocessor #if, since it's no longer in the middle of a runtime conditional: #ifdef HAVE_LIBZ + if ((archiveFormat == archCustom || archiveFormat == archDirectory) && + !user_compression_defined) + parse_compress_specification(PG_COMPRESSION_GZIP, NULL, + &compression_spec); #endif ...but that elicits a warning about "variable set but not used"... -- Justin
>From e31901414a8509317297972d1033c2e08629d903 Mon Sep 17 00:00:00 2001 From: Justin Pryzby <pryz...@telsasoft.com> Date: Fri, 24 Feb 2023 14:07:09 -0600 Subject: [PATCH] f!fixes for LZ4 --- doc/src/sgml/ref/pg_dump.sgml | 4 ++-- src/bin/pg_dump/compress_io.c | 2 +- src/bin/pg_dump/compress_io.h | 4 ++-- src/bin/pg_dump/compress_lz4.c | 4 ++-- src/bin/pg_dump/pg_backup_archiver.c | 4 ++-- src/bin/pg_dump/pg_dump.c | 2 -- src/bin/pg_dump/t/002_pg_dump.pl | 6 +++--- 7 files changed, 12 insertions(+), 14 deletions(-) diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml index 49d218905fb..6fbe49f7ede 100644 --- a/doc/src/sgml/ref/pg_dump.sgml +++ b/doc/src/sgml/ref/pg_dump.sgml @@ -331,7 +331,7 @@ PostgreSQL documentation can read. A directory format archive can be manipulated with standard Unix tools; for example, files in an uncompressed archive can be compressed with the <application>gzip</application> or - <application>lz4</application>tool. + <application>lz4</application> tools. This format is compressed by default using <literal>gzip</literal> and also supports parallel dumps. </para> @@ -655,7 +655,7 @@ PostgreSQL documentation <para> Specify the compression method and/or the compression level to use. The compression method can be set to <literal>gzip</literal> or - <literal>lz4</literal> or <literal>none</literal> for no compression. + <literal>lz4</literal>, or <literal>none</literal> for no compression. A compression detail string can optionally be specified. If the detail string is an integer, it specifies the compression level. Otherwise, it should be a comma-separated list of items, each of the diff --git a/src/bin/pg_dump/compress_io.c b/src/bin/pg_dump/compress_io.c index ce06f1eac9c..9239dbb2755 100644 --- a/src/bin/pg_dump/compress_io.c +++ b/src/bin/pg_dump/compress_io.c @@ -83,7 +83,7 @@ * used by the caller in an error message. */ char * -supports_compression(const pg_compress_specification compression_spec) +pgdump_supports_compression(const pg_compress_specification compression_spec) { const pg_compress_algorithm algorithm = compression_spec.algorithm; bool supported = false; diff --git a/src/bin/pg_dump/compress_io.h b/src/bin/pg_dump/compress_io.h index bbde2693915..46815fa2ebe 100644 --- a/src/bin/pg_dump/compress_io.h +++ b/src/bin/pg_dump/compress_io.h @@ -21,7 +21,7 @@ #define ZLIB_OUT_SIZE 4096 #define ZLIB_IN_SIZE 4096 -extern char *supports_compression(const pg_compress_specification compression_spec); +extern char *pgdump_supports_compression(const pg_compress_specification compression_spec); /* * Prototype for callback function used in writeData() @@ -172,7 +172,7 @@ struct CompressFileHandle extern CompressFileHandle *InitCompressFileHandle(const pg_compress_specification compression_spec); /* - * Initialize a compress file stream. Deffer the compression algorithm + * Initialize a compress file stream. Infer the compression algorithm * from 'path', either by examining its suffix or by appending the supported * suffixes in 'path'. */ diff --git a/src/bin/pg_dump/compress_lz4.c b/src/bin/pg_dump/compress_lz4.c index fe1014e6e77..63e794cdc68 100644 --- a/src/bin/pg_dump/compress_lz4.c +++ b/src/bin/pg_dump/compress_lz4.c @@ -161,8 +161,8 @@ typedef struct LZ4File } LZ4File; /* - * LZ4 equivalent to feof() or gzeof(). The end of file is reached if there - * is no decompressed output in the overflow buffer and the end of the file + * LZ4 equivalent to feof() or gzeof(). Return true iff there is no + * decompressed output in the overflow buffer and the end of the backing file * is reached. */ static int diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index 61ebb8fe85d..2063d6f239d 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -388,7 +388,7 @@ RestoreArchive(Archive *AHX) { if (te->hadDumper && (te->reqs & REQ_DATA) != 0) { - char *errmsg = supports_compression(AH->compression_spec); + char *errmsg = pgdump_supports_compression(AH->compression_spec); if (errmsg) pg_fatal("cannot restore from compressed archive (%s)", errmsg); @@ -3745,7 +3745,7 @@ ReadHead(ArchiveHandle *AH) else AH->compression_spec.algorithm = PG_COMPRESSION_GZIP; - errmsg = supports_compression(AH->compression_spec); + errmsg = pgdump_supports_compression(AH->compression_spec); if (errmsg) { pg_log_warning("archive is compressed, but this installation does not support compression (%s) -- no data will be available", diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 24ba936332d..ce2242195f3 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -733,8 +733,6 @@ main(int argc, char **argv) #ifdef HAVE_LIBZ parse_compress_specification(PG_COMPRESSION_GZIP, NULL, &compression_spec); -#else - /* Nothing to do in the default case */ #endif } diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl index 72b19ee6cde..ad7bc5c194b 100644 --- a/src/bin/pg_dump/t/002_pg_dump.pl +++ b/src/bin/pg_dump/t/002_pg_dump.pl @@ -4248,10 +4248,10 @@ foreach my $run (sort keys %pgdump_runs) my $test_key = $run; my $run_db = 'postgres'; - # Skip command-level tests for gzip if there is no support for it. + # Skip command-level tests for gzip/lz4 if they're not supported. if ($pgdump_runs{$run}->{compile_option} && - ($pgdump_runs{$run}->{compile_option} eq 'gzip' && !$supports_gzip) || - ($pgdump_runs{$run}->{compile_option} eq 'lz4' && !$supports_lz4)) + (($pgdump_runs{$run}->{compile_option} eq 'gzip' && !$supports_gzip) || + ($pgdump_runs{$run}->{compile_option} eq 'lz4' && !$supports_lz4))) { note "$run: skipped due to no $pgdump_runs{$run}->{compile_option} support"; next; -- 2.34.1