Date: Tue, 1 Mar 2022 17:04:35 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Georgios <gkokolatos@protonmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Rachel Heaton <rachelmheaton@gmail.com>
Subject: Re: Add LZ4 compression in pg_dump
Message-ID: <Yh3Tky8vyhMwb4yG@paquier.xyz>
References: <faUNEOpts9vunEaLnmxmG-DldLSg_ql137OC3JYDmgrOMHm1RvvWY2IdBkv_CRxm5spCCb_OmKNk2T03TMm0fBEWveFF9wA1WizPuAgB7Ss=@protonmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="aiqI1NAxYgfOrY/X"
Content-Disposition: inline
In-Reply-To: <faUNEOpts9vunEaLnmxmG-DldLSg_ql137OC3JYDmgrOMHm1RvvWY2IdBkv_CRxm5spCCb_OmKNk2T03TMm0fBEWveFF9wA1WizPuAgB7Ss=@protonmail.com>
Archived-At: <https://www.postgresql.org/message-id/Yh3Tky8vyhMwb4yG%40paquier.xyz>
Precedence: bulk


--aiqI1NAxYgfOrY/X
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Feb 25, 2022 at 12:05:31PM +0000, Georgios wrote:
> The first commit does the heavy lifting required for additional compression methods.
> It expands testing coverage for the already supported gzip compression. Commit
> bf9aa490db introduced cfp in compress_io.{c,h} with the intent of unifying
> compression related code and allow for the introduction of additional archive
> formats. However, pg_backup_archiver.c was not using that API. This commit
> teaches pg_backup_archiver.c about cfp and is using it through out.

Thanks for the patch.  I have a few high-level comments.

+   # Do not use --no-sync to give test coverage for data sync.
+   compression_gzip_directory_format => {
+       test_key => 'compression',

The tests for GZIP had better be split into their own commit, as
that's a coverage improvement for the existing code.

I was assuming that this was going to be much larger :)

+/* Routines that support LZ4 compressed data I/O */
+#ifdef HAVE_LIBLZ4
+static void InitCompressorLZ4(CompressorState *cs);
+static void ReadDataFromArchiveLZ4(ArchiveHandle *AH, ReadFunc
readF);
+static void WriteDataToArchiveLZ4(ArchiveHandle *AH, CompressorState *cs,
+                                 const char *data, size_t dLen);
+static void EndCompressorLZ4(ArchiveHandle *AH, CompressorState *cs);
+#endif

Hmm.  This is the same set of APIs as ZLIB and NONE to init, read,
write and end, but for the LZ4 compressor (NONE has no init/end).
Wouldn't it be better to refactor the existing pg_dump code to have a
central structure holding all the function definitions in a common
structure so as all those function signatures are set in stone in the
shape of a catalog of callbacks, making the addition of more
compression formats easier?  I would imagine that we'd split the code
of each compression method into their own file with their own context
data.  This would lead to a removal of compress_io.c, with its entry
points ReadDataFromArchive(), WriteDataToArchive() & co replaced by
pointers to each per-compression callback.

> Furthermore, compression was chosen based on the value of the level passed
> as an argument during the invocation of pg_dump or some hardcoded defaults. This
> does not scale for more than one compression methods. Now the method used for
> compression can be explicitly requested during command invocation, or set during
> hardcoded defaults. Then it is stored in the relevant structs and passed in the
> relevant functions, along side compression level which has lost it's special
> meaning. The method for compression is not yet stored in the actual archive.
> This is done in the next commit which does introduce a new method.

That's one thing Robert was arguing about with pg_basebackup, so that
would be consistent, and the option set is backward-compatible as far
as I get it by reading the code.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmId05MACgkQnvQgOdby
QH1mBhAAo/AyyBbJi/8vu7sx0a72VlMePX3WdCMI6HyD8WByHFd3NlUhrIJeV69f
BoReyY8/H9UiIVbn8ngHdpmGLdfZQrhV21Id9sap9/IGntjekUrM2G0STWVeHll/
0H9nlVbC+3gHWA/LQstEd2tNTOIuvMVskWVmBkHKVP8fWL3BBUOQZp1nvQOsEblt
78LX4VZ6IK9lJUbrhTNQYOJ/JUQJMvpKsiFs8/p3Qxw/s8WhbolM7OSy6/XCHxpv
y+lqhG+N0RLC2Va/iYbqeJzEew93bmLiltym5jd4wrYs7zs03Y9xSck11G+C5qL0
VcgtgZbHXKvYS33FRuCMxIsGq3mzudNKHzoo3qxRqcFie8BF1XVL+092kzIhdlRM
BDJUKN7yHCFR4pZaSVwo1+wYqyrqPPWam8oV1p8J+sjG72bvmsVBzmnOirpkp485
wsTstEUg9HTJZVj+mJaO63x0XBF9Q2ESsaSFjiSsN1niX1slRhkxyS5EzMC/5Lx+
0GNCoFnW6p8obyH8BxLAV7K/WehyDJn9b01+x+4gx+O9wEIC///1W3Y6GJCHkb3w
4ZN7/hQxY0Vg/JhJGZgldFnqYH5SUXmm+Rs+jPmrQs4PlWzc/zugzOeTSDd2PH51
/KQlQDHabDQV9K46De+UHWC4me4YppnZPIjBS3jUcvvfilaCwl4=
=3qt7
-----END PGP SIGNATURE-----

--aiqI1NAxYgfOrY/X--