Date: Tue, 28 Mar 2023 13:47:21 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: gkokolatos@pm.me, Alexander Lakhin <exclusion@gmail.com>,
	shiy.fnst@fujitsu.com, 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: <ZCM2OQUFDzmbH8q3@telsasoft.com>
References: 
 <5qAu7vrMV3hEodlIC82b0JICWTwEUSRoIDWlIaczNBIEVVfDH2lccxHNIQ726e4nX_Qqjc4lPc5_z9RtV28NMdVv7BNiADF4JB-FDahmlVk=@pm.me>
 <d8315656-3b35-7d9a-c799-8d48fdccfe45@enterprisedb.com>
 <JduQzRIUMhF5NDvC_klfyHTDhzb5Py60xH9ICRUtluHQ4dD02nw-BH7WfX7DQG6gaFWl1Q919RvX3Xvm_5mau_0YKRQeUs6ezUSFDlFVPsE=@pm.me>
 <32ba9612-1982-4b2d-135a-5d1222987082@enterprisedb.com>
 <XPMIIorJ5munrlLRZGquartiN5tuapdjiBhaPZ6iEmJccRiKWCIHSowSLPhaGj4gLpqvaS1pQn7PGkA_WNwvuUTVdl-lrU_TzHA81LqujJ4=@pm.me>
 <ZBjg4D/X05X+JbcJ@telsasoft.com>
 <3beb0dd7-c927-3eca-689f-716e255d5b27@enterprisedb.com>
 <JuDkpaGyAgfhoxfL71N4Bg1vGqzKW7bfardMjsfLjcqyQPqf-V-AW13jSDtCLQRYEzasmzwRJQED1dkx6_6uY-ocGJS54rNpPqM4iC36pSw=@pm.me>
 <bFjRZQCZego2j9Lsvunr4XtBEKRmCdxATU486xblkV4_qyePJoXoK1uItH48NzgeHki-w-3IlXPVNre4qDF_h6NRbf2VMhhP4v6FFWMK_-g=@pm.me>
 <96ba63e1-5ad6-1873-c3f5-c12a0a921c5e@enterprisedb.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <96ba63e1-5ad6-1873-c3f5-c12a0a921c5e@enterprisedb.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZCM2OQUFDzmbH8q3%40telsasoft.com>
Precedence: bulk

On Tue, Mar 28, 2023 at 06:40:03PM +0200, Tomas Vondra wrote:
> On 3/28/23 18:07, gkokolatos@pm.me wrote:
> > ------- Original Message -------
> > On Friday, March 24th, 2023 at 10:30 AM, gkokolatos@pm.me <gkokolatos@pm.me> wrote:
> > 
> >> ------- Original Message -------
> >> On Thursday, March 23rd, 2023 at 6:10 PM, Tomas Vondra tomas.vondra@enterprisedb.com wrote:
> >>
> >>> This leaves the empty-data issue (which we have a fix for) and the
> >>> switch to LZ4F. And then the zstd part.
> >>
> >> Please expect promptly a patch for the switch to frames.
> > 
> > Please find the expected patch attached. Note that the bulk of the
> > patch is code unification, variable renaming to something more
> > appropriate, and comment addition. These are changes that are not
> > strictly necessary to switch to LZ4F. I do believe that are
> > essential for code hygiene after the switch and they do belong
> > on the same commit. 
> 
> Thanks!
> 
> I agree the renames & cleanup are appropriate - it'd be silly to stick
> to misleading naming etc. Would it make sense to split the patch into
> two, to separate the renames and the switch to lz4f?
> That'd make it the changes necessary for lz4f switch clearer.

I don't think so.  Did you mean separate commits only for review ?

The patch is pretty readable - the File API has just some renames, and
the compressor API is what's being replaced, which isn't going to be any
more clear.

@Georgeos: did you consider using a C union in LZ4State, to separate the
parts used by the different APIs ?

-- 
Justin