Horton

Re: refactoring basebackup.c

Andres Freund <andres@anarazel.de>

View thread
From: Andres Freund <andres@anarazel.de>
To: Robert Haas <robertmhaas@gmail.com>
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2020-05-08T21:27:45Z
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.

Hi,

On 2020-05-08 16:53:09 -0400, Robert Haas wrote:
> They represent closely-related concepts, so much so that I initially
> thought we could get by with just one new abstraction layer. I found
> on experimentation that this did not work well, so I split it up into
> two and that worked a lot better. The distinction is this: a bbsink is
> something to which you can send a bunch of archives -- currently, each
> would be a tarfile -- and also a backup manifest. A bbarchiver is
> something to which you send every file in the data directory
> individually, or at least the ones that are getting backed up, plus
> any that are being injected into the backup (e.g. the backup_label).
> Commonly, a bbsink will do something with the data and then forward it
> to a subsequent bbsink, or a bbarchiver will do something with the
> data and then forward it to a subsequent bbarchiver or bbsink. For
> example, there's a bbarchiver_tar object which, like any bbarchiver,
> sees all the files and their contents as input. The output is a
> tarfile, which gets send to a bbsink. As things stand in the patch set
> now, the tar archives are ultimately sent to the "libpq" bbsink, which
> sends them to the client.

Hm.

I wonder if there's cases where recursively forwarding like this will
cause noticable performance effects. The only operation that seems
frequent enough to potentially be noticable would be "chunks" of the
file. So perhaps it'd be good to make sure we read in large enough
chunks?

> 0010 invents two new bbarchivers, a tar bbarchiver and a tarsize
> bbarchiver, and refactors basebackup.c to make use of them. The tar
> bbarchiver puts the files it sees into tar archives and forwards the
> resulting archives to a bbsink. The tarsize bbarchiver is used to
> support the PROGRESS option to the BASE_BACKUP command. It just
> estimates the size of the backup by summing up the file sizes without
> reading them. This approach is good for a couple of reasons. First,
> without something like this, it's impossible to keep basebackup.c from
> knowing something about the tar format, because the PROGRESS option
> doesn't just figure out how big the files to be backed up are: it
> figures out how big it thinks the archives will be, and that involves
> tar-specific considerations.

ISTM that it's not actually good to have the progress calculations
include the tar overhead. As you say:

> This area needs more work, as the whole idea of measuring progress by
> estimating the archive size is going to break down as soon as
> server-side compression is in the picture.

This, to me, indicates that we should measure the progress solely based
on how much of the "source" data was processed. The overhead of tar, the
reduction due to compression, shouldn't be included.


> What do you all think?

I've not though enough about the specifics, but I think it looks like
it's going roughly in a better direction.

One thing I wonder about is how stateful the interface is. Archivers
will pretty much always track which file is currently open etc. Somehow
such a repeating state machine seems a bit ugly - but I don't really
have a better answer.

Greetings,

Andres Freund