Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 26 Jan 2023 14:49:27 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: gkokolatos@pm.me, Tomas Vondra <tomas.vondra@enterprisedb.com>,
	pgsql-hackers@lists.postgresql.org,
	Rachel Heaton <rachelmheaton@gmail.com>
Subject: Re: Add LZ4 compression in pg_dump
Message-ID: <Y9IUZzQsusY3VuyJ@paquier.xyz>
References: <6ab89d42-0345-b96e-02d4-68e171cdbe03@enterprisedb.com>
 <7zu6GYFi72jByVk2SEe_nd8r1DHhwA35vSntAE9sAfiMX9Lfk6bwimkwmu7GDgYaEvL27SE4qF1uK4_LHWQM4H-SXOwkMAI-GV8PKGmED_E=@pm.me>
 <d8cbc586-a8e3-1662-c158-9022d5c8de83@enterprisedb.com>
 <ce49a4f6-f7cc-28d7-3761-ddee7ef732af@enterprisedb.com>
 <MMLKPM28EzV5-NC4Jd3YdWCmPlUvbpbyqrugtklWTHZwF24g5x2IJ1J1wTICSESs8CaIRdj-56eDIqC4cTpahm55PmTVRik2D77X0ZKGeVo=@pm.me>
 <20230123180014.GX13860@telsasoft.com>
 <YiPrsNILgul-hbeyD6tfNaRUK5JZEhaZE36dLe1YJvKN_i0Tm2GuHd246MqcfNGFsjSY-rNo9n54MFCrRiRHRDkYyjgE4e29V-diAN2EcNU=@pm.me>
 <20230125014203.GL13860@telsasoft.com>
 <-myGeDiZ-ePDRyi18026Iu2HTTfmwfjWaJn4OWJ2F3oglZUfJTOQ7FARPW7gYxE4vK3b0HpwYoiHR45nfOWReRvvJZfCBfJzPP5iJyn4muk=@pm.me>
 <20230125180020.GF22427@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="kp1fBFhuYlnMzzFm"
Content-Disposition: inline
In-Reply-To: <20230125180020.GF22427@telsasoft.com>
Archived-At: <https://www.postgresql.org/message-id/Y9IUZzQsusY3VuyJ%40paquier.xyz>
Precedence: bulk


--kp1fBFhuYlnMzzFm
Content-Type: multipart/mixed; boundary="Hl0fs3YhWyK16J8C"
Content-Disposition: inline


--Hl0fs3YhWyK16J8C
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 25, 2023 at 12:00:20PM -0600, Justin Pryzby wrote:
> While looking at this, I realized that commit 5e73a6048 introduced a
> regression:
>=20
> @@ -3740,19 +3762,24 @@ ReadHead(ArchiveHandle *AH)
>=20
> -       if (AH->compression !=3D 0)
> -               pg_log_warning("archive is compressed, but this installat=
ion does not support compression -- no data will be available");
> +       if (AH->compression_spec.algorithm =3D=3D PG_COMPRESSION_GZIP)
> +               pg_fatal("archive is compressed, but this installation do=
es not support compression");
>=20
> Before, it was possible to restore non-data chunks of a dump file, even
> if the current build didn't support its compression.  But that's now
> impossible - and it makes the code we're discussing in RestoreArchive()
> unreachable.

Right.  The impacts the possibility of looking at the header data,
which is useful with pg_restore -l for example.  On a dump that's been
compressed, pg_restore <=3D 15 would always print the TOC entries with
or without compression support.  On HEAD, this code prevents the
header lookup.  All *nix or BSD platforms should have support for
zlib, I hope..  Still that could be an issue on Windows, and this
would prevent folks to check the contents of the dumps after saving it
on a WIN32 host, so let's undo that.

So, I have been testing the attached with four sets of binaries from
15/HEAD and with[out] zlib support, and this brings HEAD back to the
pre-15 state (header information able to show up, still failure when
attempting to restore the dump's data without zlib).

> I don't think we can currently test for that, since it requires creating =
a dump
> using a build --with compression and then trying to restore using a build
> --without compression.

Right, the location of the data is in the header, and I don't see how
you would be able to do that without two sets of binaries at hand, but
our tests run under the assumption that you have only one.  Well,
that's not entirely true as well, as you could create a TAP test like
pg_upgrade that relies on a environment variable pointing to a second
set of binaries.  That's not worth the complication involved, IMO.

> The coverage report disagrees with me, though...
> https://coverage.postgresql.org/src/bin/pg_dump/pg_backup_archiver.c.gcov=
=2Ehtml#3901

Isn't that one of the tests like compression_gzip_plain?

Thoughts?
--
Michael

--Hl0fs3YhWyK16J8C
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="dump-header-compress.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_back=
up_archiver.c
index ba5e6acbbb..cb4386f871 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3784,7 +3784,7 @@ ReadHead(ArchiveHandle *AH)
=20
 #ifndef HAVE_LIBZ
 	if (AH->compression_spec.algorithm =3D=3D PG_COMPRESSION_GZIP)
-		pg_fatal("archive is compressed, but this installation does not support =
compression");
+		pg_log_warning("archive is compressed, but this installation does not su=
pport compression -- no data will be available");
 #endif
=20
 	if (AH->version >=3D K_VERS_1_4)

--Hl0fs3YhWyK16J8C--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmPSFGcACgkQnvQgOdby
QH2sPg//Yyn3++dbGDPnCWYxi89GRduoKN4b4fnRPV87doqUTOovjwD6tlxf113c
1e2vdBBuQ3aA6xxU2knihxSS1tYFbqIs1L2f9depNTK8EXKTAotA2+l6xViRCLDV
LEbi1UU/vevKgdhArRmbo5mnBmpADW3UNT3aBAnAw/VjjKi/GJfHNwGxk/pzDz7y
pIL1sEDa2Pz87SxJvNeo9/iwcdpCWwkr8uY6WYRkOvtJpZLLhvVz8io5Oa6+iXHw
4oMRfpfCOvqFh11Xts0oi9zEZyjLp0Ui+yIvI5OtVIIlg6b133cA+OHaTbfNtSYz
9uZjxN1qw591O4k6BFf4CNhD7f5r+bKHKPpG2Mns6SXQvCbcZF6zqgLWmCurpiKO
PiYNmEbM1gkCUzZxEN32GOAImDn57+zU9sbmaGCBWp8eSI457JcSchlXwSgkUGBA
XK6zIEK624yEmF/wbZMFIEoXuwyUOpTvTLyMVf9E6nfloduagBEwNpxX2q2KWE6+
vMTwf1CK1ERylNwu7HHpJg0PJKca/XOsXuY/mfQAyE9KiM3f2TvmdmEk8Wv+So2k
O0WQ2KC8yz90DTMf75OYYKdntlSIlm9UFQe610KHD/2fkLyc8docobZPjPGPYWjy
+1h6TANnFCVCy3jFaPrl4OmFsFzCSFoD7dINBKiy803gxnD6bF8=
=OXPp
-----END PGP SIGNATURE-----

--kp1fBFhuYlnMzzFm--