Horton

Re: refactoring basebackup.c

Robert Haas <robertmhaas@gmail.com>

View thread
From: Robert Haas <robertmhaas@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: 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-11-08T16:34:50Z
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.

Attachments

On Mon, Nov 8, 2021 at 10:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > It turns out that these commits are causing failures on prairiedog.
> > Per email from Tom off-list, that's apparently because prairiedog has
> > a fussy version of tar that doesn't like it when you omit the trailing
> > NUL blocks that are supposed to be part of a tar file.
>
> FTR, prairiedog is green.  It's Noah's AIX menagerie that's complaining.

Woops.

> It's actually a little bit disturbing that we're only seeing a failure
> on that one platform, because that means that nothing else is anchoring
> us to the strict POSIX specification for tarfile format.  We knew that
> GNU tar is forgiving about missing trailing zero blocks, but apparently
> so is BSD tar.

Yeah.

> One part of me wants to add some explicit test for the trailing blocks.
> Another says, well, the *de facto* tar standard seems not to require
> the trailing blocks, never mind the letter of POSIX --- so when AIX
> dies, will anyone care anymore?  Maybe not.

FWIW, I think both of those are pretty defensible positions. Honestly,
I'm not sure how likely the bug is to recur once we fix it here,
either. The only reason this is a problem is because of the kludge of
having the server generate the entire output file except for the last
1kB. If we eliminate that behavior I don't know that this particular
problem is especially likely to come back. But adding a test isn't
stupid either, just a bit tricky to write. When I was testing locally
this morning I found that there were considerably more than 1024 zero
bytes at the end of the file because the last file it backs up is
pg_control which ends with lots of zero bytes. So it's not sufficient
to just write a test that checks for non-zero bytes in the last 1kB of
the file. What I think you'd need to do is figure out the number of
files in the archive and the sizes of each one, and based on that work
out how big the tar archive should be: 512 bytes per file or directory
or symlink plus enough extra 512 byte chunks to cover the contents of
each file plus an extra 1024 bytes at the end. That doesn't seem
particularly simple to code. We could run 'tar tvf' and parse the
output to get the number of files and their lengths, but that seems
likely to cause more portability headaches than the underlying issue.
Since pg_basebackup now has the logic to do all of this parsing
internally, we could make it complain if it receives from a v15+
server an archive trailer that is not 1024 bytes of zeroes, but that
wouldn't help with this exact problem, because the issue in this case
is when pg_basebackup decides it doesn't need to parse in the first
place. We could add a pg_basebackup option
--force-parsing-and-check-if-the-server-seems-broken, but that seems
like overkill to me. So overall I'm inclined to just do nothing about
this unless someone has a better idea how to write a reasonable test.

Anyway, here's my proposal for fixing the issue immediately before us.
0001 adds logic to pad out the unterminated tar archives, and 0002
makes the server terminate its tar archives while preserving the logic
added by 0001 for cases where we're talking to an older server. I
assume that it's best to get something committed quickly here so will
do that in ~4 hours if there are no major objections, or sooner if I
hear some enthusiastic endorsement.

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