Date: Fri, 7 Jan 2022 15:43:06 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Georgios Kokolatos <gkokolatos@pm.me>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>,
	Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>
Subject: Re: Refactoring of compression options in pg_basebackup
Message-ID: <Ydfg+r7SNZ+HSJst@paquier.xyz>
References: <Yb3GEgWwcu4wZDuA@paquier.xyz>
 <YuCGAJCBv6dd06z-v1gysn6F69MBWxspGLLGa8wJBaj8BrYs_rxJT3BJuh1U_LEDghumIN2N9ed-Gfve7K2dBeseCCOsypharQkXknnrxeo=@pm.me>
 <YdVQICWlyMA2NBSN@paquier.xyz>
 <pAYKvZybUu5ercp9Supt5f0OVFKEJF24WGzdmoBo9ReJtBKs4zFILwcn4J1_6f4XjHfrWxFZlPa83qyh57jmelXTIc3LsONWbqXJIuJyg1Y=@pm.me>
 <CA+TgmoYb4jnOU+-Xipf2V+twF9MR1P9bbLQqjiAmi3yc3r1tOA@mail.gmail.com>
 <YdZ4R7sNek759eZX@paquier.xyz>
 <CA+TgmoaUW0v5oxaDTu05n_r--iy6DJFhsxQ2Ldbtf20ZkHm1HA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Lt1BQECwolGOXlP4"
Content-Disposition: inline
In-Reply-To: <CA+TgmoaUW0v5oxaDTu05n_r--iy6DJFhsxQ2Ldbtf20ZkHm1HA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Ydfg%2Br7SNZ%2BHSJst%40paquier.xyz>
Precedence: bulk


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

On Thu, Jan 06, 2022 at 09:27:19AM -0500, Robert Haas wrote:
> Did you mean that -z would be a synonym for --compression-method=3Dgzip?
> It doesn't really make sense for -Z to be that, unless it's also
> setting the compression level.

Yes, I meant "-z", not "-Z", to be a synonym of
--compression-method=3Dgzip.  Sorry for the typo.

> My objection to --compress=3D$LEVEL is that the compression level seems
> like it ought to rightfully be subordinate to the choice of algorithm.
> In general, there's no reason why a compression algorithm has to offer
> a choice of compression levels at all, or why they have to be numbered
> 0 through 9. For example, lz4 on my system claims to offer compression
> levels from 1 through 12, plus a separate set of "fast" compression
> levels starting with 1 and going up to an unspecified number. And then
> it also has options to favor decompression speed, change the block
> size, and a few other parameters. We don't necessarily want to expose
> all of those options, but we should structure things so that we could
> if it became important. The way to do that is to make the compression
> algorithm the primary setting, and then anything else you can set for
> that compressor is somehow a subordinate setting.

For any compression method, that maps to an integer, so..  But I am
not going to fight hard on that.

> Put another way, we don't decide first that we want to compress with
> level 7, and then afterward decide whether that's gzip, lz4, or bzip2.
> We pick the compressor first, and then MAYBE think about changing the
> compression level.

Which is why things should be checked once all the options are
processed.  I'd recommend that you read the option patch a bit more,
that may help.

> Well what I was looking at was this:
>=20
> - printf(_("  -Z, --compress=3D0-9     compress tar output with given
> compression level\n"));
> + printf(_("  -Z, --compress=3D1-9     compress tar output with given
> compression level\n"));
> + printf(_("      --compression-method=3DMETHOD\n"
> + "                         method to compress data\n"));
>=20
> That seems to show that, post-patch, the argument to -Z would be a
> compression level, even if --compression-method were something other
> than gzip.

Yes, after the patch --compress would be a compression level.  And, if
attempting to use with --compression-method set to "none", or
potentially "lz4", it would just fail.  If not using this "gzip", the
compression level is switched to Z_DEFAULT_COMPRESSION.  That's this
area of the patch, FWIW:
+   /*
+    * Compression-related options.
+    */
+   switch (compression_method)

> It's possible that I haven't read something carefully enough, but to
> me, what I said seems to be a straightforward conclusion based on
> looking at the usage help in the patch. So if I came to the wrong
> conclusion, perhaps that usage help isn't reflecting the situation you
> intend to create, or not as clearly as it ought.

Perhaps the --help output could be clearer, then.  Do you have a
suggestion?

Bringing walmethods.c at the same page for the directory and the tar
methods was my primary goal here, and the tests are a bonus, so I've
applied this part for now, leaving pg_basebackup alone until we figure
out the layer of options we should use.  Perhaps it would be better to
revisit that stuff once the server-side compression has landed.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmHX4PoACgkQnvQgOdby
QH3xdQ//SZyvTrVdySUv2EAEDwXCyQIR726tPsL8xi/zIkFV1JIkP7PDDpDzcUcL
TyIGu5RD6z6I2k17mCMD5fpE3/B9l2CafBiD4i13SF2gpB2L4rcf/k77dY7HenAv
EUHTeIyT4D0S0h9Rz3d+Miw4YOGsjYWqTOstWBwGZJtN5LxHDx+64OTZj8iQJlyU
U+AYNwX01B65gG3D93U+/NC77IRaC28n8SGI21s6mBYjpVL+EKDQc4EjNiHxwCjC
2tWFHDuE7IUjVom+e/25+IIL4rXl5V+SyRBGD07CpTvPxxDswev7P2UwUtqVCoz7
mColKBTvfKaFwyaTt2ul6I4e/UvaoJJqWCuPMoj7O/fZWXlTZ5SL+0nhgvV787Qt
5Qkj9+eQt6Jog5n0TBuwq0ILLot9Ne8XXjtyjyxnIkkt1QRBTQqRX07riHFdI43A
s/uHoPhLmo8rndAjjDSFFyDC5UUlyhuDMKDO6GCEfiLys1AVUPTXYmGHf/JU4jo/
oCiGcrwlnqCW4ntfLYJZ2zhUSZvuf1OeTwCBfXb3vxatjNnEnz9l8hq1efizSwpl
C1teFh3Vl84y9elLT2FX21uiLJLJaYc4STu0SzHoihgdxU0IhFNGQjdKPLrxVmfU
AuJ0qt+sPumDJDBMRUi9Kba2JsTczdjYGqz/0mMQ9WoaJ40hGe4=
=9qCw
-----END PGP SIGNATURE-----

--Lt1BQECwolGOXlP4--