Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 19 Dec 2022 13:06:00 +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: <Y5/jKHmkkpH4dzWv@paquier.xyz>
References: <Y4gMAkNGIU1ShaoD@paquier.xyz>
 <jBiqIGoBbxyqLL2syvEDLm1W_TPukmBtAOkFyICkAssd8qupR7zjpA066Cj_syxQOVFsQORr3fc-g_WeATSXvdoybB-e7Xj26Y-C8U7rTKA=@pm.me>
 <Y4lbSoDPlVdbes1u@paquier.xyz>
 <T33dciWfZ3WaU10Mspm2_vsftrl0DI_cs4rQcrT_pLubaLq_HCRlW5QhEOT1cWtSRtdeTVXrL2r2BTYz3Fjz_3YNh6i77lwd5AxNxb5oanQ=@pm.me>
 <Y4q4SihPgXDVCL27@paquier.xyz>
 <Y42YMz/nnNj3fxXG@paquier.xyz>
 <DQn4czCWR1rcbGPLL7p3LfEr5-kGmlySm-H05VgroINdikvhtS5r9EdI6b8D8sjnbKdJ09k-cxs2AqijBeHAWk9Q8gvEAxPRHuLRhwONcGc=@pm.me>
 <Y46LYQZltv7dx/jE@paquier.xyz>
 <4wfmZ9BuGbkM6JXggut4g_gPl1hLbVeUwPEQh5nGgzlKCgnbtRaw7-nPcyPelGdG8ttH9A7vgyX_Z2fwZxuqHt1xENg9mqDfP_94qoZ5WxQ=@pm.me>
 <20221217232615.GS1153@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="qTNiOZ+sT46AVwIu"
Content-Disposition: inline
In-Reply-To: <20221217232615.GS1153@telsasoft.com>
Archived-At: 
 <https://www.postgresql.org/message-id/Y5/jKHmkkpH4dzWv%40paquier.xyz>
Precedence: bulk


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

On Sat, Dec 17, 2022 at 05:26:15PM -0600, Justin Pryzby wrote:
> 001: still refers to "gzip", which is correct for -Fp and -Fd but not
> for -Fc, for which it's more correct to say "zlib".

Or should we begin by changing all these existing "not built with zlib=20
support" error strings to the more generic "this build does not
support compression with %s" to reduce the number of messages to
translate?  That would bring consistency with the other tools dealing
with compression.

> That affects the
> name of the function, structures, comments, etc.  I'm not sure if it's
> an issue to re-use the basebackup compression routines here.  Maybe we
> should accept "-Zn" for zlib output (-Fc), but reject "gzip:9", which
> I'm sure some will find confusing, as it does not output.  Maybe 001
> should be split into a patch to re-use the existing "cfp" interface
> (which is a clear win), and 002 to re-use the basebackup interfaces for
> user input and constants, etc.
>=20
> 001 still doesn't compile on freebsd, and 002 doesn't compile on
> windows.  Have you checked test results from cirrusci on your private
> github account ?

FYI, I have re-added an entry to the CF app to get some automated
coverage:
https://commitfest.postgresql.org/41/3571/

On MinGW, a complain about the open() callback, which I guess ought to
be avoided with a rename:
[00:16:37.254] compress_gzip.c:356:38: error: macro "open" passed 4 argumen=
ts, but takes just 3
[00:16:37.254]   356 |  ret =3D CFH->open(fname, -1, mode, CFH);
[00:16:37.254]       |                                      ^
[00:16:37.254] In file included from ../../../src/include/c.h:1309,
[00:16:37.254]                  from ../../../src/include/postgres_fe.h:25,
[00:16:37.254]                  from compress_gzip.c:15:

On MSVC, some declaration conflicts, for a similar issue:
[00:12:31.966] ../src/bin/pg_dump/compress_io.c(193): error C2371: '_read':=
 redefinition; different basic types
[00:12:31.966] C:\Program Files (x86)\Windows Kits\10\include\10.0.20348.0\=
ucrt\corecrt_io.h(252): note: see declaration of '_read'
[00:12:31.966] ../src/bin/pg_dump/compress_io.c(210): error C2371: '_write'=
: redefinition; different basic types
[00:12:31.966] C:\Program Files (x86)\Windows Kits\10\include\10.0.20348.0\=
ucrt\corecrt_io.h(294): note: see declaration of '_write'

> 002 breaks "pg_dump -Fc -Z2" because (I think) AllocateCompressor()
> doesn't store the passed-in compression_spec.

Hmm.  This looks like a gap in the existing tests that we'd better fix
first.  This CI is green on Linux.

> 003 still uses <lz4.h> and not "lz4.h".

This should be <lz4.h>, not "lz4.h".
--
Michael

--qTNiOZ+sT46AVwIu
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmOf4ygACgkQnvQgOdby
QH1yuA/7BLrWtR5ZjirfS4ArR4OutmcV1L8EIbpHC4LgHfDXvKy0M7shuR8ecTJE
h3bzgynp26U473RaOs7zz4k8DFbQZZ5nQ2OOeRFJtGfN9ogrSF507jOlEKFD+0Yt
Z/aru1N1/TnyYh5X5Uy1YIHxo/8YcFSZy8jrYe8xLWZ/5GqQA6jNpFKe/B2iIf+e
KCOuA7u3K2YfSmnPPlrAg/9XOyq8kQF/ileUZ5OE1ogzrmMIoYV/2Auh3Lq904NB
Pou6ACVoSWX740qJVoqckhsljkUpzZbKBBzuDq5wythY7/2yB/XAblVD7BINNNm8
wRMQF16aZ3j8Bm2IZ9+UsBx1R73yPTU3pbcWinPyD9lWUBCuD/epjOMyiG/Y0fSO
gIhQp1hvcWd9TFnHYqzfu2tHz0TJFMbqsjGM+HKr9VRJOuvgwNGq+8YvH9tELcTt
9mUzzPkCEn3Op7kmV2l99unMkzj1Rm7wxDfxZn1t/Do2YVdwY3S2SXAuXwh1ePv4
F8Vx3/yDmDUNeKq8h9msuXt2a8GE4oSlIxiLyYkq/xW+uHbiT0wpPClnpfrDg+5y
IxUR0Ct/9Il4pR3blWjnLig6gdoMM2M5+lRcdwCfhCZwoEKdplRQ+zOKtXpqtZSy
74SVs0b/DcI9fEcEQUbRSBiOySR9reQzOPidCd/i67linRA6wEs=
=H1Rd
-----END PGP SIGNATURE-----

--qTNiOZ+sT46AVwIu--