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-20T15:03:30Z
Lists: pgsql-hackers 




------- Original Message -------
On Friday, January 20th, 2023 at 12:34 AM, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote:


> 
> 
> On 1/19/23 18:55, Tomas Vondra wrote:
> 
> > Hi,
> > 
> > On 1/19/23 17:42, gkokolatos@pm.me wrote:
> > 
> > > ...
> > > 
> > > Agreed. It was initially submitted as one patch. Then it was requested to be
> > > split up in two parts, one to expand the use of the existing API and one to
> > > replace with the new interface. Unfortunately the expansion of usage of the
> > > existing API requires some tweaking, but that is not a very good reason for
> > > the current patch set. I should have done a better job there.
> > > 
> > > Please find v22 attach which combines back 0001 and 0002. It is missing the
> > > documentation that was discussed above as I wanted to give a quick feedback.
> > > Let me know if you think that the combined version is the one to move forward
> > > with.
> > 
> > Thanks, I'll take a look.
> 
> 
> After taking a look and thinking about it a bit more, I think we should
> keep the two parts separate. I think Michael (or whoever proposed) the
> split was right, it makes the patches easier to grok.
> 

Excellent. I will attempt a better split this time round.

> 
> While reading the thread, I also noticed this:
> 
> > By the way, I think that this 0002 should drop all the default clauses
> > in the switches for the compression method so as we'd catch any
> > missing code paths with compiler warnings if a new compression method
> > is added in the future.
> 
> 
> Now I realize why there were "not yet implemented" errors for lz4/zstd
> in all the switches, and why after removing them you had to add a
> default branch.
> 
> We DON'T want a default branch, because the idea is that after adding a
> new compression algorithm, we get warnings about switches not handling
> it correctly.
> 
> So I guess we should walk back this change too :-( It's probably easier
> to go back to v20 from January 16, and redo the couple remaining things
> I commented on.
> 

Sure.

> 
> FWIW I think this is a hint that adding LZ4/ZSTD options, in 5e73a6048,
> but without implementation, was not a great idea. It mostly defeats the
> idea of getting the compiler warnings - all the places already handle
> PG_COMPRESSION_LZ4/PG_COMPRESSION_ZSTD by throwing a pg_fatal. So you'd
> have to grep for the options, inspect all the places or something like
> that anyway. The warnings would only work for entirely new methods.
> 
> However, I now also realize the compressor API in 0002 replaces all of
> this with calls to a generic API callback, so trying to improve this was
> pretty silly from me.

I can try to do a better job at splitting things up.

> 
> Please, fix the couple remaining details in v20, add the docs for the
> callbacks, and I'll try to polish it and get it committed.

Excellent. Allow me an attempt to polish and expect a new version soon.

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,