Horton

Re: refactoring basebackup.c

Robert Haas <robertmhaas@gmail.com>

View thread
From: Robert Haas <robertmhaas@gmail.com>
To: Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>
Cc: Mark Dilger <mark.dilger@enterprisedb.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>, tushar <tushar.ahuja@enterprisedb.com>
Date: 2021-11-02T14:32:35Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Document BaseBackupSync and BaseBackupWrite wait events.

  2. Support long distance matching for zstd compression

  3. Fix possible NULL-pointer-deference in backup_compression.c.

  4. Allow parallel zstd compression when taking a base backup.

  5. Make PostgreSQL::Test::Cluster::run_log() return a useful value.

  6. Fix a few goofs in new backup compression code.

  7. Replace BASE_BACKUP COMPRESSION_LEVEL option with COMPRESSION_DETAIL.

  8. Add 'basebackup_to_shell' contrib module.

  9. Allow extensions to add new backup targets.

  10. Change HAVE_LIBLZ4 and HAVE_LIBZSTD tests to USE_LZ4 and USE_ZSTD.

  11. pg_basebackup: Clean up some bogus file extension tests.

  12. pg_basebackup: Avoid unclean failure with server-compression and -D -.

  13. Fix LZ4 tests for remaining buffer space.

  14. Add support for zstd base backup compression.

  15. pg_basebackup: Allow client-side LZ4 (de)compression.

  16. Add suport for server-side LZ4 base backup compression.

  17. Add min() and max() aggregates for xid8.

  18. Remove superfluous variable.

  19. pg_basebackup: Cleaner handling when compression is multiply specified.

  20. Allow server-side compression to be used with -Fp.

  21. pg_basebackup: Fix a couple of recently-introduced bugs.

  22. Tidy up a few cosmetic issues related to pg_basebackup.

  23. Server-side gzip compression.

  24. Unbreak pg_basebackup/t/010_pg_basebackup.pl on msys

  25. Suppress variable-set-but-not-used warning from clang 13.

  26. Extend the options of pg_basebackup to control compression

  27. Support base backup targets.

  28. Modify pg_basebackup to use a new COPY subprotocol for base backups.

  29. Document that tar archives are now properly terminated.

  30. Fix thinko in bbsink_throttle_manifest_contents.

  31. Have the server properly terminate tar archives.

  32. Minimal fix for unterminated tar archive problem.

  33. Introduce 'bbstreamer' abstraction to modularize pg_basebackup.

  34. Introduce 'bbsink' abstraction to modularize base backup code.

  35. Refactor basebackup.c's _tarWriteDir() function.

  36. Flexible options for CREATE_REPLICATION_SLOT.

  37. Flexible options for BASE_BACKUP.

On Tue, Nov 2, 2021 at 7:53 AM Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
> I have implemented the cleanup callback bbsink_lz4_cleanup() in the attached patch.
>
> Please have a look and let me know of any comments.

Looks pretty good. I think you should work on stuff like documentation
and tests, and I need to do some work on that stuff, too. Also, I
think you should try to figure out how to support different
compression levels. For gzip, I did that by making gzip1..gzip9
possible compression settings. But that might not have been the right
idea because something like lz43 to mean lz4 at level 3 would be
confusing. Also, for the lz4 command line utility, there's not only
"lz4 -3" which means LZ4 with level 3 compression, but also "lz4
--fast=3" which selects "ultra-fast compression level 3" rather than
regular old level 3. And apparently LZ4 levels go up to 12 rather than
just 9 like gzip. I'm thinking maybe we should go with something like
"gzip@9" rather than just "gzip9" to mean gzip with compression level
9, and then things like "lz4@3" or "lz4@fast3" would select either the
regular compression levels or the ultra-fast compression levels.

Meanwhile, I think it's probably OK for me to go ahead and commit
0001-0003 from my patches at this point, since it seems we have pretty
good evidence that the abstraction basically works, and there doesn't
seem to be any value in holding off and maybe having to do a bunch
more rebasing. We may also want to look into making -Fp work with
--server-compression, which would require pg_basebackup to know how to
decompress. I'm actually not sure if this is worthwhile; you'd need to
have a network connection slow enough that it's worth spending a lot
of CPU time compressing on the server and decompressing on the client
to make up for the cost of network transfer. But some people might
have that case. It might make it easier to test this, too, since we
probably can't rely on having an LZ4 binary installed. Another thing
that you probably need to investigate is also supporting client-side
LZ4 compression. I think that is probably a really desirable addition
to your patch set, since people might find it odd if that were
exclusively a server-side option. Hopefully it's not that much work.

One minor nitpick in terms of the code:

+ mysink->bytes_written = mysink->bytes_written + headerSize;

I would use += here.

-- 
Robert Haas
EDB: http://www.enterprisedb.com