Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 2 Dec 2022 10:56:26 +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: <Y4lbSoDPlVdbes1u@paquier.xyz>
References: 
 <-pU80vtiyXMfSSpn4SWOR97t71MVmDOyHaJtvGsdEfroFsFUDdc-UaqD-8LMPMw2sy-7B2_IVnlrATfVvXwtv8n3kexLePZ7iWgiOnE4jRI=@pm.me>
 <dWoK9v8RxFX0rEqRXwHPIAZ1JqOjgUJKJEQYKgXo180_Q1YJBN5GDUMYJyfYv_C8Vgs23ddTW0I2htMj2CFBPM1Rd-DFgyXXsNE5kwmyk3Q=@pm.me>
 <Y3ypKVCOo1VYP9nz@paquier.xyz>
 <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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="qGhWo5HV+sq+yPFB"
Content-Disposition: inline
In-Reply-To: 
 <jBiqIGoBbxyqLL2syvEDLm1W_TPukmBtAOkFyICkAssd8qupR7zjpA066Cj_syxQOVFsQORr3fc-g_WeATSXvdoybB-e7Xj26Y-C8U7rTKA=@pm.me>
Archived-At: 
 <https://www.postgresql.org/message-id/Y4lbSoDPlVdbes1u%40paquier.xyz>
Precedence: bulk


--qGhWo5HV+sq+yPFB
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Dec 01, 2022 at 02:58:35PM +0000, gkokolatos@pm.me wrote:
> Nuking the warning from orbit and changing the behaviour around disabling
> the requested compression when the libraries are not present, should not
> mean that we need to change the behaviour of default values for different
> formats. Please find v13 attached which reinstates it.

Gah, thanks!  And this default behavior is documented as dependent on
the compilation as well.

> Which in itself it got me looking and wondering why the tests succeeded.
> The only existing test covering that path is `defaults_dir_format` in
> `002_pg_dump.pl`. However as the test is currently written it does not
> check whether the output was compressed. The restore command would succeed
> in either case. A simple `gzip -t -r` against the directory will not
> suffice to test it, because there exist files which are never compressed
> in this format (.toc). A little bit more involved test case would need
> to be written, yet before I embark to this journey, I would like to know
> if you would agree to reinstate the defaults for those formats.

On top of my mind, I briefly recall that -r is not that portable.  And
the toc format makes the files generated non-deterministic as these
use OIDs..

[.. thinks ..]

We are going to need a new thing here, as compress_cmd cannot be
directly used.  What if we used only an array of glob()-able elements?
Let's say "expected_contents" that could include a "dir_path/*.gz"
conditional on $supports_gzip?  glob() can only be calculated when the
test is run as the file names cannot be known beforehand :/

>> As per the patch, it is true that we do not need to bump the format of
>> the dump archives, as we can still store only the compression level
>> and guess the method from it. I have added some notes about that in
>> ReadHead and WriteHead to not forget.
>=20
> Agreed. A minor suggestion if you may.
>=20
>  #ifndef HAVE_LIBZ
> -       if (AH->compression !=3D 0)
> +       if (AH->compression_spec.algorithm =3D=3D PG_COMPRESSION_GZIP)
>                 pg_log_warning("archive is compressed, but this installat=
ion does not support compression -- no data will be available");
>  #endif
>=20
> It would seem a more consistent to error out in this case. We do error
> in all other cases where the compression is not available.

Makes sense.

I have gone through the patch again, and applied it.  Thanks!
--
Michael

--qGhWo5HV+sq+yPFB
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmOJW0oACgkQnvQgOdby
QH0huQ//dwUhr8C35LCzZdD9VNhdXPk7qHod/J5qDxjoZO865lW9mPp4MlqrXn80
GP3PqJSffc+xS5TezJN8trlEirUbIi1O+iYLd8lDy4iRTgyFqwZmvt3qpiBtM8M6
Jf0C2ZzRJQda0IJLzMQv6+apW41diPp5oSDVttYmizRtJ30YBNE4GYDPQw+g0rYc
91uOpAVougR1yIp116HQkO0aWVTwn/5rab2vI89rieiMOSyfqjRrAceSTBvSCHuU
R/nCoGZkTDC9uE4JFA24y4/SJYXtIm/cj34yNvLtDKrSa7ZmJzys5MQMvfLFdCKr
tLeGyKaCbCN7xwFoE28O5BOT2C+xMzpkcw3soQYYzGeDtC1UqN3I33OSFa3ZTotG
/04XVN8wstUiMT/17P6eZZnTHNlLzwCh935cImB7MvnQhzOcI0OZIamjnPsPjZJu
go4SWV/51ABiS2Q37KQ7hT1OCNjphk/iMcpjJhqhNXreWDBEQ8xtr9IzSRkCm3Ky
FtweIXReFpvO7tZLlM0bb0OPfDLGAJZ3AgZGZoa33hOKpCwjHiRHkExrunA1mNsp
yin35ZbX0pgVvTARF+cQCox1wiRuIKtUs8gBQtLKzycCfE5aUrt/Mad+PHj3UaXS
y5FuKgQYBBxXWqqKWVGuO++zg/XD/ASnkUmZDRNqYq5RyVHz1C8=
=LR82
-----END PGP SIGNATURE-----

--qGhWo5HV+sq+yPFB--