Date: Sat, 17 Dec 2022 17:26:15 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: gkokolatos@pm.me
Cc: Michael Paquier <michael@paquier.xyz>,
	pgsql-hackers@lists.postgresql.org,
	Rachel Heaton <rachelmheaton@gmail.com>
Subject: Re: Add LZ4 compression in pg_dump
Message-ID: <20221217232615.GS1153@telsasoft.com>
References: 
 <O4mutIrCES8ZhlXJiMvzsivT7ztAMja2lkdL1LJx6O5f22I2W8PBIeLKz7mDLwxHoibcnRAYJXm1pH4tyUNC4a8eDzLn22a6Pb1S74Niexg=@pm.me>
 <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <4wfmZ9BuGbkM6JXggut4g_gPl1hLbVeUwPEQh5nGgzlKCgnbtRaw7-nPcyPelGdG8ttH9A7vgyX_Z2fwZxuqHt1xENg9mqDfP_94qoZ5WxQ=@pm.me>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20221217232615.GS1153%40telsasoft.com>
Precedence: bulk

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".  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.

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 ?

002 says:
+       save_errno = errno;                                                                                                                                                                                 
+       errno = save_errno;                                                                                                                                                                                 

I suppose that's intended to wrap the preceding library call.

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

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

Earlier this year I also suggested to include an 999 patch to change to
use LZ4 as the default compression, to exercise the new code under CI.
I suggest to re-open the cf patch entry after that passes tests on all
platforms and when it's ready for more review.

BTW, some of these review comments are the same as what I sent earlier
this year.

https://www.postgresql.org/message-id/20220326162156.GI28503%40telsasoft.com
https://www.postgresql.org/message-id/20220705151328.GQ13040%40telsasoft.com

-- 
Justin