Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 16 Jan 2023 10:28:50 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: gkokolatos@pm.me, pgsql-hackers@lists.postgresql.org,
	Rachel Heaton <rachelmheaton@gmail.com>
Subject: Re: Add LZ4 compression in pg_dump
Message-ID: <Y8SoUuqbNoMBnFJp@paquier.xyz>
References: <20221217232615.GS1153@telsasoft.com>
 <Y5/jKHmkkpH4dzWv@paquier.xyz>
 <IZzs7Z2ny5hRK1EhTynSg3DnCo_z-UvEJN4Q6Hf0CUHWx3BcNwt-2z5vvvvA-Z8PQL-AYiL9YQPlKylATZ_Z6HnJ55YZUHgL66H9MZQFi48=@pm.me>
 <20221219172706.GA1153@telsasoft.com>
 <srwGkjEwoQg3cdO9k1mMuIu9eEqkOA27Ikvf0cNR9mvBLVZPidKMk4fmKeVIsxVkgbVk2yeIL-Neyjb2Mkz9_ULGfk2qeYW8ZnM1iKIyklY=@pm.me>
 <20221220152610.GG1153@telsasoft.com>
 <f8Wmj1EmRT7CHBfMcg0jM_Wts_hyV22WsLWzf5X5KImQo_5CtZXHK3UjxL8Wne6g7zSccwXLNToLCINfEIPbNni53gKO17AuHplMjm-XBmA=@pm.me>
 <20221222170859.GO1153@telsasoft.com>
 <20230108194524.GA27637@telsasoft.com>
 <20230114214308.GC9837@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Gc5SdB1sLC36Oh1j"
Content-Disposition: inline
In-Reply-To: <20230114214308.GC9837@telsasoft.com>
Archived-At: <https://www.postgresql.org/message-id/Y8SoUuqbNoMBnFJp%40paquier.xyz>
Precedence: bulk


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

On Sat, Jan 14, 2023 at 03:43:09PM -0600, Justin Pryzby wrote:
> On Sun, Jan 08, 2023 at 01:45:25PM -0600, Justin Pryzby wrote:
>> pg_compress_specification is being passed by value, but I think it
>> should be passed as a pointer, as is done everywhere else.
>=20
> ISTM that was an issue with 5e73a6048, affecting a few public and
> private functions.  I wrote a pre-preparatory patch which changes to
> pass by reference.

The functions changed by 0001 are cfopen[_write](),
AllocateCompressor() and ReadDataFromArchive().  Why is it a good idea
to change these interfaces which basically exist to handle inputs?  Is
there some benefit in changing compression_spec within the internals
of these routines before going back one layer down to their callers?
Changing the compression_spec on-the-fly in these internal paths could
be risky, actually, no?

> And addressed a handful of other issues I reported as separate fixup
> commits.  And changed to use LZ4 by default for CI.

Are your slight changes shaped as of 0003-f.patch, 0005-f.patch and
0007-f.patch on top of the original patches sent by Georgios?

> I also rebased my 2 year old patch to support zstd in pg_dump.  I hope
> it can finally added for v16.  I'll send it for the next CF if these
> patches progress.

Good idea to see if what you have done for zstd fits with what's
presented here.

>> pg_compress_algorithm is being writen directly into the pg_dump header.

Do you mean that this is what happens once the patch series 0001~0008
sent upthread is applied on HEAD?

>> Currently, I think that's not an externally-visible value (it could be
>> renumbered, theoretically even in a minor release).  Maybe there should
>> be a "private" enum for encoding the pg_dump header, similar to
>> WAL_COMPRESSION_LZ4 vs BKPIMAGE_COMPRESS_LZ4 ?  Or else a comment there
>> should warn that the values are encoded in pg_dump, and must never be
>> changed.
>=20
> Michael, WDYT ?

Changing the order of the members in an enum would cause an ABI
breakage, so that would not happen, and we tend to be very careful
about that.  Appending new members would be fine, though.  FWIW, I'd
rather avoid adding more enums that would just be exact maps to
pg_compress_algorithm.

-   /*
-    * For now the compression type is implied by the level.  This will need
-    * to change once support for more compression algorithms is added,
-    * requiring a format bump.
-    */
-   WriteInt(AH, AH->compression_spec.level);
+   AH->WriteBytePtr(AH, AH->compression_spec.algorithm);

I may be missing something here, but it seems to me that you ought to
store as well the level in the dump header, or it would not be
possible to report in the dump's description what was used?  Hence,
K_VERS_1_15 should imply that we have both the method compression and
the compression level.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmPEqFIACgkQnvQgOdby
QH0PVA/+KCrhjH+LAtpAp3b8kOmPG4my/UI9B0H5NRIq4jPMQdfz1Id45JvOhkCF
vuRFlQZJ33aihkGnQLZZR9PNltabLDtGMugP57buZ+CjsB1BtySd1T2DHyjdecGm
0Up6eOrgULPfbtNbwCQ4RLG4B29BrKlrhQkOK/hN3yrWfu1vfZp6i2NWEVFdfeLH
RDzIpSaNiVul50MgWCWP96vWdn9g+4CcZheXKyTXFkc1Tp/jD4oSWE+hIuLBVq+7
Suv6URni718GMqcHYkTETI9IUlOZl5N8dh6TOUQxQpPFurafaiIoQhyxf25azrhJ
e0kkmBVH/LXMJozEPnKgbAFQltqXJPf2quqtARdtlT5g4u0/iQLmfaFs9Mpev3YV
1wvUboZ0IGXutFFkmMVQbue6NFC8Oz2q1htbTuXU0qDwslG8PcD09ME0j13lK7hn
WW4lqQsmESe1dVxRL0OmHG20hNN5Ia+8wzEgt1cI4GiLjA5vmTulbum8JETjKRzq
5BZVMDcDNNw8p0VoLEI4L6YiV+2Bz75cipB8faGrFtb8bS6Gcr93ZD+KWCuzntEG
DFqQSYQDulzMH9vXL/DrRb3NMmmartxLFkUEwpy3GgLiMBqeJsZ04N0jTftDDbL0
E2tyjQ9WfW7phtd+g6xUnpEcXVH96NIY+PnsVf2+t7UFyvIMnkw=
=AA5J
-----END PGP SIGNATURE-----

--Gc5SdB1sLC36Oh1j--