Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 5 Dec 2022 16:05:23 +0900
From: Michael Paquier <michael@paquier.xyz>
To: gkokolatos@pm.me
Cc: Justin Pryzby <pryzby@telsasoft.com>,
	pgsql-hackers@lists.postgresql.org,
	Rachel Heaton <rachelmheaton@gmail.com>
Subject: Re: Add LZ4 compression in pg_dump
Message-ID: <Y42YMz/nnNj3fxXG@paquier.xyz>
References: 
 <O4mutIrCES8ZhlXJiMvzsivT7ztAMja2lkdL1LJx6O5f22I2W8PBIeLKz7mDLwxHoibcnRAYJXm1pH4tyUNC4a8eDzLn22a6Pb1S74Niexg=@pm.me>
 <Y4WkZUGkic9jWb5I@paquier.xyz>
 <iKYD3Ryqsn88_UgYBiw3V803ISB5BJokmR2zsFtcedNVVeivIn_fqVv-oRRMqOpMiBFXsi72UYNMmuqMnEjjcQluTTpO-oxQWAVPeAlACnA=@pm.me>
 <Y4ao2viUjcO/c2Oc@paquier.xyz>
 <1j_ONBQqU3lQW7OqwbAFKbgcqjQosikcp7VPINqegLrHMCMGtFcXAuyjmW__VeviKyCbKTtucxFyPFUJ7sfqeOf4O90JEuUIe99Z5v7sdOg=@pm.me>
 <Y4gMAkNGIU1ShaoD@paquier.xyz>
 <jBiqIGoBbxyqLL2syvEDLm1W_TPukmBtAOkFyICkAssd8qupR7zjpA066Cj_syxQOVFsQORr3fc-g_WeATSXvdoybB-e7Xj26Y-C8U7rTKA=@pm.me>
 <Y4lbSoDPlVdbes1u@paquier.xyz>
 <T33dciWfZ3WaU10Mspm2_vsftrl0DI_cs4rQcrT_pLubaLq_HCRlW5QhEOT1cWtSRtdeTVXrL2r2BTYz3Fjz_3YNh6i77lwd5AxNxb5oanQ=@pm.me>
 <Y4q4SihPgXDVCL27@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="o9C5rQcnPLNgXvO4"
Content-Disposition: inline
In-Reply-To: <Y4q4SihPgXDVCL27@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/Y42YMz/nnNj3fxXG%40paquier.xyz>
Precedence: bulk


--o9C5rQcnPLNgXvO4
Content-Type: multipart/mixed; boundary="itEnfeIqgZwIC90d"
Content-Disposition: inline


--itEnfeIqgZwIC90d
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sat, Dec 03, 2022 at 11:45:30AM +0900, Michael Paquier wrote:
> While this is correct in checking that the contents are compressed
> under --with-zlib, this also removes the coverage where we make sure
> that this command is able to complete under --without-zlib without
> compressing any of the table data files.  Hence my point from
> upthread: this test had better not use compile_option, but change
> glob_pattern depending on if the build uses zlib or not.

In short, I mean something like the attached.  I have named the flag
content_patterns, and switched it to an array so as we can check that
toc.dat is always uncompression and that the other data files are
always uncompressed.

> In order to check this behavior with defaults_custom_format, perhaps
> we could just remove the -Z6 from it or add an extra command for its
> default behavior?

This is slightly more complicated as there is just one file generated
for the compression and non-compression cases, so I have let that as
it is now.
--
Michael

--itEnfeIqgZwIC90d
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v15-0001-Provide-coverage-for-pg_dump-default-compression.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 5c583358caed5598fec9abea6750ff7fbd98d269 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Mon, 5 Dec 2022 16:04:57 +0900
Subject: [PATCH v15] Provide coverage for pg_dump default compression for d=
ir
 format

The restore program will succeed regardless of whether the dumped output was
compressed or not. This commit implements a portable way to check the conte=
nts
of the directory via perl's build in filename expansion.
---
 src/bin/pg_dump/t/002_pg_dump.pl | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_du=
mp.pl
index 709db0986d..9796d2667f 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -36,6 +36,9 @@ my $tempdir =3D PostgreSQL::Test::Utils::tempdir;
 # to test pg_restore's ability to parse manually compressed files
 # that otherwise pg_dump does not compress on its own (e.g. *.toc).
 #
+# content_patterns is an optional array consisting of strings compilable
+# with glob() to check the files generated after a dump.
+#
 # 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
@@ -46,6 +49,10 @@ my $tempdir =3D PostgreSQL::Test::Utils::tempdir;
 # database and then pg_dump *that* database (or something along
 # those lines) to validate that part of the process.
