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: Michael Paquier <michael@paquier.xyz>, Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@lists.postgresql.org, Rachel Heaton <rachelmheaton@gmail.com>
Date: 2023-01-18T19:05:43Z
Lists: pgsql-hackers

Attachments





------- Original Message -------
On Wednesday, January 18th, 2023 at 3:00 PM, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote:


> 
> 
> Hi,
> 
> On 1/16/23 16:14, gkokolatos@pm.me wrote:
> 
> > Hi,
> > 
> > I admit I am completely at lost as to what is expected from me anymore.
> 
<snip>
> 
> Unfortunately, this plays against this patch - I'm certainly in favor of
> adding lz4 (and other compression algos) into pg_dump, but if I commit
> 0001 we get little benefit, and the other parts actually adding lz4/zstd
> are treated as "WIP / for completeness" so it's unclear when we'd get to
> commit them.

Thank you for your kindness and for taking the time to explain.
 
> So if I could recommend one thing, it'd be to get at least one of those
> WIP patches into a shape that's likely committable right after 0001.

This was clearly my fault. I misunderstood a suggestion upthread to focus
on the first patch of the series and ignore documentation and comments on
the rest.

Please find v21 to contain 0002 and 0003 in a state which I no longer consider
as WIP but worthy of proper consideration. Some guidance on where is best to add
documentation in 0002 for the function pointers in CompressFileHandle will
be welcomed.

> 
> > I had posted v19-0001 for a committer's consideration and v19-000{2,3} for completeness.
> > Please find a rebased v20 attached.
> 
> 
> I took a quick look at 0001, so a couple comments (sorry if some of this
> was already discussed in the thread):

Much appreciated!

> 
> 1) I don't think a "refactoring" patch should reference particular
> compression algorithms (lz4/zstd), and in particular I don't think we
> should have "not yet implemented" messages. We only have a couple other
> places doing that, when we didn't have a better choice. But here we can
> simply reject the algorithm when parsing the options, we don't need to
> do that in a dozen other places.

I have now removed lz4/zstd from where they were present with the exception
of pg_dump.c which is responsible for parsing.

> 2) I wouldn't reorder the cases in WriteDataToArchive, i.e. I'd keep
> "none" at the end. It might make backpatches harder.

Agreed. However a 'default' is needed in order to avoid compilation warnings.
Also note that 0002 completely does away with cases within WriteDataToArchive.

> 3) While building, I get bunch of warnings about missing cfdopen()
> prototype and pg_backup_archiver.c not knowing about cfdopen() and
> adding an implicit prototype (so I doubt it actually works).

Fixed. cfdopen() got prematurely introduced in 5e73a6048 and then got removed
in 69fb29d1af. v20 failed to properly take 69fb29d1af in consideration. Note
that cfdopen is removed in 0002 which explains why cfbot didn't complain.
 
> 4) "cfp" struct no longer wraps gzFile, but the comment was not updated.
> FWIW I'm not sure switching to "void *" is an improvement, maybe it'd be
> better to have a "union" of correct types?

Please find and updated comment and a union in place of the void *. Also
note that 0002 completely does away with cfp in favour of a new struct
CompressFileHandle. I maintained the void * there because it is used by
private methods of the compressors. 0003 contains such an example with
LZ4CompressorState.

> 5) cfopen/cfdopen are missing comments. cfopen_internal has an updated
> comment, but that's a static function while cfopen/cfdopen are the
> actual API.

Added comments to cfopen/cfdopen.

> 
> > Also please let me know if I should silently step away from it and let other people lead
> > it. I would be glad to comply either way.
> 
> 
> Please don't. I promise to take a look at this patch again.

Thank you very much.

> Thanks for doing all the work.

Thank you.

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,