Date: Wed, 30 Mar 2022 14:54:34 +0900
From: Michael Paquier <michael@paquier.xyz>
To: gkokolatos@pm.me
Cc: Justin Pryzby <pryzby@telsasoft.com>,
	Rachel Heaton <rachelmheaton@gmail.com>, Greg Stark <stark@mit.edu>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Add LZ4 compression in pg_dump
Message-ID: <YkPwmndklVKG4PE+@paquier.xyz>
References: 
 <faUNEOpts9vunEaLnmxmG-DldLSg_ql137OC3JYDmgrOMHm1RvvWY2IdBkv_CRxm5spCCb_OmKNk2T03TMm0fBEWveFF9wA1WizPuAgB7Ss=@protonmail.com>
 <20220304161039.GL27651@telsasoft.com>
 <CAM-w4HNr=THV3WQoeUPs5jefY8zcXtZJQ5pjWjUAsSpDtEeCaw@mail.gmail.com>
 <20220325132253.GD28503@telsasoft.com>
 <CADJcwiUbfBV0oNNpKvd2H4kP7m2U+7SmL-GAGO8Uv5H3HZUbAw@mail.gmail.com>
 <PSMV5cBDnG2Wd93aYAyTE9RxCPpO0K7ZZQezDkqxwrDTrnp0R8fokD3N8JcmxmuHqAI4cRp_vMmPSUu1igCtaZfk-rLwoSWuBoFiCcRl8WQ=@pm.me>
 <Yj6rXhLLW3h1vYt0@paquier.xyz>
 <20220326061441.GH28503@telsasoft.com>
 <YkK05K+0tjwmhLre@paquier.xyz>
 <yTrMijGhpyxYVVeJuJPM5NandUq_5Qq647cqNuUpVkXSfbYPDj0VLfRK72jB2HWoeDgRIYIbjG7YbBrUuzv8fRF9Y5vALIrM9BGIgQLh8vM=@pm.me>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="HScUv9d//ae60EaW"
Content-Disposition: inline
In-Reply-To: 
 <yTrMijGhpyxYVVeJuJPM5NandUq_5Qq647cqNuUpVkXSfbYPDj0VLfRK72jB2HWoeDgRIYIbjG7YbBrUuzv8fRF9Y5vALIrM9BGIgQLh8vM=@pm.me>
Archived-At: 
 <https://www.postgresql.org/message-id/YkPwmndklVKG4PE%2B%40paquier.xyz>
Precedence: bulk


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

On Tue, Mar 29, 2022 at 09:46:27AM +0000, gkokolatos@pm.me wrote:
> On Tuesday, March 29th, 2022 at 9:27 AM, Michael Paquier <michael@paquier=
=2Exyz> wrote:
>> On Sat, Mar 26, 2022 at 01:14:41AM -0500, Justin Pryzby wrote:
>> Wow. This stuff is old enough to vote (c3e18804), dead since its
>> introduction. There is indeed an argument for removing that, it is
>> not good to keep around that that has never been stressed and/or
>> used. Upon review, the cleanup done looks correct, as we have never
>> been able to generate .dat.gz files in for a dump in the tar format.
>=20
> Correct. My driving force behind it was to ease up the cleanup/refactoring
> work that follows, by eliminating the callers of the GZ*() macros.

Makes sense to me.

>> + command_fails_like(
>>
>> + [ 'pg_dump', '--compress', '1', '--format', 'tar' ],
>> This addition depending on HAVE_LIBZ is a good thing as a reminder of
>> any work that could be done in 0002. Now that's waiting for 20 years
>> so I would not hold my breath on this support. I think that this
>> could be just applied first, with 0002 on top of it, as a first
>> improvement.
>=20
> Excellent, thank you.

I have applied the test for --compress and --format=3Dtar, separating it
=66rom the rest.

While moving on with 0002, I have noticed the following in
_StartBlob():
    if (AH->compression !=3D 0)
        sfx =3D ".gz";
    else
        sfx =3D "";
=09
Shouldn't this bit also be simplified, adding a fatal() like the other
code paths, for safety?

>> + compress_cmd =3D> [
>> + $ENV{'GZIP_PROGRAM'},
>> + '-f',
>> [...]
>> + $ENV{'GZIP_PROGRAM'},
>> + '-k', '-d',
>> -f and -d are available everywhere I looked at, but is -k/--keep a
>> portable choice with a gzip command? I don't see this option in
>> OpenBSD, for one. So this test is going to cause problems on those
>> buildfarm machines, at least. Couldn't this part be replaced by a
>> simple --test to check that what has been compressed is in correct
>> shape? We know that this works, based on our recent experiences with
>> the other tests.
>=20
> I would argue that the simple '--test' will not do in this case, as the
> TAP tests do need a file named <test>.sql to compare the contents with.
> This file is generated either directly by pg_dump itself, or by running
> pg_restore on pg_dump's output. In the case of compression pg_dump will
> generate a <test>.sql.<compression program suffix> which can not be
> used in the comparison tests. So the intention of this block, is not to
> simply test for validity, but to also decompress pg_dump's output for it
> to be able to be used.

Ahh, I see, thanks.  I would add a comment about that in the area of
compression_gzip_plain_format.

+   my $supports_compression =3D check_pg_config("#define HAVE_LIBZ 1");

This part could be moved within the if block a couple of lines down.

+   my $compress_program =3D $ENV{GZIP_PROGRAM};

It seems to me that it is enough to rely on {compress_cmd}, hence
there should be no need for $compress_program, no?

It seems to me that we should have a description for compress_cmd at
the top of 002_pg_dump.pl (close to "Definition of the pg_dump runs to
make").  There is an order dependency with restore_cmd.

> I updated the patch to simply remove the '-k' flag.

Okay.
--
Michael

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

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

iQIyBAEBCAAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmJD8JoACgkQnvQgOdby
QH3/Qg/4kFg0aogyASLEEqrAMwEgfQzbWBhLD4UiD7OCAyUUW7yIsnZXdt+wJ/ad
0PTLkZfj8YPCEIdadISAgEnBuMa7tpY9evu6Knxv2mcH8YagITRCUGvYQqIxq1Sh
PEZjXGvfjvPKGnm82xumn9YrxqoIgTFXqynOxw+eUzlFRFBzpo98EvkXReSx9Kjq
G99ch5Q4LjaIRChMCe1waHbBCH2YA5NMPiMi9m4dKrrZJHlD7H/8DG9w7efFaLyU
Crc/Gta6wIcfqrVOkYlTJ8J00TzRWbbS/NrR/lYwe6e08rbUwTHGo0eAF9XWb6zi
/JugvCPBTenEJvufrp5F2bPMBIFlWzh3PEaAoEvWOEJqo6nx5vOZeasoVec4PUfq
PKgtJX6XNdVCsVuoQwzd8OyA6s1pzhTB5/ZmbLCoXZncWjajg23++C6tN56rA08n
do2kpmHV++tZUDsOjBcUpeM3qdju4N9pQAM3yGuvArPBjH21tUilc+4c0Gy8j1TA
3iGLIS5GTnqIAY/s7yzS0rT8cSbxSQr+lEVwFmxdLRVUwJcCMrCh1McroqHOwfMz
MLRw0FyriW6HYlqQmhOii9BQ8FacM6FIzAvIGcYGp1KFxm3CEYeAbka2VNfeTZ3T
6jEqsGTG1jQGa3vBsreyQ/4/YkYx9VNLwrcF1a4V2Le2lmmyyA==
=g424
-----END PGP SIGNATURE-----

--HScUv9d//ae60EaW--