tar-related code in PostgreSQL
Hi,
It has come to my attention that PostgreSQL has a bunch of code to
read and write 'tar' archives and it's kind of a mess. Attached are
two patches. The second one was written first, and does some modest
cleanups, most notably replacing the use of the constants 512 and the
formula (x + 511) & ~511 - x in lotsa places with a new constant
TAR_BLOCK_SIZE and a new static line function
tarPaddingBytesRequired(). In developing that patch, I found a bug, so
the first patch (which was written second) fixes it. The problem is
here:
if (state.is_recovery_guc_supported)
{
tarCreateHeader(header, "standby.signal", NULL,
0, /* zero-length file */
pg_file_create_mode, 04000, 02000,
time(NULL));
writeTarData(&state, header, sizeof(header));
writeTarData(&state, zerobuf, 511);
}
We have similar code in many places -- because evidently nobody
thought it would be a good idea to have all the logic for reading and
writing tarfiles in a centralized location rather than having many
copies of it -- and typically it's written to pad the block out to a
multiple of 512 bytes. But here, the file is 0 bytes long, and then we
add 511 zero bytes. This results in a tarfile whose length is not a
multiple of the TAR block size:
[rhaas ~]$ pg_basebackup -D pgbackup -Ft && ls -l pgbackup
total 80288
-rw------- 1 rhaas staff 135255 Apr 24 11:04 backup_manifest
-rw------- 1 rhaas staff 24183296 Apr 24 11:04 base.tar
-rw------- 1 rhaas staff 16778752 Apr 24 11:04 pg_wal.tar
[rhaas ~]$ rm -rf pgbackup
[rhaas ~]$ pg_basebackup -D pgbackup -Ft -R && ls -l pgbackup
total 80288
-rw------- 1 rhaas staff 135255 Apr 24 11:04 backup_manifest
-rw------- 1 rhaas staff 24184319 Apr 24 11:04 base.tar
-rw------- 1 rhaas staff 16778752 Apr 24 11:04 pg_wal.tar
[rhaas ~]$ perl -e 'print $_ % 512, "\n" for qw(24183296 24184319 16778752);'
0
511
0
That seems bad. At first, I thought maybe the problem was the fact
that we were adding 511 zero bytes here instead of 512, but then I
realized the real problem is that we shouldn't be adding any zero
bytes at all. A zero-byte file is already padded out to a multiple of
512, because 512 * 0 = 0. The problem happens not to have any adverse
consequences in this case because this is the last thing that gets put
into the tar file, and the end-of-tar-file marker is 1024 zero bytes,
so the extra 511 zero bytes we're adding here get interpreted as the
beginning of the end-of-file marker, and the first 513 bytes of what
we intended as the actual end of file marker get interpreted as the
rest of it. Then there are 511 bytes of garbage zeros at the end but
my version of tar, at least, does not care.
However, it's possible to make it blow up pretty good with a slightly
different test case, because there's one case in master where we
insert one extra file -- backup_manifest -- into the tar file AFTER we
insert standby.signal. That case is when we're writing the output to
stdout:
[rhaas ~]$ pg_basebackup -D - -Ft -Xnone -R > everything.tar
NOTICE: WAL archiving is not enabled; you must ensure that all
required WAL segments are copied through other means to complete the
backup
[rhaas ~]$ tar tf everything.tar | grep manifest
tar: Damaged tar archive
tar: Retrying...
tar: Damaged tar archive
tar: Retrying...
(it repeats this ~100 times and then exits)
If I change the offending writeTarData(&state, zerobuf, 511) to
writeTarData(&state, zerobuf, 512), then the complaint about a damaged
archive goes away, but the backup_manifest file doesn't appear to be
included in the archive, because apparently one spurious 512-byte
block of zeroes is enough to make my version of tar think it's hit the
end of the archive:
[rhaas ~]$ tar tf everything.tar | grep manifest
[rhaas ~]$
If I remove the offending writeTarData(&state, zerobuf, 511) line
entirely - which I believe to the correct fix - then it works as
expected:
[rhaas ~]$ tar tf everything.tar | grep manifest
backup_manifest
This problem appears to have been introduced by commit
2dedf4d9a899b36d1a8ed29be5efbd1b31a8fe85, "Integrate recovery.conf
into postgresql.conf". The code has been substantially modified twice
since then, but those modifications seem to have just preserved the
bug first introduced in that commit.
I tentatively propose to do the following:
1. Commit 0001, which removes the 511 bytes of bogus padding and thus
fixes the bug, to master in the near future.
2. Possibly back-patch 0001 to v12, where the bug first appeared. I'm
not sure this is strictly necessary, because in v12, standby.signal is
always the very last thing in the tarfile, so there shouldn't be an
issue unless somebody has a version of tar that cares about the 511
bytes of trailing garbage. I will do this if people think it's a good
idea; otherwise I'll skip it.
3. Commit 0002, the aforementioned cleanup patch, to master, either
immediately if people are OK with that, or else after we branch. I am
inclined to regard the widespread use of the arbitrary constants 512
and 511 as something of a hazard that ought to be corrected sooner
rather than later, but someone could not-unreasonably take the view
that it's unnecessary tinkering post-feature freeze.
Long term, it might be wiser to either switch to using a real
archiving library rather than a bunch of hand-rolled code, or at the
very least centralize things better so that it's not so easy to make
mistakes of this type. However, I don't see that as a reasonable
argument against either of these patches.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
0002-Assorted-cleanup-of-tar-related-code.patchapplication/octet-stream; name=0002-Assorted-cleanup-of-tar-related-code.patchDownload
From f9b84f54654400c0d40d2692ed45ee123d3818f6 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Fri, 24 Apr 2020 10:38:10 -0400
Subject: [PATCH 2/2] Assorted cleanup of tar-related code.
Introduce TAR_BLOCK_SIZE and replace many instances of 512 with
the new constant. Introduce function tarPaddingBytesRequired
and use it to replace numerous repetitions of (x + 511) & ~511.
Add preprocessor guards against multiple inclusion to pgtar.h.
Reformat the prototype for tarCreateHeader so it doesn't extend
beyond 80 characters.
---
src/backend/replication/basebackup.c | 29 ++++++++++++------
src/bin/pg_basebackup/pg_basebackup.c | 44 +++++++++++++--------------
src/bin/pg_basebackup/walmethods.c | 25 +++++++++------
src/bin/pg_dump/pg_backup_tar.c | 30 +++++++++---------
src/include/pgtar.h | 23 ++++++++++++--
5 files changed, 93 insertions(+), 58 deletions(-)
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index fbdc28ec39..8b332d110d 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -665,7 +665,11 @@ perform_base_backup(basebackup_options *opt)
errmsg("unexpected WAL file size \"%s\"", walFileName)));
}
- /* wal_segment_size is a multiple of 512, so no need for padding */
+ /*
+ * wal_segment_size is a multiple of TAR_BLOCK_SIZE, so no need
+ * for padding.
+ */
+ Assert(wal_segment_size % TAR_BLOCK_SIZE == 0);
FreeFile(fp);
@@ -1118,11 +1122,11 @@ sendFileWithContent(const char *filename, const char *content,
pq_putmessage('d', content, len);
update_basebackup_progress(len);
- /* Pad to 512 byte boundary, per tar format requirements */
- pad = ((len + 511) & ~511) - len;
+ /* Pad to a multiple of the tar block size. */
+ pad = tarPaddingBytesRequired(len);
if (pad > 0)
{
- char buf[512];
+ char buf[TAR_BLOCK_SIZE];
MemSet(buf, 0, pad);
pq_putmessage('d', buf, pad);
@@ -1491,9 +1495,14 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces,
if (sent || sizeonly)
{
- /* Add size, rounded up to 512byte block */
- size += ((statbuf.st_size + 511) & ~511);
- size += 512; /* Size of the header of the file */
+ /* Add size. */
+ size += statbuf.st_size;
+
+ /* Pad to a multiple of the tar block size. */
+ size += tarPaddingBytesRequired(statbuf.st_size);
+
+ /* Size of the header for the file. */
+ size += TAR_BLOCK_SIZE;
}
}
else
@@ -1784,11 +1793,11 @@ sendFile(const char *readfilename, const char *tarfilename,
}
/*
- * Pad to 512 byte boundary, per tar format requirements. (This small
+ * Pad to a block boundary, per tar format requirements. (This small
* piece of data is probably not worth throttling, and is not checksummed
* because it's not actually part of the file.)
*/
- pad = ((len + 511) & ~511) - len;
+ pad = tarPaddingBytesRequired(len);
if (pad > 0)
{
MemSet(buf, 0, pad);
@@ -1822,7 +1831,7 @@ static int64
_tarWriteHeader(const char *filename, const char *linktarget,
struct stat *statbuf, bool sizeonly)
{
- char h[512];
+ char h[TAR_BLOCK_SIZE];
enum tarError rc;
if (!sizeonly)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index c1d3d8624b..0b7660d15b 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -62,7 +62,7 @@ typedef struct WriteTarState
int tablespacenum;
char filename[MAXPGPATH];
FILE *tarfile;
- char tarhdr[512];
+ char tarhdr[TAR_BLOCK_SIZE];
bool basetablespace;
bool in_tarhdr;
bool skip_file;
@@ -1024,7 +1024,7 @@ writeTarData(WriteTarState *state, char *buf, int r)
static void
ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
{
- char zerobuf[1024];
+ char zerobuf[TAR_BLOCK_SIZE * 2];
WriteTarState state;
memset(&state, 0, sizeof(state));
@@ -1168,7 +1168,7 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
if (state.basetablespace && writerecoveryconf)
{
- char header[512];
+ char header[TAR_BLOCK_SIZE];
/*
* If postgresql.auto.conf has not been found in the streamed data,
@@ -1187,7 +1187,7 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
pg_file_create_mode, 04000, 02000,
time(NULL));
- padding = ((recoveryconfcontents->len + 511) & ~511) - recoveryconfcontents->len;
+ padding = tarPaddingBytesRequired(recoveryconfcontents->len);
writeTarData(&state, header, sizeof(header));
writeTarData(&state, recoveryconfcontents->data,
@@ -1223,7 +1223,7 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
*/
if (strcmp(basedir, "-") == 0 && manifest)
{
- char header[512];
+ char header[TAR_BLOCK_SIZE];
PQExpBufferData buf;
initPQExpBuffer(&buf);
@@ -1241,7 +1241,7 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
termPQExpBuffer(&buf);
}
- /* 2 * 512 bytes empty data at end of file */
+ /* 2 * TAR_BLOCK_SIZE bytes empty data at end of file */
writeTarData(&state, zerobuf, sizeof(zerobuf));
#ifdef HAVE_LIBZ
@@ -1302,9 +1302,9 @@ ReceiveTarCopyChunk(size_t r, char *copybuf, void *callback_data)
*
* To do this, we have to process the individual files inside the TAR
* stream. The stream consists of a header and zero or more chunks,
- * all 512 bytes long. The stream from the server is broken up into
- * smaller pieces, so we have to track the size of the files to find
- * the next header structure.
+ * each with a length equal to TAR_BLOCK_SIZE. The stream from the
+ * server is broken up into smaller pieces, so we have to track the
+ * size of the files to find the next header structure.
*/
int rr = r;
int pos = 0;
@@ -1317,17 +1317,17 @@ ReceiveTarCopyChunk(size_t r, char *copybuf, void *callback_data)
* We're currently reading a header structure inside the TAR
* stream, i.e. the file metadata.
*/
- if (state->tarhdrsz < 512)
+ if (state->tarhdrsz < TAR_BLOCK_SIZE)
{
/*
* Copy the header structure into tarhdr in case the
- * header is not aligned to 512 bytes or it's not returned
+ * header is not aligned properly or it's not returned
* in whole by the last PQgetCopyData call.
*/
int hdrleft;
int bytes2copy;
- hdrleft = 512 - state->tarhdrsz;
+ hdrleft = TAR_BLOCK_SIZE - state->tarhdrsz;
bytes2copy = (rr > hdrleft ? hdrleft : rr);
memcpy(&state->tarhdr[state->tarhdrsz], copybuf + pos,
@@ -1360,14 +1360,14 @@ ReceiveTarCopyChunk(size_t r, char *copybuf, void *callback_data)
state->filesz = read_tar_number(&state->tarhdr[124], 12);
state->file_padding_len =
- ((state->filesz + 511) & ~511) - state->filesz;
+ tarPaddingBytesRequired(state->filesz);
if (state->is_recovery_guc_supported &&
state->is_postgresql_auto_conf &&
writerecoveryconf)
{
/* replace tar header */
- char header[512];
+ char header[TAR_BLOCK_SIZE];
tarCreateHeader(header, "postgresql.auto.conf", NULL,
state->filesz + recoveryconfcontents->len,
@@ -1387,7 +1387,7 @@ ReceiveTarCopyChunk(size_t r, char *copybuf, void *callback_data)
* If we're not skipping the file, write the tar
* header unmodified.
*/
- writeTarData(state, state->tarhdr, 512);
+ writeTarData(state, state->tarhdr, TAR_BLOCK_SIZE);
}
}
@@ -1424,15 +1424,15 @@ ReceiveTarCopyChunk(size_t r, char *copybuf, void *callback_data)
int padding;
int tailsize;
- tailsize = (512 - state->file_padding_len) + recoveryconfcontents->len;
- padding = ((tailsize + 511) & ~511) - tailsize;
+ tailsize = (TAR_BLOCK_SIZE - state->file_padding_len) + recoveryconfcontents->len;
+ padding = tarPaddingBytesRequired(tailsize);
writeTarData(state, recoveryconfcontents->data,
recoveryconfcontents->len);
if (padding)
{
- char zerobuf[512];
+ char zerobuf[TAR_BLOCK_SIZE];
MemSet(zerobuf, 0, sizeof(zerobuf));
writeTarData(state, zerobuf, padding);
@@ -1550,12 +1550,12 @@ ReceiveTarAndUnpackCopyChunk(size_t r, char *copybuf, void *callback_data)
/*
* No current file, so this must be the header for a new file
*/
- if (r != 512)
+ if (r != TAR_BLOCK_SIZE)
{
pg_log_error("invalid tar block header size: %zu", r);
exit(1);
}
- totaldone += 512;
+ totaldone += TAR_BLOCK_SIZE;
state->current_len_left = read_tar_number(©buf[124], 12);
@@ -1565,10 +1565,10 @@ ReceiveTarAndUnpackCopyChunk(size_t r, char *copybuf, void *callback_data)
#endif
/*
- * All files are padded up to 512 bytes
+ * All files are padded up to a multiple of TAR_BLOCK_SIZE
*/
state->current_padding =
- ((state->current_len_left + 511) & ~511) - state->current_len_left;
+ tarPaddingBytesRequired(state->current_len_left);
/*
* First part of header is zero terminated filename
diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
index ecff08740c..bd1947d623 100644
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -386,7 +386,7 @@ typedef struct TarMethodFile
{
off_t ofs_start; /* Where does the *header* for this file start */
off_t currpos;
- char header[512];
+ char header[TAR_BLOCK_SIZE];
char *pathname;
size_t pad_to_size;
} TarMethodFile;
@@ -625,7 +625,8 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
if (!tar_data->compression)
{
errno = 0;
- if (write(tar_data->fd, tar_data->currentfile->header, 512) != 512)
+ if (write(tar_data->fd, tar_data->currentfile->header,
+ TAR_BLOCK_SIZE) != TAR_BLOCK_SIZE)
{
save_errno = errno;
pg_free(tar_data->currentfile);
@@ -639,7 +640,8 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
else
{
/* Write header through the zlib APIs but with no compression */
- if (!tar_write_compressed_data(tar_data->currentfile->header, 512, true))
+ if (!tar_write_compressed_data(tar_data->currentfile->header,
+ TAR_BLOCK_SIZE, true))
return NULL;
/* Re-enable compression for the rest of the file */
@@ -665,7 +667,9 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
/* Uncompressed, so pad now */
tar_write_padding_data(tar_data->currentfile, pad_to_size);
/* Seek back to start */
- if (lseek(tar_data->fd, tar_data->currentfile->ofs_start + 512, SEEK_SET) != tar_data->currentfile->ofs_start + 512)
+ if (lseek(tar_data->fd,
+ tar_data->currentfile->ofs_start + TAR_BLOCK_SIZE,
+ SEEK_SET) != tar_data->currentfile->ofs_start + TAR_BLOCK_SIZE)
return NULL;
tar_data->currentfile->currpos = 0;
@@ -778,14 +782,14 @@ tar_close(Walfile f, WalCloseMethod method)
}
/*
- * Get the size of the file, and pad the current data up to the nearest
- * 512 byte boundary.
+ * Get the size of the file, and pad out to a multiple of the tar block
+ * size.
*/
filesize = tar_get_current_pos(f);
- padding = ((filesize + 511) & ~511) - filesize;
+ padding = tarPaddingBytesRequired(filesize);
if (padding)
{
- char zerobuf[512];
+ char zerobuf[TAR_BLOCK_SIZE];
MemSet(zerobuf, 0, padding);
if (tar_write(f, zerobuf, padding) != padding)
@@ -826,7 +830,7 @@ tar_close(Walfile f, WalCloseMethod method)
if (!tar_data->compression)
{
errno = 0;
- if (write(tar_data->fd, tf->header, 512) != 512)
+ if (write(tar_data->fd, tf->header, TAR_BLOCK_SIZE) != TAR_BLOCK_SIZE)
{
/* if write didn't set errno, assume problem is no disk space */
if (errno == 0)
@@ -845,7 +849,8 @@ tar_close(Walfile f, WalCloseMethod method)
}
/* Overwrite the header, assuming the size will be the same */
- if (!tar_write_compressed_data(tar_data->currentfile->header, 512, true))
+ if (!tar_write_compressed_data(tar_data->currentfile->header,
+ TAR_BLOCK_SIZE, true))
return -1;
/* Turn compression back on */
diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c
index 775118f297..d5ed7cce68 100644
--- a/src/bin/pg_dump/pg_backup_tar.c
+++ b/src/bin/pg_dump/pg_backup_tar.c
@@ -893,7 +893,7 @@ _CloseArchive(ArchiveHandle *AH)
/*
* EOF marker for tar files is two blocks of NULLs.
*/
- for (i = 0; i < 512 * 2; i++)
+ for (i = 0; i < TAR_BLOCK_SIZE * 2; i++)
{
if (fputc(0, ctx->tarFH) == EOF)
WRITE_ERROR_EXIT;
@@ -1113,7 +1113,7 @@ _tarAddFile(ArchiveHandle *AH, TAR_MEMBER *th)
buf1, buf2);
}
- pad = ((len + 511) & ~511) - len;
+ pad = tarPaddingBytesRequired(len);
for (i = 0; i < pad; i++)
{
if (fputc('\0', th->tarFH) == EOF)
@@ -1130,7 +1130,7 @@ _tarPositionTo(ArchiveHandle *AH, const char *filename)
lclContext *ctx = (lclContext *) AH->formatData;
TAR_MEMBER *th = pg_malloc0(sizeof(TAR_MEMBER));
char c;
- char header[512];
+ char header[TAR_BLOCK_SIZE];
size_t i,
len,
blks;
@@ -1189,17 +1189,19 @@ _tarPositionTo(ArchiveHandle *AH, const char *filename)
th->targetFile, filename);
/* Header doesn't match, so read to next header */
- len = ((th->fileLen + 511) & ~511); /* Padded length */
- blks = len >> 9; /* # of 512 byte blocks */
+ len = th->fileLen;
+ len += tarPaddingBytesRequired(th->fileLen);
+ blks = len / TAR_BLOCK_SIZE; /* # of tar blocks */
for (i = 0; i < blks; i++)
- _tarReadRaw(AH, &header[0], 512, NULL, ctx->tarFH);
+ _tarReadRaw(AH, &header[0], TAR_BLOCK_SIZE, NULL, ctx->tarFH);
if (!_tarGetHeader(AH, th))
fatal("could not find header for file \"%s\" in tar archive", filename);
}
- ctx->tarNextMember = ctx->tarFHpos + ((th->fileLen + 511) & ~511);
+ ctx->tarNextMember = ctx->tarFHpos + th->fileLen
+ + tarPaddingBytesRequired(th->fileLen);
th->pos = 0;
return th;
@@ -1210,7 +1212,7 @@ static int
_tarGetHeader(ArchiveHandle *AH, TAR_MEMBER *th)
{
lclContext *ctx = (lclContext *) AH->formatData;
- char h[512];
+ char h[TAR_BLOCK_SIZE];
char tag[100 + 1];
int sum,
chk;
@@ -1223,12 +1225,12 @@ _tarGetHeader(ArchiveHandle *AH, TAR_MEMBER *th)
/* Save the pos for reporting purposes */
hPos = ctx->tarFHpos;
- /* Read a 512 byte block, return EOF, exit if short */
- len = _tarReadRaw(AH, h, 512, NULL, ctx->tarFH);
+ /* Read the next tar block, return EOF, exit if short */
+ len = _tarReadRaw(AH, h, TAR_BLOCK_SIZE, NULL, ctx->tarFH);
if (len == 0) /* EOF */
return 0;
- if (len != 512)
+ if (len != TAR_BLOCK_SIZE)
fatal(ngettext("incomplete tar header found (%lu byte)",
"incomplete tar header found (%lu bytes)",
len),
@@ -1248,7 +1250,7 @@ _tarGetHeader(ArchiveHandle *AH, TAR_MEMBER *th)
{
int i;
- for (i = 0; i < 512; i++)
+ for (i = 0; i < TAR_BLOCK_SIZE; i++)
{
if (h[i] != 0)
{
@@ -1294,12 +1296,12 @@ _tarGetHeader(ArchiveHandle *AH, TAR_MEMBER *th)
static void
_tarWriteHeader(TAR_MEMBER *th)
{
- char h[512];
+ char h[TAR_BLOCK_SIZE];
tarCreateHeader(h, th->targetFile, NULL, th->fileLen,
0600, 04000, 02000, time(NULL));
/* Now write the completed header. */
- if (fwrite(h, 1, 512, th->tarFH) != 512)
+ if (fwrite(h, 1, TAR_BLOCK_SIZE, th->tarFH) != TAR_BLOCK_SIZE)
WRITE_ERROR_EXIT;
}
diff --git a/src/include/pgtar.h b/src/include/pgtar.h
index 0a875903a7..0f08dc0c2c 100644
--- a/src/include/pgtar.h
+++ b/src/include/pgtar.h
@@ -11,6 +11,10 @@
*
*-------------------------------------------------------------------------
*/
+#ifndef PG_TAR_H
+#define PG_TAR_H
+
+#define TAR_BLOCK_SIZE 512
enum tarError
{
@@ -19,8 +23,23 @@ enum tarError
TAR_SYMLINK_TOO_LONG
};
-extern enum tarError tarCreateHeader(char *h, const char *filename, const char *linktarget,
- pgoff_t size, mode_t mode, uid_t uid, gid_t gid, time_t mtime);
+extern enum tarError tarCreateHeader(char *h, const char *filename,
+ const char *linktarget, pgoff_t size,
+ mode_t mode, uid_t uid, gid_t gid,
+ time_t mtime);
extern uint64 read_tar_number(const char *s, int len);
extern void print_tar_number(char *s, int len, uint64 val);
extern int tarChecksum(char *header);
+
+/*
+ * Compute the number of padding bytes required for an entry in a tar
+ * archive. We must pad out to a multiple of TAR_BLOCK_SIZE. Since that's
+ * a power of 2, we can use TYPEALIGN().
+ */
+static inline size_t
+tarPaddingBytesRequired(size_t len)
+{
+ return TYPEALIGN(TAR_BLOCK_SIZE, len) - len;
+}
+
+#endif
--
2.24.2 (Apple Git-127)
0001-Fix-bogus-tar-file-padding-logic-for-standby.signal.patchapplication/octet-stream; name=0001-Fix-bogus-tar-file-padding-logic-for-standby.signal.patchDownload
From 7428e0cc951b40dc556b61dbb338c0c38effecbe Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Fri, 24 Apr 2020 11:26:51 -0400
Subject: [PATCH 1/2] Fix bogus tar-file padding logic for standby.signal.
When pg_basebackup -R is used, we inject standby.signal into the
tar file for the main tablespace. The proper thing to do is to pad
each file injected into the tar file out to a 512-byte boundary
by appending nulls, but here the file is of length 0 and we add
511 zero bytes. Since 0 is already a multiple of 512, we should
not add any zero bytes. Do that instead.
---
src/bin/pg_basebackup/pg_basebackup.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index faa69d14ce..c1d3d8624b 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1207,7 +1207,12 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
time(NULL));
writeTarData(&state, header, sizeof(header));
- writeTarData(&state, zerobuf, 511);
+
+ /*
+ * we don't need to pad out to a multiple of the tar block size
+ * here, because the file is zero length, which is a multiple of
+ * any block size.
+ */
}
}
--
2.24.2 (Apple Git-127)
Robert Haas <robertmhaas@gmail.com> writes:
We have similar code in many places -- because evidently nobody
thought it would be a good idea to have all the logic for reading and
writing tarfiles in a centralized location rather than having many
copies of it -- and typically it's written to pad the block out to a
multiple of 512 bytes. But here, the file is 0 bytes long, and then we
add 511 zero bytes. This results in a tarfile whose length is not a
multiple of the TAR block size:
Bleah. Whether or not the nearest copy of tar happens to spit up on
that, it's a clear violation of the POSIX standard for tar files.
I'd vote for back-patching your 0001.
I'd lean mildly to holding 0002 until after we branch. It probably
won't break anything, but it probably won't fix anything either.
regards, tom lane
On Fri, Apr 24, 2020 at 12:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Bleah. Whether or not the nearest copy of tar happens to spit up on
that, it's a clear violation of the POSIX standard for tar files.
I'd vote for back-patching your 0001.
Done.
I'd lean mildly to holding 0002 until after we branch. It probably
won't break anything, but it probably won't fix anything either.
True.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Mon, Apr 27, 2020 at 2:07 PM Robert Haas <robertmhaas@gmail.com> wrote:
I'd lean mildly to holding 0002 until after we branch. It probably
won't break anything, but it probably won't fix anything either.True.
Committed now.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
The patch works perfectly.
The new status of this patch is: Ready for Committer
On Sun, Jun 28, 2020 at 11:24 AM Hamid Akhtar <hamid.akhtar@gmail.com> wrote:
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not testedThe patch works perfectly.
The new status of this patch is: Ready for Committer
Thanks, but this was committed on June 15th, as per my previous email.
Perhaps I forgot to update the CommitFest application....
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 29 Jun 2020, at 13:52, Robert Haas <robertmhaas@gmail.com> wrote:
On Sun, Jun 28, 2020 at 11:24 AM Hamid Akhtar <hamid.akhtar@gmail.com> wrote:
The new status of this patch is: Ready for Committer
Thanks, but this was committed on June 15th, as per my previous email.
Perhaps I forgot to update the CommitFest application....
Done now, marked as committed.
cheers ./daniel