On Fri, Jul 16, 2021 at 02:08:57PM +0900, Michael Paquier wrote: > This behavior is rather debatable, and it would be more instinctive to > me to just skip any business related to the pre-padding if compression > is enabled, at the cost of one extra callback in WalWriteMethod to > grab the compression level (dir_open_for_write() skips that for > compression) to allow receivelog.c to handle that. But at the same > time few users are going to care about that as pg_receivewal has most > likely always the same set of options, so complicating this code is > not really appealing either.
I have chewed on that over the weekend, and skipping the padding logic if we are in compression mode in open_walfile() makes sense, so attached is a patch that I'd like to backpatch. Another advantage of this patch is the handling of ".gz" is reduced to one code path instead of four. That makes a bit easier the introduction of new compression methods. A second thing that was really confusing is that the name of the WAL segment generated in this code path completely ignored the type of compression. This led to one confusing error message if failing to open a segment for write where we'd mention a .partial file rather than a .gz.partial file. The versions of zlib I used on Windows looked buggy so I cannot conclude there, but I am sure that this should allow bowerbird to handle the test correctly. -- Michael
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 3952a3f943..7af9009320 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -88,26 +88,29 @@ static bool
open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
{
Walfile *f;
- char fn[MAXPGPATH];
+ char *fn;
ssize_t size;
XLogSegNo segno;
XLByteToSeg(startpoint, segno, WalSegSz);
XLogFileName(current_walfile_name, stream->timeline, segno, WalSegSz);
- snprintf(fn, sizeof(fn), "%s%s", current_walfile_name,
- stream->partial_suffix ? stream->partial_suffix : "");
+ /* Note that this considers the compression used if necessary */
+ fn = stream->walmethod->get_file_name(current_walfile_name,
+ stream->partial_suffix);
/*
* When streaming to files, if an existing file exists we verify that it's
* either empty (just created), or a complete WalSegSz segment (in which
* case it has been created and padded). Anything else indicates a corrupt
- * file.
+ * file. Compressed files have no need for padding, so just ignore this
+ * case.
*
* When streaming to tar, no file with this name will exist before, so we
* never have to verify a size.
*/
- if (stream->walmethod->existsfile(fn))
+ if (stream->walmethod->compression() == 0 &&
+ stream->walmethod->existsfile(fn))
{
size = stream->walmethod->get_file_size(fn);
if (size < 0)
diff --git a/src/bin/pg_basebackup/t/020_pg_receivewal.pl b/src/bin/pg_basebackup/t/020_pg_receivewal.pl
index 158f7d176f..17fd71a450 100644
--- a/src/bin/pg_basebackup/t/020_pg_receivewal.pl
+++ b/src/bin/pg_basebackup/t/020_pg_receivewal.pl
@@ -72,13 +72,11 @@ $primary->command_ok(
my @partial_wals = glob "$stream_dir/*\.partial";
is(scalar(@partial_wals), 1, "one partial WAL segment was created");
-# Check ZLIB compression if available. On Windows, some old versions
-# of zlib can cause some instabilities with this test, so disable it
-# for now.
+# Check ZLIB compression if available.
SKIP:
{
- skip "postgres was not built with ZLIB support, or Windows is involved", 5
- if (!check_pg_config("#define HAVE_LIBZ 1") || $windows_os);
+ skip "postgres was not built with ZLIB support", 5
+ if (!check_pg_config("#define HAVE_LIBZ 1"));
# Generate more WAL worth one completed, compressed, segment.
$primary->psql('postgres', 'SELECT pg_switch_wal();');
diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
index a15bbb20e7..3c0da70a04 100644
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -68,20 +68,32 @@ dir_getlasterror(void)
return strerror(errno);
}
+static char *
+dir_get_file_name(const char *pathname, const char *temp_suffix)
+{
+ char *tmppath = pg_malloc0(MAXPGPATH * sizeof(char));
+
+ snprintf(tmppath, MAXPGPATH, "%s%s%s",
+ pathname, dir_data->compression > 0 ? ".gz" : "",
+ temp_suffix ? temp_suffix : "");
+
+ return tmppath;
+}
+
static Walfile
dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_size)
{
static char tmppath[MAXPGPATH];
+ char *filename;
int fd;
DirectoryMethodFile *f;
#ifdef HAVE_LIBZ
gzFile gzfp = NULL;
#endif
- snprintf(tmppath, sizeof(tmppath), "%s/%s%s%s",
- dir_data->basedir, pathname,
- dir_data->compression > 0 ? ".gz" : "",
- temp_suffix ? temp_suffix : "");
+ filename = dir_get_file_name(pathname, temp_suffix);
+ snprintf(tmppath, sizeof(tmppath), "%s/%s",
+ dir_data->basedir, filename);
/*
* Open a file for non-compressed as well as compressed files. Tracking
@@ -232,26 +244,30 @@ dir_close(Walfile f, WalCloseMethod method)
/* Build path to the current version of the file */
if (method == CLOSE_NORMAL && df->temp_suffix)
{
+ char *filename;
+ char *filename2;
+
/*
* If we have a temp prefix, normal operation is to rename the
* file.
*/
- snprintf(tmppath, sizeof(tmppath), "%s/%s%s%s",
- dir_data->basedir, df->pathname,
- dir_data->compression > 0 ? ".gz" : "",
- df->temp_suffix);
- snprintf(tmppath2, sizeof(tmppath2), "%s/%s%s",
- dir_data->basedir, df->pathname,
- dir_data->compression > 0 ? ".gz" : "");
+ filename = dir_get_file_name(df->pathname, df->temp_suffix);
+ snprintf(tmppath, sizeof(tmppath), "%s/%s",
+ dir_data->basedir, filename);
+
+ filename2 = dir_get_file_name(df->pathname, NULL);
+ snprintf(tmppath2, sizeof(tmppath2), "%s/%s",
+ dir_data->basedir, filename2);
r = durable_rename(tmppath, tmppath2);
}
else if (method == CLOSE_UNLINK)
{
+ char *filename;
+
/* Unlink the file once it's closed */
- snprintf(tmppath, sizeof(tmppath), "%s/%s%s%s",
- dir_data->basedir, df->pathname,
- dir_data->compression > 0 ? ".gz" : "",
- df->temp_suffix ? df->temp_suffix : "");
+ filename = dir_get_file_name(df->pathname, df->temp_suffix);
+ snprintf(tmppath, sizeof(tmppath), "%s/%s",
+ dir_data->basedir, filename);
r = unlink(tmppath);
}
else
@@ -313,6 +329,12 @@ dir_get_file_size(const char *pathname)
return statbuf.st_size;
}
+static int
+dir_compression(void)
+{
+ return dir_data->compression;
+}
+
static bool
dir_existsfile(const char *pathname)
{
@@ -355,6 +377,8 @@ CreateWalDirectoryMethod(const char *basedir, int compression, bool sync)
method->write = dir_write;
method->get_current_pos = dir_get_current_pos;
method->get_file_size = dir_get_file_size;
+ method->get_file_name = dir_get_file_name;
+ method->compression = dir_compression;
method->close = dir_close;
method->sync = dir_sync;
method->existsfile = dir_existsfile;
@@ -527,11 +551,22 @@ tar_write_padding_data(TarMethodFile *f, size_t bytes)
return true;
}
+static char *
+tar_get_file_name(const char *pathname, const char *temp_suffix)
+{
+ char *tmppath = pg_malloc0(MAXPGPATH * sizeof(char));
+
+ snprintf(tmppath, sizeof(tmppath), "%s%s",
+ pathname, temp_suffix ? temp_suffix : "");
+
+ return tmppath;
+}
+
static Walfile
tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_size)
{
int save_errno;
- static char tmppath[MAXPGPATH];
+ char *tmppath;
tar_clear_error();
@@ -583,8 +618,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
tar_data->currentfile = pg_malloc0(sizeof(TarMethodFile));
- snprintf(tmppath, sizeof(tmppath), "%s%s",
- pathname, temp_suffix ? temp_suffix : "");
+ tmppath = tar_get_file_name(pathname, temp_suffix);
/* Create a header with size set to 0 - we will fill out the size on close */
if (tarCreateHeader(tar_data->currentfile->header, tmppath, NULL, 0, S_IRUSR | S_IWUSR, 0, 0, time(NULL)) != TAR_OK)
@@ -689,6 +723,12 @@ tar_get_file_size(const char *pathname)
return -1;
}
+static int
+tar_compression(void)
+{
+ return tar_data->compression;
+}
+
static off_t
tar_get_current_pos(Walfile f)
{
@@ -994,6 +1034,8 @@ CreateWalTarMethod(const char *tarbase, int compression, bool sync)
method->write = tar_write;
method->get_current_pos = tar_get_current_pos;
method->get_file_size = tar_get_file_size;
+ method->get_file_name = tar_get_file_name;
+ method->compression = tar_compression;
method->close = tar_close;
method->sync = tar_sync;
method->existsfile = tar_existsfile;
diff --git a/src/bin/pg_basebackup/walmethods.h b/src/bin/pg_basebackup/walmethods.h
index fc4bb52cb7..3f08f4256d 100644
--- a/src/bin/pg_basebackup/walmethods.h
+++ b/src/bin/pg_basebackup/walmethods.h
@@ -52,6 +52,15 @@ struct WalWriteMethod
/* Return the size of a file, or -1 on failure. */
ssize_t (*get_file_size) (const char *pathname);
+ /*
+ * Return the name of the current file working on, without the base
+ * directory. This is useful for logging.
+ */
+ char *(*get_file_name) (const char *pathname, const char *temp_suffix);
+
+ /* Return the level of compression */
+ int (*compression) (void);
+
/*
* Write count number of bytes to the file, and return the number of bytes
* actually written or -1 for error.
signature.asc
Description: PGP signature
