Horton

Re: Add LZ4 compression in pg_dump

Justin Pryzby <pryzby@telsasoft.com>

View thread View on postgr.es
From: Justin Pryzby <pryzby@telsasoft.com>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>, gkokolatos@pm.me
Cc: Michael Paquier <michael@paquier.xyz>, shiy.fnst@fujitsu.com, pgsql-hackers@lists.postgresql.org, Rachel Heaton <rachelmheaton@gmail.com>
Date: 2023-04-12T00:41:11Z
Lists: pgsql-hackers 
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

Commits

  1. Advance input pointer when LZ4 compressing data

  2. Null-terminate the output buffer of LZ4Stream_gets

  3. Rework code defining default compression for dir/custom formats in pg_dump

  4. pg_dump: Use only LZ4 frame format for compression

  5. Minor comment improvements for compress_lz4

  6. Unify buffer sizes in pg_dump compression API

  7. Improve type handling in pg_dump's compress file API

  8. Improve wording in pg_dump compression docs

  9. Fix condition in pg_dump TAP test

  10. Add LZ4 compression to pg_dump

  11. Introduce a generic pg_dump compression API

  12. Prepare pg_dump internals for additional compression methods

  13. Fix behavior with pg_restore -l and compressed dumps

  14. Add 250c8ee07ed to git-blame-ignore-revs

  15. Provide test coverage in pg_dump for default behaviors with compression

  16. Switch pg_dump to use compression specifications

  17. Refactor code parsing compression option values (-Z/--compress)

  18. meson: Add some missing env settings for tests of pg_dump and pg_verifybackup

  19. Extend TAP tests of pg_dump to test for compression with gzip

  20. Clean up some dead code in pg_dump with tar format and gzip compression

  21. Add TAP test in pg_dump with --format=tar and --compress

  22. Replace BASE_BACKUP COMPRESSION_LEVEL option with COMPRESSION_DETAIL.

  23. Refactor the pg_dump zlib code from pg_backup_custom.c to a separate file,