Horton

Re: refactoring basebackup.c

Robert Haas <robertmhaas@gmail.com>

View thread
From: Robert Haas <robertmhaas@gmail.com>
To: Sergei Kornilov <sk@zsrv.org>
Cc: Dilip Kumar <dilipbalaut@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: 2021-09-21T15:25:03Z
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, Sep 14, 2021 at 11:30 AM Sergei Kornilov <sk@zsrv.org> wrote:
> I found that in 0001 you propose to rename few options. Probably we could rename another option for clarify? I think FAST (it's about some bw limits?) and WAIT (wait for what? checkpoint?) option names are confusing.
> Could we replace FAST with "CHECKPOINT [fast|spread]" and WAIT to WAIT_WAL_ARCHIVED? I think such names would be more descriptive.

I think CHECKPOINT { 'spread' | 'fast' } is probably a good idea; the
options logic for pg_basebackup uses the same convention, and if
somebody ever wanted to introduce a third kind of checkpoint, it would
be a lot easier if you could just make pg_basebackup -cbanana send
CHECKPOINT 'banana' to the server. I don't think renaming WAIT ->
WAIT_WAL_ARCHIVED has much value. The replication grammar isn't really
intended to be consumed directly by end-users, and it's also not clear
that WAIT_WAL_ARCHIVED would attract more support than any of 5 or 10
other possible variants. I'd rather leave it alone.

> -               if (PQserverVersion(conn) >= 100000)
> -                       /* pg_recvlogical doesn't use an exported snapshot, so suppress */
> -                       appendPQExpBufferStr(query, " NOEXPORT_SNAPSHOT");
> +               /* pg_recvlogical doesn't use an exported snapshot, so suppress */
> +               if (use_new_option_syntax)
> +                       AppendStringCommandOption(query, use_new_option_syntax,
> +                                                                          "SNAPSHOT", "nothing");
> +               else
> +                       AppendPlainCommandOption(query, use_new_option_syntax,
> +                                                                        "NOEXPORT_SNAPSHOT");
>
> In 0002, it looks like condition for 9.x releases was lost?

Good catch, thanks.

I'll post an updated version of these two patches on the thread
dedicated to those two patches, which can be found at
http://postgr.es/m/CA+Tgmob2cbCPNbqGoixp0J6aib0p00XZerswGZwx-5G=0M+BMA@mail.gmail.com

> Also my gcc version 8.3.0 is not happy with v5-0007-Support-base-backup-targets.patch and produces:
>
> basebackup.c: In function ‘parse_basebackup_options’:
> basebackup.c:970:7: error: ‘target_str’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>        errmsg("target '%s' does not accept a target detail",
>        ^~~~~~

OK, I'll fix that. Thanks.

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