Horton

Re: Add LZ4 compression in pg_dump

gkokolatos@pm.me

View thread View on postgr.es
From: gkokolatos@pm.me
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>, Alexander Lakhin <exclusion@gmail.com>, shiy.fnst@fujitsu.com, Michael Paquier <michael@paquier.xyz>, pgsql-hackers@lists.postgresql.org, Rachel Heaton <rachelmheaton@gmail.com>
Date: 2023-03-31T09:19:47Z
Lists: pgsql-hackers

Attachments





------- Original Message -------
On Wednesday, March 29th, 2023 at 12:02 AM, Tomas Vondra <tomas.vondra@enterprisedb.com> 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.
> 
> 
> I think the patch is fine, but I'm wondering if the renames shouldn't go
> a bit further. It removes references to LZ4File struct, but there's a
> bunch of functions with LZ4File_ prefix. Why not to simply use LZ4_
> prefix? We don't have GzipFile either.
> 
> Sure, it might be a bit confusing because lz4.h uses LZ4_ prefix, but
> then we probably should not define LZ4_compressor_init ...

This is a good point. The initial thought was that since lz4.h is now
removed, such ambiguity will not be present. In v2 of the patch the
function is renamed to `LZ4State_compression_init` since this name
describes better its purpose. It initializes the LZ4State for
compression.

As for the LZ4File_ prefix, I have no objections. Please find the
prefix changed to LZ4Stream_. For the record, the word 'File' is not
unique to the lz4 implementation. The common data structure used by
the API in compress_io.h:

   typedef struct CompressFileHandle CompressFileHandle; 

The public functions for this API are named:

  InitCompressFileHandle
  InitDiscoverCompressFileHandle
  EndCompressFileHandle

And within InitCompressFileHandle the pattern is:

    if (compression_spec.algorithm == PG_COMPRESSION_NONE)
        InitCompressFileHandleNone(CFH, compression_spec);
    else if (compression_spec.algorithm == PG_COMPRESSION_GZIP)
        InitCompressFileHandleGzip(CFH, compression_spec);
    else if (compression_spec.algorithm == PG_COMPRESSION_LZ4)
        InitCompressFileHandleLZ4(CFH, compression_spec);

It was felt that a prefix was required due to the inclusion 'lz4.h'
header where naming functions as 'LZ4_' would be wrong. The prefix
'LZ4File_' seemed to be in line with the naming of the rest of
the relevant functions and structures. Other compressions, gzip and
none, did not face the same issue.

To conclude, I think that having a prefix is slightly preferred
over not having one. Since the prefix `LZ4File_` is not desired,
I propose `LZ4Stream_` in v2.

I will not object to dismissing the argument and drop `File` from
the prefix, if so requested.

> 
> Also, maybe the comments shouldn't use "File API" when compress_io.c
> calls that "Compressed stream API".

Done.

Cheers,
//Georgios

> 
> 
> regards
> 
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company

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,