=20
+my $supports_icu  =3D ($ENV{with_icu} eq 'yes');
+my $supports_lz4  =3D check_pg_config("#define USE_LZ4 1");
+my $supports_gzip =3D check_pg_config("#define HAVE_LIBZ 1");
+
 my %pgdump_runs =3D (
 	binary_upgrade =3D> {
 		dump_cmd =3D> [
@@ -213,6 +220,9 @@ my %pgdump_runs =3D (
 	},
=20
 	# Do not use --no-sync to give test coverage for data sync.
+	# By default, the directory format compresses its contents
+	# when the code is compiled with gzip support, and lets things
+	# uncompressed when not compiled with it.
 	defaults_dir_format =3D> {
 		test_key =3D> 'defaults',
 		dump_cmd =3D> [
@@ -224,6 +234,11 @@ my %pgdump_runs =3D (
 			"--file=3D$tempdir/defaults_dir_format.sql",
 			"$tempdir/defaults_dir_format",
 		],
+		content_patterns =3D> ["$tempdir/defaults_dir_format/toc.dat",
+				     $supports_gzip ?
+				     "$tempdir/defaults_dir_format/*.dat.gz" :
+				     "$tempdir/defaults_dir_format/*.dat",
+		],
 	},
=20
 	# Do not use --no-sync to give test coverage for data sync.
@@ -3920,10 +3935,6 @@ if ($collation_check_stderr !~ /ERROR: /)
 	$collation_support =3D 1;
 }
=20
-my $supports_icu  =3D ($ENV{with_icu} eq 'yes');
-my $supports_lz4  =3D check_pg_config("#define USE_LZ4 1");
-my $supports_gzip =3D check_pg_config("#define HAVE_LIBZ 1");
-
 # ICU doesn't work with some encodings
 my $encoding =3D $node->safe_psql('postgres', 'show server_encoding');
 $supports_icu =3D 0 if $encoding eq 'SQL_ASCII';
@@ -4153,6 +4164,16 @@ foreach my $run (sort keys %pgdump_runs)
 		command_ok(\@full_compress_cmd, "$run: compression commands");
 	}
=20
+	if ($pgdump_runs{$run}->{content_patterns})
+	{
+		my $content_patterns =3D $pgdump_runs{$run}->{content_patterns};
+		foreach my $content_pattern (@{$content_patterns})
+		{
+			my @glob_output =3D glob($content_pattern);
+			is(scalar(@glob_output) > 0, 1, "$run: content check for $content_patte=
rn");
+		}
+	}
+
 	if ($pgdump_runs{$run}->{restore_cmd})
 	{
 		$node->command_ok(\@{ $pgdump_runs{$run}->{restore_cmd} },
--=20
2.38.1


--itEnfeIqgZwIC90d--

--o9C5rQcnPLNgXvO4
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmONmDMACgkQnvQgOdby
QH0U3RAAmWgUIR8ILfYg9Ul19T2CVRqRjWFbOr7BEsSH70hlIoTz0jJ8fb2VC15Q
W3MJavgh2rd9o4OuO3eZ/SW0moqr0KjV/9Roj5sTxJpfCdVNn8DDM60gJvmJOndZ
cSeCeGvQ8TZjZwrkLflUFbparEvHZ+0PnxpQ0GXGlsTXdOgT8vHNLlTW6+8Krpiu
I7c5kyI07vJhMtc5hqHxyvj+eGAeiM073n3bXjvVosjl1z3XrOflggUUcTTu9pGJ
5Vza/T6B3aCPuNtBWm6NxMytWkq4iuEDJf8drQYK/IhvF3QDDxpunJBPqmKOn8z0
1k41Udkh6GflWEw1+jltNLnfaJAt0v5S+1WA77amLTPA7mVPn2LmRp31sKPCRgG9
kxU4zB9uxJN5U4IBO4cfAvcmDh7KTq5epLXIUkbF0+MHcsF9N8J7jDL0rpyMdnNh
1O+f24e93G6xMiql0p5Rxq6pkF9osEMZeeKrZGTCCKrfzVwwL6rf7rugr6SjRIUj
+/LQFaS5IcyM0gUZ/JyowQ91xPhKtfcM24LLH/CjVIGcUJ1jAbLo/kgjGZaufowf
BhNuRLHqSIWtiMQ1LifgdDPaNLmILAqlRoz0cPDGazxUhLW3I+NyRa+Mzu4fu0iU
9AxBQyFu8T64Ze7M9yUgozkG0B9H7uQYdFTcr6a+W3b6ADL4nL0=
=frAf
-----END PGP SIGNATURE-----

--o9C5rQcnPLNgXvO4--