From 000f3adfdc4336df83777eba86ce48f36cb6c6e9 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Fri, 7 Jan 2022 13:48:59 +0900 Subject: [PATCH] Refactor tar method of walmethods.c to rely on the compression method Since d62bcc8, the directory method of walmethods.c uses the compression method to determine which code path to take. The tar method, used by pg_basebackup --format=t, was inconsistent regarding that, as it relied on the compression level to check if no compression or gzip should be used. This commit makes the code more consistent as a whole in this file, making the tar logic use a compression method rather than assigning COMPRESSION_NONE that would be ignored. The options of pg_basebackup are planned to be reworked but we are not sure yet of the shape they should have as this has some dependency with the integration of the server-side compression for base backups, so this is left out for the moment. This change has as benefit to make easier the future integration of new compression methods for the tar method of walmethods.c, for the client-side compression. Reviewed-by: Georgios Kokolatos Discussion: https://postgr.es/m/Yb3GEgWwcu4wZDuA@paquier.xyz --- src/bin/pg_basebackup/pg_basebackup.c | 3 +- src/bin/pg_basebackup/walmethods.c | 57 +++++++++++++++++---------- 2 files changed, 38 insertions(+), 22 deletions(-) diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index 1739ac6382..17ff0132d9 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -524,7 +524,8 @@ LogStreamerMain(logstreamer_param *param) stream.do_sync); else stream.walmethod = CreateWalTarMethod(param->xlog, - COMPRESSION_NONE, /* ignored */ + (compresslevel > 0) ? + COMPRESSION_GZIP : COMPRESSION_NONE, compresslevel, stream.do_sync); diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c index affdc5055f..424070531b 100644 --- a/src/bin/pg_basebackup/walmethods.c +++ b/src/bin/pg_basebackup/walmethods.c @@ -749,7 +749,7 @@ tar_write(Walfile f, const void *buf, size_t count) tar_clear_error(); /* Tarfile will always be positioned at the end */ - if (!tar_data->compression_level) + if (tar_data->compression_method == COMPRESSION_NONE) { errno = 0; r = write(tar_data->fd, buf, count); @@ -763,21 +763,20 @@ tar_write(Walfile f, const void *buf, size_t count) return r; } #ifdef HAVE_LIBZ - else + else if (tar_data->compression_method == COMPRESSION_GZIP) { if (!tar_write_compressed_data(unconstify(void *, buf), count, false)) return -1; ((TarMethodFile *) f)->currpos += count; return count; } -#else +#endif else { - /* Can't happen - compression enabled with no libz */ + /* Can't happen - compression enabled with no method set */ tar_data->lasterrno = ENOSYS; return -1; } -#endif } static bool @@ -833,7 +832,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_ } #ifdef HAVE_LIBZ - if (tar_data->compression_level) + if (tar_data->compression_method == COMPRESSION_GZIP) { tar_data->zp = (z_streamp) pg_malloc(sizeof(z_stream)); tar_data->zp->zalloc = Z_NULL; @@ -884,7 +883,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_ pg_free(tmppath); #ifdef HAVE_LIBZ - if (tar_data->compression_level) + if (tar_data->compression_method == COMPRESSION_GZIP) { /* Flush existing data */ if (!tar_write_compressed_data(NULL, 0, true)) @@ -909,7 +908,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_ } tar_data->currentfile->currpos = 0; - if (!tar_data->compression_level) + if (tar_data->compression_method == COMPRESSION_NONE) { errno = 0; if (write(tar_data->fd, tar_data->currentfile->header, @@ -923,7 +922,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_ } } #ifdef HAVE_LIBZ - else + else if (tar_data->compression_method == COMPRESSION_GZIP) { /* Write header through the zlib APIs but with no compression */ if (!tar_write_compressed_data(tar_data->currentfile->header, @@ -938,6 +937,11 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_ } } #endif + else + { + /* not reachable */ + Assert(false); + } tar_data->currentfile->pathname = pg_strdup(pathname); @@ -948,7 +952,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_ if (pad_to_size) { tar_data->currentfile->pad_to_size = pad_to_size; - if (!tar_data->compression_level) + if (tar_data->compression_method == COMPRESSION_NONE) { /* Uncompressed, so pad now */ if (!tar_write_padding_data(tar_data->currentfile, pad_to_size)) @@ -1009,7 +1013,7 @@ tar_sync(Walfile f) * Always sync the whole tarfile, because that's all we can do. This makes * no sense on compressed files, so just ignore those. */ - if (tar_data->compression_level) + if (tar_data->compression_method != COMPRESSION_NONE) return 0; r = fsync(tar_data->fd); @@ -1030,7 +1034,7 @@ tar_close(Walfile f, WalCloseMethod method) if (method == CLOSE_UNLINK) { - if (tar_data->compression_level) + if (tar_data->compression_method != COMPRESSION_NONE) { tar_set_error("unlink not supported with compression"); return -1; @@ -1061,7 +1065,7 @@ tar_close(Walfile f, WalCloseMethod method) */ if (tf->pad_to_size) { - if (tar_data->compression_level) + if (tar_data->compression_method == COMPRESSION_GZIP) { /* * A compressed tarfile is padded on close since we cannot know @@ -1102,7 +1106,7 @@ tar_close(Walfile f, WalCloseMethod method) #ifdef HAVE_LIBZ - if (tar_data->compression_level) + if (tar_data->compression_method == COMPRESSION_GZIP) { /* Flush the current buffer */ if (!tar_write_compressed_data(NULL, 0, true)) @@ -1131,7 +1135,7 @@ tar_close(Walfile f, WalCloseMethod method) tar_data->lasterrno = errno; return -1; } - if (!tar_data->compression_level) + if (tar_data->compression_method == COMPRESSION_NONE) { errno = 0; if (write(tar_data->fd, tf->header, TAR_BLOCK_SIZE) != TAR_BLOCK_SIZE) @@ -1142,7 +1146,7 @@ tar_close(Walfile f, WalCloseMethod method) } } #ifdef HAVE_LIBZ - else + else if (tar_data->compression_method == COMPRESSION_GZIP) { /* Turn off compression */ if (deflateParams(tar_data->zp, 0, 0) != Z_OK) @@ -1164,6 +1168,11 @@ tar_close(Walfile f, WalCloseMethod method) } } #endif + else + { + /* not reachable */ + Assert(false); + } /* Move file pointer back down to end, so we can write the next file */ if (lseek(tar_data->fd, 0, SEEK_END) < 0) @@ -1212,7 +1221,7 @@ tar_finish(void) /* A tarfile always ends with two empty blocks */ MemSet(zerobuf, 0, sizeof(zerobuf)); - if (!tar_data->compression_level) + if (tar_data->compression_method == COMPRESSION_NONE) { errno = 0; if (write(tar_data->fd, zerobuf, sizeof(zerobuf)) != sizeof(zerobuf)) @@ -1223,7 +1232,7 @@ tar_finish(void) } } #ifdef HAVE_LIBZ - else + else if (tar_data->compression_method == COMPRESSION_GZIP) { if (!tar_write_compressed_data(zerobuf, sizeof(zerobuf), false)) return false; @@ -1268,6 +1277,11 @@ tar_finish(void) } } #endif + else + { + /* not reachable */ + Assert(false); + } /* sync the empty blocks as well, since they're after the last file */ if (tar_data->sync) @@ -1312,7 +1326,8 @@ CreateWalTarMethod(const char *tarbase, int compression_level, bool sync) { WalWriteMethod *method; - const char *suffix = (compression_level != 0) ? ".tar.gz" : ".tar"; + const char *suffix = (compression_method == COMPRESSION_GZIP) ? + ".tar.gz" : ".tar"; method = pg_malloc0(sizeof(WalWriteMethod)); method->open_for_write = tar_open_for_write; @@ -1335,7 +1350,7 @@ CreateWalTarMethod(const char *tarbase, tar_data->compression_level = compression_level; tar_data->sync = sync; #ifdef HAVE_LIBZ - if (compression_level) + if (compression_method == COMPRESSION_GZIP) tar_data->zlibOut = (char *) pg_malloc(ZLIB_OUT_SIZE + 1); #endif @@ -1347,7 +1362,7 @@ FreeWalTarMethod(void) { pg_free(tar_data->tarfilename); #ifdef HAVE_LIBZ - if (tar_data->compression_level) + if (tar_data->compression_method == COMPRESSION_GZIP) pg_free(tar_data->zlibOut); #endif pg_free(tar_data);