From: Justin Pryzby <pryzby@telsasoft.com>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: gkokolatos@pm.me
Cc: Michael Paquier <michael@paquier.xyz>
Cc: shiy.fnst@fujitsu.com, pgsql-hackers@lists.postgresql.org, Rachel Heaton <rachelmheaton@gmail.com>
Subject: Re: Add LZ4 compression in pg_dump
Date: Tue, 11 Apr 2023 19:41:11 -0500
Message-ID: <ZDX+J72FYmJhH+fC@telsasoft.com> (raw)
In-Reply-To: <qRJGr1JjK3B_pbOxyGqI0iGJWEcRxnq9c7_mIqTY1CIIvYMY0hamPIrvjuS-6gyjr_ItOG8SLRacF1oj7d8R4QKHfZp85YTHI9B-U6LSLAQ=@pm.me> <qSzzmYsXqUf1gDvFspOEbqoE8U_F8IsphDG8c25VZ5KTXimQVYfLvnCcuU7_X2raa2SzZinDF0NpDIKSOALRUYP8pu_NL5QJwIDdR_RT-4U=@pm.me> <8ffd14fe-0278-7f8d-78d4-472816d06268@enterprisedb.com>
On Mon, Feb 27, 2023 at 02:33:04PM +0000, gkokolatos@pm.me wrote:
> > > - Finally, the "Nothing to do in the default case" comment comes from
> > > Michael's commit 5e73a6048:
> > >
> > > + /*
> > > + * Custom and directory formats are compressed by default with gzip when
> > > + * available, not the others.
> > > + /
> > > + if ((archiveFormat == archCustom || archiveFormat == archDirectory) &&
> > > + !user_compression_defined)
> > > {
> > > #ifdef HAVE_LIBZ
> > > - if (archiveFormat == archCustom || archiveFormat == archDirectory)
> > > - compressLevel = Z_DEFAULT_COMPRESSION;
> > > - else
> > > + parse_compress_specification(PG_COMPRESSION_GZIP, NULL,
> > > + &compression_spec);
> > > +#else
> > > + / Nothing to do in the default case */
> > > #endif
> > > - compressLevel = 0;
> > > }
> > >
> > > As the comment says: for -Fc and -Fd, the compression is set to zlib, if
> > > enabled, and when not otherwise specified by the user.
> > >
> > > Before 5e73a6048, this set compressLevel=0 for -Fp and -Ft, and when
> > > zlib was unavailable.
> > >
> > > But I'm not sure why there's now an empty "#else". I also don't know
> > > what "the default case" refers to.
> > >
> > > Maybe the best thing here is to move the preprocessor #if, since it's no
> > > longer in the middle of a runtime conditional:
> > >
> > > #ifdef HAVE_LIBZ
> > > + if ((archiveFormat == archCustom || archiveFormat == archDirectory) &&
> > > + !user_compression_defined)
> > > + parse_compress_specification(PG_COMPRESSION_GZIP, NULL,
> > > + &compression_spec);
> > > #endif
> > >
> > > ...but that elicits a warning about "variable set but not used"...
> >
> >
> > Not sure, I need to think about this a bit.
> /* Nothing to do for the default case when LIBZ is not available */
> is easier to understand.
Maybe I would write it as: "if zlib is unavailable, default to no
compression". But I think that's best done in the leading comment, and
not inside an empty preprocessor #else.
I was hoping Michael would comment on this.
The placement and phrasing of the comment makes no sense to me.
--
Justin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, tomas.vondra@enterprisedb.com, gkokolatos@pm.me, michael@paquier.xyz, rachelmheaton@gmail.com
Subject: Re: Add LZ4 compression in pg_dump
In-Reply-To: <ZDX+J72FYmJhH+fC@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox