Date: Wed, 13 Apr 2022 14:25:02 +0900
From: Michael Paquier <michael@paquier.xyz>
To: gkokolatos@pm.me
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>,
	Robert Haas <robertmhaas@gmail.com>
Subject: Re: Fixes for compression options of pg_receivewal and refactoring
 of backup_compression.{c,h}
Message-ID: <YlZernJnRsqntRGC@paquier.xyz>
References: <YlPQGNAAa04raObK@paquier.xyz>
 <Pw7ltgmMRlWiJ8w3EW_1Paoeaf1QIJtV-c-U9UgF8f12w3wxpFbqwp97hr3OchnlOFetTA50zYcMQ3coWDutqP3a1qsG2SXOKipKY3lNqBE=@pm.me>
 <YlVE6E687KDOABoN@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="tPP7hahSjzZLCtxv"
Content-Disposition: inline
In-Reply-To: <YlVE6E687KDOABoN@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/YlZernJnRsqntRGC%40paquier.xyz>
Precedence: bulk


--tPP7hahSjzZLCtxv
Content-Type: multipart/mixed; boundary="RL5IJ4sIEOiDNQDR"
Content-Disposition: inline


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

On Tue, Apr 12, 2022 at 06:22:48PM +0900, Michael Paquier wrote:
> On Mon, Apr 11, 2022 at 12:46:02PM +0000, gkokolatos@pm.me wrote:
>> It looks good. If you choose to discard the comment regarding the use of
>> 'method' over 'algorithm' from above, can you please use the full word i=
n the
>> variable, e.g. 'wal_compress_algorithm' instead of 'wal_compress_algo'. =
I can
>> not really explain it, the later reads a bit rude. Then again that may b=
e just
>> me.
>=20
> Thanks.  I have been able to do an extra pass on 0001 and 0002, fixing
> those naming inconsistencies with "algo" vs "algorithm" that you and
> Robert have reported, and applied them.  For 0003, I'll look at it
> later.  Attached is a rebase with improvements about the variable
> names.

This has been done with the proper renames.  With that in place, I see
no reason now to not be able to set the compression level as it is
possible to pass it down with the options available.  This requires
only a couple of lines, as of the attached.  LZ4 has a dummy structure
called LZ4F_INIT_PREFERENCES to initialize LZ4F_preferences_t, that
holds the compression level before passing it down to
LZ4F_compressBegin(), but that's available only in v1.8.3.  Using it
risks lowering down the minimal version of LZ4 we are able to use now,
but replacing that with a set of memset()s is also a way to set up
things as per its documentation.

Thoughts?
--
Michael

--RL5IJ4sIEOiDNQDR
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="receivewal-level.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/wal=
methods.c
index d5bcc208a9..1b3cc5c4f7 100644
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -165,6 +165,7 @@ dir_open_for_write(const char *pathname, const char *te=
mp_suffix, size_t pad_to_
 	{
 		size_t		ctx_out;
 		size_t		header_size;
+		LZ4F_preferences_t prefs;
=20
 		ctx_out =3D LZ4F_createCompressionContext(&ctx, LZ4F_VERSION);
 		if (LZ4F_isError(ctx_out))
@@ -177,8 +178,13 @@ dir_open_for_write(const char *pathname, const char *t=
emp_suffix, size_t pad_to_
 		lz4bufsize =3D LZ4F_compressBound(LZ4_IN_SIZE, NULL);
 		lz4buf =3D pg_malloc0(lz4bufsize);
=20
+		/* assign the compression level, default is 0 */
+		memset(&prefs, 0, sizeof(prefs));
+		memset(&prefs.frameInfo, 0, sizeof(prefs.frameInfo));
+		prefs.compressionLevel =3D dir_data->compression_level;
+
 		/* add the header */
-		header_size =3D LZ4F_compressBegin(ctx, lz4buf, lz4bufsize, NULL);
+		header_size =3D LZ4F_compressBegin(ctx, lz4buf, lz4bufsize, &prefs);
 		if (LZ4F_isError(header_size))
 		{
 			dir_data->lasterrstring =3D LZ4F_getErrorName(header_size);

--RL5IJ4sIEOiDNQDR--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmJWXq4ACgkQnvQgOdby
QH0ELw//f7Nli4Fx1U4L6lDdlhCVEPlGNn61ILJNKAx/5saPJ+tG+WX7ESo8tKvv
apLypuUTrt1i6lzktp5pOmoGWqkApphNBq7ROEpI+c5kCfTZD25tVvsPqImKE4UH
of9DVF3Q3CpOPswiIak02wVGEaZmzdKRciGugjeofg3yRL9Ja6jfAl7PK+/sr6uT
fw+Z3Xr7agBWCfG50IN2W3Drn56Yb85J+YmfD0McWVxg/Ndxx/TbQGchRD4XgaA0
N03JP5aED5VOb4AiiUoDQKAzcfLSkKxOaN3NjDngyeove6R/mOejs4pIHl+rq+ih
6z+zbiuSnuqwEXQJS6KqDLjU3vhPuZTmINKQgJo2tJkf1WBhIisRW4i2MV5+AhL5
DFl90/UtMRCWsTYlKNFhTP/4NiLh8l+d9depQBzzjCB+k/LsGkbmHDPgSNDil5+R
HumYbKCCZ6JDwvoFZ8Fy3J4WqC1d/3wwHUWhXnh/MZrPATOTN9f9WZdtpOvQWkUS
UY4M3pbBUsx5xXgKwtmRLie1zHl20WJfsbR+LRvYskaUKqm92QwQFvSUNplCU7/M
1Aq7lDoIDQNpjGVQf94oHYP7p1Poir+yVRggxR0NkB1UGsa1BOuimQ3DzDCxpHB2
Vynnc25nEfFZolEBufLLuIQ0aFDSe8+kS+8ZXYzwqqUUkFOihOo=
=DBIq
-----END PGP SIGNATURE-----

--tPP7hahSjzZLCtxv--