Re: refactoring basebackup.c

On Fri, Nov 5, 2021 at 11:50 AM Robert Haas <robertmhaas@gmail.com> wrote:
> I went ahead and committed 0001 and 0002, but got nervous about
> proceeding with 0003.

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. So how did this
get broken?

It turns out that in the current state of the world, the server sends
an almost-tarfile to the client. What I mean by an almost-tarfile is
that it sends something that looks like a valid tarfile except that
the two blocks of trailing NUL bytes are omitted. Prior to these
patches, that was a very strategic omission, because the pg_basebackup
code wants to edit the tar files, and it wasn't smart enough to parse
them, so it just received all the data from the server, then added any
members that it wanted to add (e.g. recovery.signal) and then added
the terminator itself. I would classify this as an ugly hack, but it
worked. With these changes, the client is now capable of really
parsing a tarfile, so it would have no problem injecting new files
into the archive whether or not the server terminates it properly. It
also has no problem adding the two blocks of terminating NUL bytes if
the server omits them, but not otherwise. All in all, it's
significantly smarter code.

However, I also set things up so that the client doesn't bother
parsing the tar file from the server if it's not doing anything that
requires editing the tar file on the fly. That saves some overhead,
and it's also important for the rest of the patch set, which wants to
make it so that the server could send us something besides a tarfile,
like maybe a .tar.gz. We can't just have a convention of adding 1024
NUL bytes to any file the server sends us unless what the server sends
us is always and precisely an unterminated tarfile.  Unfortunately,
that means that in the case where the tar parsing logic isn't used,
the tar file ends up with the proper terminator. Because most 'tar'
implementations are happy to ignore that defect, the tests pass on my
machine, but not on prairiedog. I think I realized this problem at
some point during the development process of this patch, but then I
forgot about it again and ended up committing something that has a
problem of which, at some earlier point in time, I had been entirely
aware. Oops.

It's tempting to try to fix this problem by changing the server so
that it properly terminates the tar files it sends to the client.
Honestly, I don't know how we ever thought it was OK to design a
protocol for base backups that involved the server sending something
that is almost but not quite a valid tarfile. However, that's not
quite good enough, because pg_basebackup is supposed to be backward
compatible, so we'd still have the same problem if a new version of
pg_basebackup were used with an old server. So what I'm inclined to do
is fix both the server and pg_basebackup. On the server side, properly
terminate the tarfile. On the client side, if we're talking to a
pre-v15 server and don't need to parse the tarfile, blindly add 1024
NUL bytes at the end.

I think I can get patches for this done today. Please let me know ASAP
if you have objections to this line of attack.

Thanks,

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