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
4 years ago · 000f3adfdc
parent 7ead9925ff
commit 000f3adfdc
2 changed files with 38 additions and 22 deletions
--- 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);

--- 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);