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: gkokolatos@pm.me
Cc: Michael Paquier <michael@paquier.xyz>, Tomas Vondra <tomas.vondra@enterprisedb.com>, pgsql-hackers@lists.postgresql.org, Rachel Heaton <rachelmheaton@gmail.com>
Date: 2023-01-27T17:23:20Z
Lists: pgsql-hackers 
On Thu, Jan 26, 2023 at 12:22:45PM -0600, Justin Pryzby wrote:
> That commit also added this to pg-dump.c:
> 
> +               case PG_COMPRESSION_ZSTD:
> +                       pg_fatal("compression with %s is not yet supported", "ZSTD");
> +                       break;
> +               case PG_COMPRESSION_LZ4:
> +                       pg_fatal("compression with %s is not yet supported", "LZ4");
> +                       break;
> 
> In 002, that could be simplified by re-using the supports_compression()
> function.  (And maybe the same in WriteDataToArchive()?)

The first patch aims to minimize references to ".gz" and "GZIP" and
ZLIB.  pg_backup_directory.c comments still refers to ".gz".  I think
the patch should ideally change to refer to "the compressed file
extension" (similar to compress_io.c), avoiding the need to update it
later.

I think the file extension stuff could be generalized, so it doesn't
need to be updated in multiple places (pg_backup_directory.c and
compress_io.c).  Maybe it's useful to add a function to return the
extension of a given compression method.  It could go in compression.c,
and be useful in basebackup.

For the 2nd patch:

I might be in the minority, but I still think some references to "gzip"
should say "zlib":

+} GzipCompressorState;
+
+/* Private routines that support gzip compressed data I/O */
+static void
+DeflateCompressorGzip(ArchiveHandle *AH, CompressorState *cs, bool flush)

In my mind, three things here are misleading, because it doesn't use
gzip headers:

| GzipCompressorState, DeflateCompressorGzip, "gzip compressed".

This comment is about exactly that:

  * underlying stream. The second API is a wrapper around fopen/gzopen and
  * friends, providing an interface similar to those, but abstracts away
  * the possible compression. Both APIs use libz for the compression, but
  * the second API uses gzip headers, so the resulting files can be easily
  * manipulated with the gzip utility.

AIUI, Michael says that it's fine that the user-facing command-line
options use "-Z gzip" (even though the "custom" format doesn't use gzip
headers).  I'm okay with that, as long as that's discussed/understood.

-- 
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,