Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 21 Jul 2022 14:04:18 +0900
From: Michael Paquier <michael@paquier.xyz>
To: gkokolatos@pm.me
Cc: Justin Pryzby <pryzby@telsasoft.com>,
	pgsql-hackers@lists.postgresql.org,
	Rachel Heaton <rachelmheaton@gmail.com>
Subject: Re: Add LZ4 compression in pg_dump
Message-ID: <YtjeUvZxIN+yIEMw@paquier.xyz>
References: <20220626155532.GA9311@telsasoft.com>
 <u7dLpRH_6r_rGyqNWHSDx2Wz5g1virAU3-f4XMC2BwiR81Tegqm4Sxtte0YAe77je9KVMa4jVxDNSt3XkYLYpyOoabCid1Bm7BZxwoitcmY=@pm.me>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="ZfvhrjBif7up7sam"
Content-Disposition: inline
In-Reply-To: 
 <u7dLpRH_6r_rGyqNWHSDx2Wz5g1virAU3-f4XMC2BwiR81Tegqm4Sxtte0YAe77je9KVMa4jVxDNSt3XkYLYpyOoabCid1Bm7BZxwoitcmY=@pm.me>
Archived-At: 
 <https://www.postgresql.org/message-id/YtjeUvZxIN%2ByIEMw%40paquier.xyz>
Precedence: bulk


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

On Tue, Jul 05, 2022 at 01:22:47PM +0000, gkokolatos@pm.me wrote:
> I have updated for "some" of the comments. This is not an unwillingness to
> incorporate those specific comments. Simply this patchset had started to =
divert
> heavily already based on comments from Mr. Paquier who had already reques=
ted for
> the APIs to be refactored to use function pointers. This is happening in =
0002 of
> the patchset. 0001 of the patchset is using the new compression.h under c=
ommon.
>=20
> This patchset should be considered a late draft, as commentary, documenta=
tion,
> and some finer details are not yet finalized; because I am expecting the =
proposed
> refactor to receive a wealth of comments. It would be helpful to understa=
nd if
> the proposed direction is something worth to be worked upon, before movin=
g to the
> finer details.

I have read through the patch set, and I like a lot the separation you
are doing here with CompressFileHandle where a compression method has
to specify a full set of callbacks depending on the actions that need
to be taken.  One advantage, as you patch shows, is that you reduce
the dependency of each code path depending on the compression method,
with #ifdefs and such located mostly into their own file structure, so
as adding a new compression method becomes really easier.  These
callbacks are going to require much more documentation to describe
what anybody using them should expect from them, and perhaps they
could be renamed in a more generic way as the currect names come from
POSIX (say read_char(), read_string()?), even if this patch has just
inherited the names coming from pg_dump itself, but this can be tuned
over and over.

The split into three parts as of 0001 to plug into pg_dump the new
compression option set, 0002 to introduce the callbacks and 0003 to
add LZ4, building on the two first parts, makes sense to me.  0001 and
0002 could be done in a reversed order as they are mostly independent,
this order is fine as-is.

In short, I am fine with the proposed approach.

+#define K_VERS_1_15 MAKE_ARCHIVE_VERSION(1, 15, 0) /* add compressionMethod
+                                                    * in header */
Indeed, the dump format needs a version bump for this information.

+static bool
+parse_compression_option(const char *opt,
+                        pg_compress_specification *compress_spec)
This parsing logic in pg_dump.c looks a lot like what pg_receivewal.c
does with its parse_compress_options() where, for compatibility:
- If only a number is given:
-- Assume no compression if level is 0.
-- Assume gzip with given compression if level > 0.
- If a string is found, assume a full spec, with optionally a level.
So some consolidation could be done between both.

By the way, I can see that GZCLOSE(), etc. are still defined in
compress_io.h but they are not used.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmLY3lIACgkQnvQgOdby
QH0EwQ/+NPiyQY6Cuk4JByS2X7pS690vLhtbZIsi3Vx34DpAKFrPy0G6cI2/bgcU
34MpHqwnRRGp4HVaf83bEUD6ktBpx1cPaGHpRy3Q3SGkIgEcHu5aGQ/V8H9skYYf
C6LDDjF8H++q2J9eM/0xLyD6DlfNaTi7fgi5Aok+y35vVDnkDYBBvP+Ha20HDOfG
+vKl4YaWXwyR6xGTEWVYCbXhUP0yhWp5PcV7KsA5UWPfRRvGcsmAinqXQfSdFngp
k2duXIn8EqBExJvGY/AxjqcUnLk8wUppoK7RBHteNnqpKFfjxNrL1ke3+4P7iF2X
ZKGNw/rLYwEay5SzVCabmlmwhCdmzIHgAHTqXRfnMB5Cv+FGw2VL0bWTnwCYuMyK
Wtf7yEAlZuOfss1t6RE49mUc2IsjZwGYdmTjlgAcWbrPLaG2b+P5cGAN4iMEsKNK
dQmv8HsXlGTXlT5ZicW0b3h2cgEE8BqTCnqeSzsSQJ1Ipl7pmfqykDIQqgRwNK5Z
jP075trFs9dCJRH1gThlQfQIxNiVn0kqFncYLOeYGOVWZL4Hwn1ZEDzH5kLCakOn
FWHeKTgteFBsp+vgB/k5pt3DfGV+EhwZ602ovmUa9vIV9rLp1pmeKfddEdPG6r4l
7cTBcHa42t8WX1S7S7c0+Y7nGPVIW1j+afIs5Bc/Kquv4sQ/zEk=
=/WHJ
-----END PGP SIGNATURE-----

--ZfvhrjBif7up7sam--