Date: Fri, 4 Mar 2022 16:19:32 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Andrey Borodin <x4mmm@yandex-team.ru>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	pgsql-hackers <pgsql-hackers@postgresql.org>,
	Thomas Munro <thomas.munro@gmail.com>, Peter Geoghegan <pg@bowt.ie>,
	Andres Freund <andres@anarazel.de>,
	Robert Haas <robertmhaas@gmail.com>
Subject: Re: wal_compression=zstd
Message-ID: <YiG9hOS/vivHvJny@paquier.xyz>
References: <20220222231948.GJ9008@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="/AGASGmVb8ttb/vk"
Content-Disposition: inline
In-Reply-To: <20220222231948.GJ9008@telsasoft.com>
Archived-At: <https://www.postgresql.org/message-id/YiG9hOS/vivHvJny%40paquier.xyz>
Precedence: bulk


--/AGASGmVb8ttb/vk
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Feb 22, 2022 at 05:19:48PM -0600, Justin Pryzby wrote:
> I am not claiming that zstd is generally better for WAL.  Rather, if we're
> going to support alternate compression methods, it's nice to give a couple
> options (in addition to pglz).  Some use cases would certainly suffer from
> slower WAL.  But providing the option will benefit some others.  Note that a
> superuser can set wal_compression for a given session - this would probably
> benefit bulk-loading in an otherwise OLTP environment.

Well, I cannot say which one is better either as it depends on your
workload, mostly, but we know for sure that both have benefits, so I
don't mind adding it now.  What you are proposing is built on top of
the existing code, making it a very simple change.

> As writen, this patch uses zstd level=1 (whereas the ZSTD's default compress
> level is 6).

Why?  ZSTD using this default has its reasons, no?  And it would be
consistent to do the same for ZSTD as for the other two methods.

-        The supported methods are <literal>pglz</literal> and
-        <literal>lz4</literal> (if <productname>PostgreSQL</productname> was
-        compiled with <option>--with-lz4</option>). The default value is
-        <literal>off</literal>. Only superusers can change this setting.
+        The supported methods are <literal>pglz</literal>, and
+        (if configured when <productname>PostgreSQL</productname>was built)
+        <literal>lz4</literal> and zstd.
+        The default value is <literal>off</literal>.
+        Only superusers can change this setting.

This is making the docs less verbose.  I think that you should keep
the mention to respectively --with-lz4 and --with-zstd after each
value.

       <para>
        Build with <productname>ZSTD</productname> compression support.
+       This enables use of <productname>ZSTD</productname> for
+       compression of WAL data.

This addition is not necessary, see d7a9786.

Not related to your patch, but we have more reasons to add an
check in the block of BKPIMAGE_COMPRESSED() in RestoreBlockImage() of
xlogreader.c to make sure that only one bit is set for the compression
type.  We could use a pg_popcount() == 1 for that, returning
report_invalid_record() when we see some corrupted data.

As a whole, 0001 looks pretty good to me after a detailed read (not
tested, though).

> Only 0001 should be reviewed for pg15 - the others are optional/future work.

That's wiser for v15.  FWIW, I have the impression that we don't need
most of what's proposed in 0002~.

 /* compression methods supported */
-#define BKPIMAGE_COMPRESS_PGLZ 0x04
-#define BKPIMAGE_COMPRESS_LZ4  0x08
-#define BKPIMAGE_COMPRESS_ZSTD 0x10
-
+#define BKPIMAGE_COMPRESS_METHOD1      0x04    /* bits to encode  compression method */
+#define BKPIMAGE_COMPRESS_METHOD2      0x08    /* 0=none, 1=pglz, 2=lz4, 3=zstd */

As of 0002.  We still have some room for this data and this makes the
code harder to follow, so I'd live this part of the code as it is
proposed in 0001.

0003, defaulting to zstd, and 0004 to extend compression to support a
level would require a lot of benchmarking to be justified.  I have
argued against making the code more complicated for such things in the
past, with reloptions.  The footprint on the code is much smaller,
here, still..

0007, for ZLIB, does not make sense once one can choose between LZ4
and ZSTD.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmIhvYQACgkQnvQgOdby
QH2Daw/+PUATCY15g5QTIl/8Hb2Xf1czzrxyKtRNeyPlS1tbe0NUdA0Fz/4MKSQE
8Vx2TEdoGdxv583bqV2YzwuKCOHaAZJrePV3Jj+hjBMf+YdSqi9Q8Z+xDWu4mEHM
yVv/5sGLXBT3FCuQPHi316ateYp1BUifc4rjK45RagKf0RDyAPhh6Xt6bzNYe40p
ZxbZpBraFKQGwSsnrMl04ZLTa6DF2a68AQC3hdlEtktVKDPSUNTrxQF6Bk67M1VD
1K5xbbPhhJQVfL+LpBTZDpywgIaTYhr8bTqFYb362SMKzXNIbAMdg4aY4Nxw5jxx
US/UjTImPiJrGqitRCcHeTFySX+XgPvagtULrJclWmsLgOHvrHpXZW+LO+bfmLCF
V/zhijaG8kVoQ8bGcbkYf7WBTVimazh1xt5GpxkQ37a1OaXf21lK+Hx1Pzf6+KdO
ZhFE7IiOMS+Zu19fb7HYbdvSpkdU4N/8joJgA7JIhoILElI2OQWFlM2Ph6RBUWOr
fT1QDQfNG5qPUvBeZWTAwJbauuIII1TLCPeOou6lFz4p79hxMML4+rzKc9k6qCdM
46e4q1fp1aEn8eOnnpVp7ajil7hMKN4wllDD4ZyLZKTJl+TB9d2rSlUsXpFH8xBl
Omg9iNRLivER1R/wN6hdjPWuJg0Wm5nbXFxH0Wyfdui6rFYCXlU=
=WH87
-----END PGP SIGNATURE-----

--/AGASGmVb8ttb/vk--