On Fri, Apr 01, 2022 at 03:06:40PM +0000, gkokola...@pm.me wrote:
> I understand the itch. Indeed when LZ4 is added as compression method, this
> block changes slightly. I went with the minimum amount changed. Please find
> in 0001 of the attached this variable renamed as $gzip_program_exist. I
> thought
> that as prefix it will match better the already used $ENV{GZIP_PROGRAM}.Hmm. I have spent some time on that, and upon review I really think that we should skip the tests marked as dedicated to the gzip compression entirely if the build is not compiled with this option, rather than letting the code run a dump for nothing in some cases, relying on the default to uncompress the contents in others. In the latter case, it happens that we have already some checks like defaults_custom_format, but you already mentioned that. We should also skip the later parts of the tests if the compression program does not exist as we rely on it, but only if the command does not exist. This will count for LZ4. > I can see the overlap case. Yet, I understand the test_key as serving > different > purpose, as it is a key of %tests and %full_runs. I do not expect the database > content of the generated dump to change based on which compression method is > used. Contrary to the current LZ4 tests in pg_dump, what we have here is a check for a command-level run and not a data-level check. So what's introduced is a new concept, and we need a new way to control if the tests should be entirely skipped or not, particularly if we finish by not using test_key to make the difference. Perhaps the best way to address that is to have a new keyword in the $runs structure. The attached defines a new compile_option, that can be completed later for new compression methods introduced in the tests. So the idea is to mark all the tests related to compression with the same test_key, and the tests can be skipped depending on what compile_option requires. > In the attached version, I propose that the compression_cmd is converted into > a hash. It contains two keys, the program and the arguments. Maybe it is > easier > to read than before or than simply grabbing the first element of the array. Splitting the program and its arguments makes sense. At the end I am finishing with the attached. I also saw an overlap with the addition of --jobs for the directory format vs not using the option, so I have removed the case where --jobs was not used in the directory format. -- Michael
From b705f77862c9e41a149ce078df638032903bce3d Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 5 Apr 2022 10:30:06 +0900
Subject: [PATCH v6] Extend compression coverage for pg_dump, pg_restore
---
src/bin/pg_dump/Makefile | 2 +
src/bin/pg_dump/t/002_pg_dump.pl | 93 +++++++++++++++++++++++++++++++-
2 files changed, 93 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_dump/Makefile b/src/bin/pg_dump/Makefile
index 302f7e02d6..2f524b09bf 100644
--- a/src/bin/pg_dump/Makefile
+++ b/src/bin/pg_dump/Makefile
@@ -16,6 +16,8 @@ subdir = src/bin/pg_dump
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+export GZIP_PROGRAM=$(GZIP)
+
override CPPFLAGS := -I$(libpq_srcdir) $(CPPFLAGS)
LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index af5d6fa5a3..dda3649373 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -20,12 +20,22 @@ my $tempdir = PostgreSQL::Test::Utils::tempdir;
# test_key indicates that a given run should simply use the same
# set of like/unlike tests as another run, and which run that is.
#
+# compile_option indicates if the commands run depend on a compilation
+# option, if any. This controls if tests are skipped if a dependency
+# is not satisfied.
+#
# dump_cmd is the pg_dump command to run, which is an array of
# the full command and arguments to run. Note that this is run
# using $node->command_ok(), so the port does not need to be
# specified and is pulled from $PGPORT, which is set by the
# PostgreSQL::Test::Cluster system.
#
+# compress_cmd is the utility command for (de)compression, if any.
+# Note that this should generally be used on pg_dump's output
+# either to generate a text file to run the through the tests, or
+# to test pg_restore's ability to parse manually compressed files
+# that otherwise pg_dump does not compress on its own (e.g. *.toc).
+#
# restore_cmd is the pg_restore command to run, if any. Note
# that this should generally be used when the pg_dump goes to
# a non-text file and that the restore can then be used to
@@ -54,6 +64,58 @@ my %pgdump_runs = (
"$tempdir/binary_upgrade.dump",
],
},
+
+ # Do not use --no-sync to give test coverage for data sync.
+ compression_gzip_custom => {
+ test_key => 'compression',
+ compile_option => 'gzip',
+ dump_cmd => [
+ 'pg_dump', '--format=custom',
+ '--compress=1', "--file=$tempdir/compression_gzip_custom.dump",
+ 'postgres',
+ ],
+ restore_cmd => [
+ 'pg_restore',
+ "--file=$tempdir/compression_gzip_custom.sql",
+ "$tempdir/compression_gzip_custom.dump",
+ ],
+ },
+
+ # Do not use --no-sync to give test coverage for data sync.
+ compression_gzip_dir => {
+ test_key => 'compression',
+ compile_option => 'gzip',
+ dump_cmd => [
+ 'pg_dump', '--jobs=2',
+ '--format=directory', '--compress=1',
+ "--file=$tempdir/compression_gzip_dir", 'postgres',
+ ],
+ # Give coverage for manually compressed blob.toc files during
+ # restore.
+ compress_cmd => {
+ program => $ENV{'GZIP_PROGRAM'},
+ args => [ '-f', "$tempdir/compression_gzip_dir/blobs.toc", ],
+ },
+ restore_cmd => [
+ 'pg_restore', '--jobs=2',
+ "--file=$tempdir/compression_gzip_dir.sql",
+ "$tempdir/compression_gzip_dir",
+ ],
+ },
+
+ compression_gzip_plain => {
+ test_key => 'compression',
+ compile_option => 'gzip',
+ dump_cmd => [
+ 'pg_dump', '--format=plain', '-Z1',
+ "--file=$tempdir/compression_gzip_plain.sql.gz", 'postgres',
+ ],
+ # Decompress the generated file to run through the tests.
+ compress_cmd => {
+ program => $ENV{'GZIP_PROGRAM'},
+ args => [ '-d', "$tempdir/compression_gzip_plain.sql.gz", ],
+ },
+ },
clean => {
dump_cmd => [
'pg_dump',
@@ -424,6 +486,7 @@ my %full_runs = (
binary_upgrade => 1,
clean => 1,
clean_if_exists => 1,
+ compression => 1,
createdb => 1,
defaults => 1,
exclude_dump_test_schema => 1,
@@ -3098,6 +3161,7 @@ my %tests = (
binary_upgrade => 1,
clean => 1,
clean_if_exists => 1,
+ compression => 1,
createdb => 1,
defaults => 1,
exclude_test_table => 1,
@@ -3171,6 +3235,7 @@ my %tests = (
binary_upgrade => 1,
clean => 1,
clean_if_exists => 1,
+ compression => 1,
createdb => 1,
defaults => 1,
exclude_dump_test_schema => 1,
@@ -3833,8 +3898,9 @@ if ($collation_check_stderr !~ /ERROR: /)
$collation_support = 1;
}
-# Determine whether build supports LZ4.
-my $supports_lz4 = check_pg_config("#define USE_LZ4 1");
+# Determine whether build supports LZ4 and gzip.
+my $supports_lz4 = check_pg_config("#define USE_LZ4 1");
+my $supports_gzip = check_pg_config("#define HAVE_LIBZ 1");
# Create additional databases for mutations of schema public
$node->psql('postgres', 'create database regress_pg_dump_test;');
@@ -3947,9 +4013,32 @@ 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.
+ if ( defined($pgdump_runs{$run}->{compile_option})
+ && $pgdump_runs{$run}->{compile_option} eq 'gzip'
+ && !$supports_gzip)
+ {
+ note "$run: skipped due to no gzip support";
+ next;
+ }
+
$node->command_ok(\@{ $pgdump_runs{$run}->{dump_cmd} },
"$run: pg_dump runs");
+ if ($pgdump_runs{$run}->{compress_cmd})
+ {
+ my ($compress_cmd) = $pgdump_runs{$run}->{compress_cmd};
+ my $compress_program = $compress_cmd->{program};
+
+ # Skip the rest of the test if the compression program is
+ # not defined.
+ next if (!defined($compress_program) || $compress_program eq '');
+
+ my @full_compress_cmd =
+ ($compress_cmd->{program}, @{ $compress_cmd->{args} });
+ command_ok(\@full_compress_cmd, "$run: compression commands");
+ }
+
if ($pgdump_runs{$run}->{restore_cmd})
{
$node->command_ok(\@{ $pgdump_runs{$run}->{restore_cmd} },
--
2.35.1
signature.asc
Description: PGP signature
