Horton

Re: refactoring basebackup.c (zstd)

Robert Haas <robertmhaas@gmail.com>

View thread
From: Robert Haas <robertmhaas@gmail.com>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Jeevan Ladhe <jeevanladhe.os@gmail.com>, "Shinoda, Noriyoshi (PN Japan FSIP)" <noriyoshi.shinoda@hpe.com>, Dipesh Pandit <dipesh.pandit@gmail.com>, Abhijit Menon-Sen <ams@toroid.org>, Dmitry Dolgov <9erthalion6@gmail.com>, Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>, Mark Dilger <mark.dilger@enterprisedb.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>, tushar <tushar.ahuja@enterprisedb.com>
Date: 2022-02-16T15:51:36Z
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, Feb 15, 2022 at 12:59 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> There's superfluous changes to ./configure unrelated to the changes in
> configure.ac.  Probably because you're using a different version of autotools,
> or a vendor's patched copy.  You can remove the changes with git checkout -p or
> similar.

I noticed this already and fixed it in the version of the patch I
posted on the other thread.

> +++ b/src/backend/replication/basebackup_zstd.c
> +bbsink *
> +bbsink_zstd_new(bbsink *next, int compresslevel)
> +{
> +#ifndef HAVE_LIBZSTD
> +       ereport(ERROR,
> +                       (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +                        errmsg("zstd compression is not supported by this build")));
> +#else
>
> This should have an return; like what's added by 71cbbbbe8 and 302612a6c.
> Also, the parens() around errcode aren't needed since last year.

The parens are still acceptable style, though. The return I guess is needed.

> +       bbsink_zstd *sink;
> +
> +       Assert(next != NULL);
> +       Assert(compresslevel >= 0 && compresslevel <= 22);
> +
> +       if (compresslevel < 0 || compresslevel > 22)
> +               ereport(ERROR,
>
> This looks like dead code in assert builds.
> If it's unreachable, it can be elog().

Actually, the right thing to do here is remove the assert, I think. I
don't believe that the code is unreachable. If I'm wrong and it is
unreachable then the test-and-ereport should be removed.

> + * Compress the input data to the output buffer until we run out of input
> + * data. Each time the output buffer falls below the compression bound for
> + * the input buffer, invoke the archive_contents() method for then next sink.
>
> *the next sink ?

Yeah.

> Does anyone plan to include this for pg15 ?  If so, I think at least the WAL
> compression should have support added too.  I'd plan to rebase Michael's patch.
> https://www.postgresql.org/message-id/YNqWd2GSMrnqWIfx@paquier.xyz

Yes, I'd like to get this into PG15. It's very similar to the LZ4
compression support which was already committed, so it feels like
finishing it up and including it in the release makes a lot of sense.
I'm not against the idea of using ZSTD in other places where it makes
sense as well, but I think that's a separate issue from this patch. As
far as I'm concerned, either basebackup compression with ZSTD or WAL
compression with ZSTD could be committed even if the other is not, and
I plan to spend my time on this project, not that project. However, if
you're saying you want to work on the WAL compression stuff, I've got
no objection to that.

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