walmethods.c/h are doing some strange things

Started by Robert Haasover 3 years ago5 messages

robertmhaas@gmail.com

over 3 years ago

2 attachment(s)

Hi,

We have a number of places in the system where we are using
object-oriented design patterns. For example, a foreign data wrapper
returns a table of function pointers which are basically methods for
operating on a planner or executor node that corresponds to a foreign
table that uses that foreign data wrapper. More simply, a
TupleTableSlot or TableAmRoutine or bbstreamer or bbsink object
contains a pointer to a table of callbacks which are methods that can
be applied to that object. walmethods.c/h also try to do something
sort of like this, but I find the way that they do it really weird,
because while Create{Directory|Tar}WalMethod() does return a table of
callbacks, those callbacks aren't tied to any specific object.
Instead, each set of callbacks refers to the one and only object of
that type that can ever exist, and the pointer to that object is
stored in a global variable managed by walmethods.c. So whereas in
other cases we give you the object and then a way to get the
corresponding set of callbacks, here we only give you the callbacks,
and we therefore have to impose the artificial restriction that there
can only ever be one object.

I think it would be better to structure things so that Walfile and
WalWriteMethod function as abstract base classes; that is, each is a
struct containing those members that are common to all
implementations, and then each implementation extends that struct with
whatever additional members it needs. One advantage of this is that it
would allow us to simplify the communication between receivelog.c and
walmethods.c. Right now, for example, there's a get_current_pos()
method in WalWriteMethods. The way that works is that
WalDirectoryMethod has a struct where it stores a 'curpos' value that
is returned by this method, and WalTrMethod has a different struct
that also stores a 'currpos' value that is returned by this method.
There is no real benefit in having the same variable in two different
structs and having to access it via a callback when we could just put
it into a common struct and access it directly. There's also a
compression_algorithm() method which has exactly the same issue,
though that is an overall property of the WalWriteMethod rather than a
per-Walfile property. There's also a getlasterr callback which is
basically just duplicate code across the two implementations; we could
unify that code. There's also a global variable current_walfile_name[]
in receivelog.c which only needs to exist because the file name is
inconveniently hidden inside the WalWriteMethod abstraction layer; we
can just make it visible.

Attached are a couple of hastily-written patches implementing this.
There might be good arguments for more thoroughly renaming some of the
things these patches touch, but I thought that doing any more renaming
would make it less clear what the core of the change is, so I'm
posting it like this for now. One thing I noticed while writing these
patches is that the existing code isn't very clear about whether
"Walfile" is supposed to be an abstraction for a pointer to the
implementation-specific struct, or the struct itself. From looking at
walmethods.h, you'd think it's a pointer to the struct, because we
declare typedef void *Walfile. walmethods.c agrees, but receivelog.c
takes a different view, declaring all of its variables as type
"Walfile *". This doesn't cause a compiler error because void * is
just as interchangeable with void ** as it is with DirectoryMethodFile
* or TarMethodFile *, but I think it is clearly a mistake, and the
approach I'm proposing here makes such mistakes more difficult to
make.

Aside from the stuff that I am complaining about here which is mostly
stylistic, I think that the division of labor between receivelog.c and
walmethods.c is questionable in a number of ways. There are things
which are specific to one walmethod or the other that are handled in
the common code (receivelog.c) rather than the type-specific code
(walmethod.c), and in general it feels like receivelog.c knows way too
much about what is really happening beneath the abstraction layer that
walmethods.c supposedly creates. This comment is one of the clearer
examples of this:

/*
* 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. 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.
*/

There's nothing generic here. We're not describing an algorithm that
could be used with any walmethod that might exist now or in the
future. We're describing something that will produce the right result
given the two walmethods we actually have and the actual behavior of
the callbacks of each one. I don't really know what to do about this
part of the problem; these pieces of code are deeply intertwined in
complex ways that don't seem simple to untangle. Maybe I'll have a
better idea later, or perhaps someone else will. For now, I'd like to
get some thoughts on the attached refactoring patches that deal with
some more superficial aspects of the problem.

Thanks,

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

Attachments:

v1-0001-walmethods.c-h-Make-Walfile-an-extensible-struct-.patchapplication/octet-stream; name=v1-0001-walmethods.c-h-Make-Walfile-an-extensible-struct-.patchDownload

From 16e23b431b08e2c6b2a89f10eb5817f5fc836d66 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Thu, 1 Sep 2022 13:05:25 -0400
Subject: [PATCH v1 1/2] walmethods.c/h: Make Walfile an extensible struct,
 instead of void *

---
 src/bin/pg_basebackup/receivelog.c | 44 ++++++---------
 src/bin/pg_basebackup/walmethods.c | 89 +++++++++++-------------------
 src/bin/pg_basebackup/walmethods.h | 27 ++++++---
 3 files changed, 67 insertions(+), 93 deletions(-)

diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 5f6fd3201f..c48090661b 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -25,9 +25,8 @@
 #include "receivelog.h"
 #include "streamutil.h"
 
-/* fd and filename for currently open WAL file */
+/* currently open WAL file */
 static Walfile *walfile = NULL;
-static char current_walfile_name[MAXPGPATH] = "";
 static bool reportFlushPosition = false;
 static XLogRecPtr lastFlushPosition = InvalidXLogRecPtr;
 
@@ -82,8 +81,7 @@ mark_file_as_archived(StreamCtl *stream, const char *fname)
  * Open a new WAL file in the specified directory.
  *
  * Returns true if OK; on failure, returns false after printing an error msg.
- * On success, 'walfile' is set to the FD for the file, and the base filename
- * (without partial_suffix) is stored in 'current_walfile_name'.
+ * On success, 'walfile' is set to the FD for the file.
  *
  * The file will be padded to 16Mb with zeroes.
  */
@@ -94,12 +92,13 @@ open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
 	char	   *fn;
 	ssize_t		size;
 	XLogSegNo	segno;
+	char		walfile_name[MAXPGPATH];
 
 	XLByteToSeg(startpoint, segno, WalSegSz);
-	XLogFileName(current_walfile_name, stream->timeline, segno, WalSegSz);
+	XLogFileName(walfile_name, stream->timeline, segno, WalSegSz);
 
 	/* Note that this considers the compression used if necessary */
-	fn = stream->walmethod->get_file_name(current_walfile_name,
+	fn = stream->walmethod->get_file_name(walfile_name,
 										  stream->partial_suffix);
 
 	/*
@@ -126,7 +125,7 @@ open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
 		if (size == WalSegSz)
 		{
 			/* Already padded file. Open it for use */
-			f = stream->walmethod->open_for_write(current_walfile_name, stream->partial_suffix, 0);
+			f = stream->walmethod->open_for_write(walfile_name, stream->partial_suffix, 0);
 			if (f == NULL)
 			{
 				pg_log_error("could not open existing write-ahead log file \"%s\": %s",
@@ -165,7 +164,7 @@ open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
 
 	/* No file existed, so create one */
 
-	f = stream->walmethod->open_for_write(current_walfile_name,
+	f = stream->walmethod->open_for_write(walfile_name,
 										  stream->partial_suffix, WalSegSz);
 	if (f == NULL)
 	{
@@ -196,21 +195,10 @@ close_walfile(StreamCtl *stream, XLogRecPtr pos)
 		return true;
 
 	/* Note that this considers the compression used if necessary */
-	fn = stream->walmethod->get_file_name(current_walfile_name,
+	fn = stream->walmethod->get_file_name(walfile->pathname,
 										  stream->partial_suffix);
 
-	currpos = stream->walmethod->get_current_pos(walfile);
-
-	if (currpos == -1)
-	{
-		pg_log_error("could not determine seek position in file \"%s\": %s",
-					 fn, stream->walmethod->getlasterror());
-		stream->walmethod->close(walfile, CLOSE_UNLINK);
-		walfile = NULL;
-
-		pg_free(fn);
-		return false;
-	}
+	currpos = walfile->currpos;
 
 	if (stream->partial_suffix)
 	{
@@ -247,7 +235,7 @@ close_walfile(StreamCtl *stream, XLogRecPtr pos)
 	if (currpos == WalSegSz && stream->mark_done)
 	{
 		/* writes error message if failed */
-		if (!mark_file_as_archived(stream, current_walfile_name))
+		if (!mark_file_as_archived(stream, walfile->pathname))
 			return false;
 	}
 
@@ -690,7 +678,7 @@ ReceiveXlogStream(PGconn *conn, StreamCtl *stream)
 error:
 	if (walfile != NULL && stream->walmethod->close(walfile, CLOSE_NO_RENAME) != 0)
 		pg_log_error("could not close file \"%s\": %s",
-					 current_walfile_name, stream->walmethod->getlasterror());
+					 walfile->pathname, stream->walmethod->getlasterror());
 	walfile = NULL;
 	return false;
 }
@@ -777,7 +765,7 @@ HandleCopyStream(PGconn *conn, StreamCtl *stream,
 		{
 			if (stream->walmethod->sync(walfile) != 0)
 				pg_fatal("could not fsync file \"%s\": %s",
-						 current_walfile_name, stream->walmethod->getlasterror());
+						 walfile->pathname, stream->walmethod->getlasterror());
 			lastFlushPosition = blockpos;
 
 			/*
@@ -1024,7 +1012,7 @@ ProcessKeepaliveMsg(PGconn *conn, StreamCtl *stream, char *copybuf, int len,
 			 */
 			if (stream->walmethod->sync(walfile) != 0)
 				pg_fatal("could not fsync file \"%s\": %s",
-						 current_walfile_name, stream->walmethod->getlasterror());
+						 walfile->pathname, stream->walmethod->getlasterror());
 			lastFlushPosition = blockpos;
 		}
 
@@ -1092,10 +1080,10 @@ ProcessXLogDataMsg(PGconn *conn, StreamCtl *stream, char *copybuf, int len,
 	else
 	{
 		/* More data in existing segment */
-		if (stream->walmethod->get_current_pos(walfile) != xlogoff)
+		if (walfile->currpos != xlogoff)
 		{
 			pg_log_error("got WAL data offset %08x, expected %08x",
-						 xlogoff, (int) stream->walmethod->get_current_pos(walfile));
+						 xlogoff, (int) walfile->currpos);
 			return false;
 		}
 	}
@@ -1129,7 +1117,7 @@ ProcessXLogDataMsg(PGconn *conn, StreamCtl *stream, char *copybuf, int len,
 									 bytes_to_write) != bytes_to_write)
 		{
 			pg_log_error("could not write %d bytes to WAL file \"%s\": %s",
-						 bytes_to_write, current_walfile_name,
+						 bytes_to_write, walfile->pathname,
 						 stream->walmethod->getlasterror());
 			return false;
 		}
diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
index e90aa0ba37..096b8686e4 100644
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -62,9 +62,8 @@ static DirectoryMethodData *dir_data = NULL;
  */
 typedef struct DirectoryMethodFile
 {
+	Walfile		base;
 	int			fd;
-	off_t		currpos;
-	char	   *pathname;
 	char	   *fullpath;
 	char	   *temp_suffix;
 #ifdef HAVE_LIBZ
@@ -104,7 +103,7 @@ dir_get_file_name(const char *pathname, const char *temp_suffix)
 	return filename;
 }
 
-static Walfile
+static Walfile *
 dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_size)
 {
 	char		tmppath[MAXPGPATH];
@@ -279,18 +278,18 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 	}
 #endif
 
+	f->base.currpos = 0;
+	f->base.pathname = pg_strdup(pathname);
 	f->fd = fd;
-	f->currpos = 0;
-	f->pathname = pg_strdup(pathname);
 	f->fullpath = pg_strdup(tmppath);
 	if (temp_suffix)
 		f->temp_suffix = pg_strdup(temp_suffix);
 
-	return f;
+	return &f->base;
 }
 
 static ssize_t
-dir_write(Walfile f, const void *buf, size_t count)
+dir_write(Walfile *f, const void *buf, size_t count)
 {
 	ssize_t		r;
 	DirectoryMethodFile *df = (DirectoryMethodFile *) f;
@@ -366,22 +365,12 @@ dir_write(Walfile f, const void *buf, size_t count)
 		}
 	}
 	if (r > 0)
-		df->currpos += r;
+		df->base.currpos += r;
 	return r;
 }
 
-static off_t
-dir_get_current_pos(Walfile f)
-{
-	Assert(f != NULL);
-	dir_clear_error();
-
-	/* Use a cached value to prevent lots of reseeks */
-	return ((DirectoryMethodFile *) f)->currpos;
-}
-
 static int
-dir_close(Walfile f, WalCloseMethod method)
+dir_close(Walfile *f, WalCloseMethod method)
 {
 	int			r;
 	DirectoryMethodFile *df = (DirectoryMethodFile *) f;
@@ -440,13 +429,13 @@ dir_close(Walfile f, WalCloseMethod method)
 			 * If we have a temp prefix, normal operation is to rename the
 			 * file.
 			 */
-			filename = dir_get_file_name(df->pathname, df->temp_suffix);
+			filename = dir_get_file_name(df->base.pathname, df->temp_suffix);
 			snprintf(tmppath, sizeof(tmppath), "%s/%s",
 					 dir_data->basedir, filename);
 			pg_free(filename);
 
 			/* permanent name, so no need for the prefix */
-			filename2 = dir_get_file_name(df->pathname, NULL);
+			filename2 = dir_get_file_name(df->base.pathname, NULL);
 			snprintf(tmppath2, sizeof(tmppath2), "%s/%s",
 					 dir_data->basedir, filename2);
 			pg_free(filename2);
@@ -467,7 +456,7 @@ dir_close(Walfile f, WalCloseMethod method)
 			char	   *filename;
 
 			/* Unlink the file once it's closed */
-			filename = dir_get_file_name(df->pathname, df->temp_suffix);
+			filename = dir_get_file_name(df->base.pathname, df->temp_suffix);
 			snprintf(tmppath, sizeof(tmppath), "%s/%s",
 					 dir_data->basedir, filename);
 			pg_free(filename);
@@ -498,7 +487,7 @@ dir_close(Walfile f, WalCloseMethod method)
 	LZ4F_freeCompressionContext(df->ctx);
 #endif
 
-	pg_free(df->pathname);
+	pg_free(df->base.pathname);
 	pg_free(df->fullpath);
 	pg_free(df->temp_suffix);
 	pg_free(df);
@@ -507,7 +496,7 @@ dir_close(Walfile f, WalCloseMethod method)
 }
 
 static int
-dir_sync(Walfile f)
+dir_sync(Walfile *f)
 {
 	int			r;
 
@@ -630,7 +619,6 @@ CreateWalDirectoryMethod(const char *basedir,
 	method = pg_malloc0(sizeof(WalWriteMethod));
 	method->open_for_write = dir_open_for_write;
 	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_algorithm = dir_compression_algorithm;
@@ -665,10 +653,9 @@ FreeWalDirectoryMethod(void)
 
 typedef struct TarMethodFile
 {
+	Walfile		base;
 	off_t		ofs_start;		/* Where does the *header* for this file start */
-	off_t		currpos;
 	char		header[TAR_BLOCK_SIZE];
-	char	   *pathname;
 	size_t		pad_to_size;
 } TarMethodFile;
 
@@ -755,7 +742,7 @@ tar_write_compressed_data(void *buf, size_t count, bool flush)
 #endif
 
 static ssize_t
-tar_write(Walfile f, const void *buf, size_t count)
+tar_write(Walfile *f, const void *buf, size_t count)
 {
 	ssize_t		r;
 
@@ -773,7 +760,7 @@ tar_write(Walfile f, const void *buf, size_t count)
 			tar_data->lasterrno = errno ? errno : ENOSPC;
 			return -1;
 		}
-		((TarMethodFile *) f)->currpos += r;
+		f->currpos += r;
 		return r;
 	}
 #ifdef HAVE_LIBZ
@@ -781,7 +768,7 @@ tar_write(Walfile f, const void *buf, size_t count)
 	{
 		if (!tar_write_compressed_data(unconstify(void *, buf), count, false))
 			return -1;
-		((TarMethodFile *) f)->currpos += count;
+		f->currpos += count;
 		return count;
 	}
 #endif
@@ -803,7 +790,7 @@ tar_write_padding_data(TarMethodFile *f, size_t bytes)
 	while (bytesleft)
 	{
 		size_t		bytestowrite = Min(bytesleft, XLOG_BLCKSZ);
-		ssize_t		r = tar_write(f, zerobuf.data, bytestowrite);
+		ssize_t		r = tar_write(&f->base, zerobuf.data, bytestowrite);
 
 		if (r < 0)
 			return false;
@@ -824,7 +811,7 @@ tar_get_file_name(const char *pathname, const char *temp_suffix)
 	return filename;
 }
 
-static Walfile
+static Walfile *
 tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_size)
 {
 	char	   *tmppath;
@@ -920,7 +907,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 		tar_data->currentfile = NULL;
 		return NULL;
 	}
-	tar_data->currentfile->currpos = 0;
+	tar_data->currentfile->base.currpos = 0;
 
 	if (tar_data->compression_algorithm == PG_COMPRESSION_NONE)
 	{
@@ -957,7 +944,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 		Assert(false);
 	}
 
-	tar_data->currentfile->pathname = pg_strdup(pathname);
+	tar_data->currentfile->base.pathname = pg_strdup(pathname);
 
 	/*
 	 * Uncompressed files are padded on creation, but for compression we can't
@@ -980,11 +967,11 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 				return NULL;
 			}
 
-			tar_data->currentfile->currpos = 0;
+			tar_data->currentfile->base.currpos = 0;
 		}
 	}
 
-	return tar_data->currentfile;
+	return &tar_data->currentfile->base;
 }
 
 static ssize_t
@@ -1003,17 +990,8 @@ tar_compression_algorithm(void)
 	return tar_data->compression_algorithm;
 }
 
-static off_t
-tar_get_current_pos(Walfile f)
-{
-	Assert(f != NULL);
-	tar_clear_error();
-
-	return ((TarMethodFile *) f)->currpos;
-}
-
 static int
-tar_sync(Walfile f)
+tar_sync(Walfile *f)
 {
 	int			r;
 
@@ -1037,7 +1015,7 @@ tar_sync(Walfile f)
 }
 
 static int
-tar_close(Walfile f, WalCloseMethod method)
+tar_close(Walfile *f, WalCloseMethod method)
 {
 	ssize_t		filesize;
 	int			padding;
@@ -1065,7 +1043,7 @@ tar_close(Walfile f, WalCloseMethod method)
 			return -1;
 		}
 
-		pg_free(tf->pathname);
+		pg_free(tf->base.pathname);
 		pg_free(tf);
 		tar_data->currentfile = NULL;
 
@@ -1085,7 +1063,7 @@ tar_close(Walfile f, WalCloseMethod method)
 			 * A compressed tarfile is padded on close since we cannot know
 			 * the size of the compressed output until the end.
 			 */
-			size_t		sizeleft = tf->pad_to_size - tf->currpos;
+			size_t		sizeleft = tf->pad_to_size - tf->base.currpos;
 
 			if (sizeleft)
 			{
@@ -1099,7 +1077,7 @@ tar_close(Walfile f, WalCloseMethod method)
 			 * An uncompressed tarfile was padded on creation, so just adjust
 			 * the current position as if we seeked to the end.
 			 */
-			tf->currpos = tf->pad_to_size;
+			tf->base.currpos = tf->pad_to_size;
 		}
 	}
 
@@ -1107,7 +1085,7 @@ tar_close(Walfile f, WalCloseMethod method)
 	 * Get the size of the file, and pad out to a multiple of the tar block
 	 * size.
 	 */
-	filesize = tar_get_current_pos(f);
+	filesize = f->currpos;
 	padding = tarPaddingBytesRequired(filesize);
 	if (padding)
 	{
@@ -1140,7 +1118,7 @@ tar_close(Walfile f, WalCloseMethod method)
 		 * We overwrite it with what it was before if we have no tempname,
 		 * since we're going to write the buffer anyway.
 		 */
-		strlcpy(&(tf->header[0]), tf->pathname, 100);
+		strlcpy(&(tf->header[0]), tf->base.pathname, 100);
 
 	print_tar_number(&(tf->header[148]), 8, tarChecksum(((TarMethodFile *) f)->header));
 	if (lseek(tar_data->fd, tf->ofs_start, SEEK_SET) != ((TarMethodFile *) f)->ofs_start)
@@ -1199,11 +1177,11 @@ tar_close(Walfile f, WalCloseMethod method)
 	{
 		/* XXX this seems pretty bogus; why is only this case fatal? */
 		pg_fatal("could not fsync file \"%s\": %s",
-				 tf->pathname, tar_getlasterror());
+				 tf->base.pathname, tar_getlasterror());
 	}
 
 	/* Clean up and done */
-	pg_free(tf->pathname);
+	pg_free(tf->base.pathname);
 	pg_free(tf);
 	tar_data->currentfile = NULL;
 
@@ -1227,7 +1205,7 @@ tar_finish(void)
 
 	if (tar_data->currentfile)
 	{
-		if (tar_close(tar_data->currentfile, CLOSE_NORMAL) != 0)
+		if (tar_close(&tar_data->currentfile->base, CLOSE_NORMAL) != 0)
 			return false;
 	}
 
@@ -1343,7 +1321,6 @@ CreateWalTarMethod(const char *tarbase,
 	method = pg_malloc0(sizeof(WalWriteMethod));
 	method->open_for_write = tar_open_for_write;
 	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_algorithm = tar_compression_algorithm;
diff --git a/src/bin/pg_basebackup/walmethods.h b/src/bin/pg_basebackup/walmethods.h
index 76530dc941..cf5ed87fbe 100644
--- a/src/bin/pg_basebackup/walmethods.h
+++ b/src/bin/pg_basebackup/walmethods.h
@@ -11,7 +11,20 @@
 
 #include "common/compression.h"
 
-typedef void *Walfile;
+struct WalWriteMethod;
+typedef struct WalWriteMethod WalWriteMethod;
+
+typedef struct
+{
+	off_t		currpos;
+	char	   *pathname;
+	/*
+	 * MORE DATA FOLLOWS AT END OF STRUCT
+	 *
+	 * Each WalWriteMethod is expected to embed this as the first member of
+	 * a larger struct with method-specific fields following.
+	 */
+} Walfile;
 
 typedef enum
 {
@@ -30,7 +43,6 @@ typedef enum
  * care not to clobber errno between a failed method call and use of
  * getlasterror() to retrieve the message.
  */
-typedef struct WalWriteMethod WalWriteMethod;
 struct WalWriteMethod
 {
 	/*
@@ -39,13 +51,13 @@ struct WalWriteMethod
 	 * automatically renamed in close(). If pad_to_size is specified, the file
 	 * will be padded with NUL up to that size, if supported by the Walmethod.
 	 */
-	Walfile		(*open_for_write) (const char *pathname, const char *temp_suffix, size_t pad_to_size);
+	Walfile	   *(*open_for_write) (const char *pathname, const char *temp_suffix, size_t pad_to_size);
 
 	/*
 	 * Close an open Walfile, using one or more methods for handling automatic
 	 * unlinking etc. Returns 0 on success, other values for error.
 	 */
-	int			(*close) (Walfile f, WalCloseMethod method);
+	int			(*close) (Walfile *f, WalCloseMethod method);
 
 	/* Check if a file exist */
 	bool		(*existsfile) (const char *pathname);
@@ -66,15 +78,12 @@ struct WalWriteMethod
 	 * Write count number of bytes to the file, and return the number of bytes
 	 * actually written or -1 for error.
 	 */
-	ssize_t		(*write) (Walfile f, const void *buf, size_t count);
-
-	/* Return the current position in a file or -1 on error */
-	off_t		(*get_current_pos) (Walfile f);
+	ssize_t		(*write) (Walfile *f, const void *buf, size_t count);
 
 	/*
 	 * fsync the contents of the specified file. Returns 0 on success.
 	 */
-	int			(*sync) (Walfile f);
+	int			(*sync) (Walfile *f);
 
 	/*
 	 * Clean up the Walmethod, closing any shared resources. For methods like
-- 
2.24.3 (Apple Git-128)

v1-0002-walmethods.c-h-Make-WalWriteMethod-an-extensible-.patchapplication/octet-stream; name=v1-0002-walmethods.c-h-Make-WalWriteMethod-an-extensible-.patchDownload

From d0a7e16854c2327937ce14a58c29b79601dfcebf Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Thu, 1 Sep 2022 16:13:36 -0400
Subject: [PATCH v1 2/2] walmethods.c/h: Make WalWriteMethod an extensible
 struct.

Make it have a constant ops table.

Remove some global variables.
---
 src/bin/pg_basebackup/pg_basebackup.c |   8 +-
 src/bin/pg_basebackup/pg_receivewal.c |   6 +-
 src/bin/pg_basebackup/receivelog.c    |  89 ++---
 src/bin/pg_basebackup/walmethods.c    | 510 ++++++++++++++------------
 src/bin/pg_basebackup/walmethods.h    |  60 +--
 5 files changed, 357 insertions(+), 316 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 9ce30d43a4..d12441a7bc 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -570,7 +570,7 @@ LogStreamerMain(logstreamer_param *param)
 		return 1;
 	}
 
-	if (!stream.walmethod->finish())
+	if (!stream.walmethod->ops->finish(stream.walmethod))
 	{
 		pg_log_error("could not finish writing WAL files: %m");
 #ifdef WIN32
@@ -581,11 +581,7 @@ LogStreamerMain(logstreamer_param *param)
 
 	PQfinish(param->bgconn);
 
-	if (format == 'p')
-		FreeWalDirectoryMethod();
-	else
-		FreeWalTarMethod();
-	pg_free(stream.walmethod);
+	stream.walmethod->ops->free(stream.walmethod);
 
 	return 0;
 }
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index f064cff4ab..8deab50c3f 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -658,7 +658,7 @@ StreamLog(void)
 
 	ReceiveXlogStream(conn, &stream);
 
-	if (!stream.walmethod->finish())
+	if (!stream.walmethod->ops->finish(stream.walmethod))
 	{
 		pg_log_info("could not finish writing WAL files: %m");
 		return;
@@ -667,9 +667,7 @@ StreamLog(void)
 	PQfinish(conn);
 	conn = NULL;
 
-	FreeWalDirectoryMethod();
-	pg_free(stream.walmethod);
-	pg_free(stream.sysidentifier);
+	stream.walmethod->ops->free(stream.walmethod);
 }
 
 /*
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index c48090661b..5ed7758283 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -59,18 +59,19 @@ mark_file_as_archived(StreamCtl *stream, const char *fname)
 	snprintf(tmppath, sizeof(tmppath), "archive_status/%s.done",
 			 fname);
 
-	f = stream->walmethod->open_for_write(tmppath, NULL, 0);
+	f = stream->walmethod->ops->open_for_write(stream->walmethod, tmppath,
+											   NULL, 0);
 	if (f == NULL)
 	{
 		pg_log_error("could not create archive status file \"%s\": %s",
-					 tmppath, stream->walmethod->getlasterror());
+					 tmppath, GetLastWalMethodError(stream->walmethod));
 		return false;
 	}
 
-	if (stream->walmethod->close(f, CLOSE_NORMAL) != 0)
+	if (stream->walmethod->ops->close(f, CLOSE_NORMAL) != 0)
 	{
 		pg_log_error("could not close archive status file \"%s\": %s",
-					 tmppath, stream->walmethod->getlasterror());
+					 tmppath, GetLastWalMethodError(stream->walmethod));
 		return false;
 	}
 
@@ -98,8 +99,9 @@ open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
 	XLogFileName(walfile_name, stream->timeline, segno, WalSegSz);
 
 	/* Note that this considers the compression used if necessary */
-	fn = stream->walmethod->get_file_name(walfile_name,
-										  stream->partial_suffix);
+	fn = stream->walmethod->ops->get_file_name(stream->walmethod,
+											   walfile_name,
+											   stream->partial_suffix);
 
 	/*
 	 * When streaming to files, if an existing file exists we verify that it's
@@ -111,35 +113,35 @@ open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
 	 * When streaming to tar, no file with this name will exist before, so we
 	 * never have to verify a size.
 	 */
-	if (stream->walmethod->compression_algorithm() == PG_COMPRESSION_NONE &&
-		stream->walmethod->existsfile(fn))
+	if (stream->walmethod->compression_algorithm == PG_COMPRESSION_NONE &&
+		stream->walmethod->ops->existsfile(stream->walmethod, fn))
 	{
-		size = stream->walmethod->get_file_size(fn);
+		size = stream->walmethod->ops->get_file_size(stream->walmethod, fn);
 		if (size < 0)
 		{
 			pg_log_error("could not get size of write-ahead log file \"%s\": %s",
-						 fn, stream->walmethod->getlasterror());
+						 fn, GetLastWalMethodError(stream->walmethod));
 			pg_free(fn);
 			return false;
 		}
 		if (size == WalSegSz)
 		{
 			/* Already padded file. Open it for use */
-			f = stream->walmethod->open_for_write(walfile_name, stream->partial_suffix, 0);
+			f = stream->walmethod->ops->open_for_write(stream->walmethod, walfile_name, stream->partial_suffix, 0);
 			if (f == NULL)
 			{
 				pg_log_error("could not open existing write-ahead log file \"%s\": %s",
-							 fn, stream->walmethod->getlasterror());
+							 fn, GetLastWalMethodError(stream->walmethod));
 				pg_free(fn);
 				return false;
 			}
 
 			/* fsync file in case of a previous crash */
-			if (stream->walmethod->sync(f) != 0)
+			if (stream->walmethod->ops->sync(f) != 0)
 			{
 				pg_log_error("could not fsync existing write-ahead log file \"%s\": %s",
-							 fn, stream->walmethod->getlasterror());
-				stream->walmethod->close(f, CLOSE_UNLINK);
+							 fn, GetLastWalMethodError(stream->walmethod));
+				stream->walmethod->ops->close(f, CLOSE_UNLINK);
 				exit(1);
 			}
 
@@ -164,12 +166,14 @@ open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
 
 	/* No file existed, so create one */
 
-	f = stream->walmethod->open_for_write(walfile_name,
-										  stream->partial_suffix, WalSegSz);
+	f = stream->walmethod->ops->open_for_write(stream->walmethod,
+											   walfile_name,
+											   stream->partial_suffix,
+											   WalSegSz);
 	if (f == NULL)
 	{
 		pg_log_error("could not open write-ahead log file \"%s\": %s",
-					 fn, stream->walmethod->getlasterror());
+					 fn, GetLastWalMethodError(stream->walmethod));
 		pg_free(fn);
 		return false;
 	}
@@ -195,30 +199,31 @@ close_walfile(StreamCtl *stream, XLogRecPtr pos)
 		return true;
 
 	/* Note that this considers the compression used if necessary */
-	fn = stream->walmethod->get_file_name(walfile->pathname,
-										  stream->partial_suffix);
+	fn = stream->walmethod->ops->get_file_name(stream->walmethod,
+											   walfile->pathname,
+											   stream->partial_suffix);
 
 	currpos = walfile->currpos;
 
 	if (stream->partial_suffix)
 	{
 		if (currpos == WalSegSz)
-			r = stream->walmethod->close(walfile, CLOSE_NORMAL);
+			r = stream->walmethod->ops->close(walfile, CLOSE_NORMAL);
 		else
 		{
 			pg_log_info("not renaming \"%s\", segment is not complete", fn);
-			r = stream->walmethod->close(walfile, CLOSE_NO_RENAME);
+			r = stream->walmethod->ops->close(walfile, CLOSE_NO_RENAME);
 		}
 	}
 	else
-		r = stream->walmethod->close(walfile, CLOSE_NORMAL);
+		r = stream->walmethod->ops->close(walfile, CLOSE_NORMAL);
 
 	walfile = NULL;
 
 	if (r != 0)
 	{
 		pg_log_error("could not close file \"%s\": %s",
-					 fn, stream->walmethod->getlasterror());
+					 fn, GetLastWalMethodError(stream->walmethod));
 
 		pg_free(fn);
 		return false;
@@ -261,7 +266,7 @@ existsTimeLineHistoryFile(StreamCtl *stream)
 
 	TLHistoryFileName(histfname, stream->timeline);
 
-	return stream->walmethod->existsfile(histfname);
+	return stream->walmethod->ops->existsfile(stream->walmethod, histfname);
 }
 
 static bool
@@ -283,31 +288,32 @@ writeTimeLineHistoryFile(StreamCtl *stream, char *filename, char *content)
 		return false;
 	}
 
-	f = stream->walmethod->open_for_write(histfname, ".tmp", 0);
+	f = stream->walmethod->ops->open_for_write(stream->walmethod,
+											   histfname, ".tmp", 0);
 	if (f == NULL)
 	{
 		pg_log_error("could not create timeline history file \"%s\": %s",
-					 histfname, stream->walmethod->getlasterror());
+					 histfname, GetLastWalMethodError(stream->walmethod));
 		return false;
 	}
 
-	if ((int) stream->walmethod->write(f, content, size) != size)
+	if ((int) stream->walmethod->ops->write(f, content, size) != size)
 	{
 		pg_log_error("could not write timeline history file \"%s\": %s",
-					 histfname, stream->walmethod->getlasterror());
+					 histfname, GetLastWalMethodError(stream->walmethod));
 
 		/*
 		 * If we fail to make the file, delete it to release disk space
 		 */
-		stream->walmethod->close(f, CLOSE_UNLINK);
+		stream->walmethod->ops->close(f, CLOSE_UNLINK);
 
 		return false;
 	}
 
-	if (stream->walmethod->close(f, CLOSE_NORMAL) != 0)
+	if (stream->walmethod->ops->close(f, CLOSE_NORMAL) != 0)
 	{
 		pg_log_error("could not close file \"%s\": %s",
-					 histfname, stream->walmethod->getlasterror());
+					 histfname, GetLastWalMethodError(stream->walmethod));
 		return false;
 	}
 
@@ -676,9 +682,9 @@ ReceiveXlogStream(PGconn *conn, StreamCtl *stream)
 	}
 
 error:
-	if (walfile != NULL && stream->walmethod->close(walfile, CLOSE_NO_RENAME) != 0)
+	if (walfile != NULL && stream->walmethod->ops->close(walfile, CLOSE_NO_RENAME) != 0)
 		pg_log_error("could not close file \"%s\": %s",
-					 walfile->pathname, stream->walmethod->getlasterror());
+					 walfile->pathname, GetLastWalMethodError(stream->walmethod));
 	walfile = NULL;
 	return false;
 }
@@ -763,9 +769,9 @@ HandleCopyStream(PGconn *conn, StreamCtl *stream,
 		 */
 		if (stream->synchronous && lastFlushPosition < blockpos && walfile != NULL)
 		{
-			if (stream->walmethod->sync(walfile) != 0)
+			if (stream->walmethod->ops->sync(walfile) != 0)
 				pg_fatal("could not fsync file \"%s\": %s",
-						 walfile->pathname, stream->walmethod->getlasterror());
+						 walfile->pathname, GetLastWalMethodError(stream->walmethod));
 			lastFlushPosition = blockpos;
 
 			/*
@@ -1010,9 +1016,9 @@ ProcessKeepaliveMsg(PGconn *conn, StreamCtl *stream, char *copybuf, int len,
 			 * data has been successfully replicated or not, at the normal
 			 * shutdown of the server.
 			 */
-			if (stream->walmethod->sync(walfile) != 0)
+			if (stream->walmethod->ops->sync(walfile) != 0)
 				pg_fatal("could not fsync file \"%s\": %s",
-						 walfile->pathname, stream->walmethod->getlasterror());
+						 walfile->pathname, GetLastWalMethodError(stream->walmethod));
 			lastFlushPosition = blockpos;
 		}
 
@@ -1113,12 +1119,13 @@ ProcessXLogDataMsg(PGconn *conn, StreamCtl *stream, char *copybuf, int len,
 			}
 		}
 
-		if (stream->walmethod->write(walfile, copybuf + hdr_len + bytes_written,
-									 bytes_to_write) != bytes_to_write)
+		if (stream->walmethod->ops->write(walfile,
+										  copybuf + hdr_len + bytes_written,
+										  bytes_to_write) != bytes_to_write)
 		{
 			pg_log_error("could not write %d bytes to WAL file \"%s\": %s",
 						 bytes_to_write, walfile->pathname,
-						 stream->walmethod->getlasterror());
+						 GetLastWalMethodError(stream->walmethod));
 			return false;
 		}
 
diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
index 096b8686e4..397f4a6153 100644
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -2,9 +2,6 @@
  *
  * walmethods.c - implementations of different ways to write received wal
  *
- * NOTE! The caller must ensure that only one method is instantiated in
- *		 any given program, and that it's only instantiated once!
- *
  * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
  *
  * IDENTIFICATION
@@ -43,19 +40,41 @@
  *-------------------------------------------------------------------------
  */
 
+static Walfile *dir_open_for_write(WalWriteMethod *wwmethod,
+								   const char *pathname,
+								   const char *temp_suffix,
+								   size_t pad_to_size);
+static int dir_close(Walfile *f, WalCloseMethod method);
+static bool dir_existsfile(WalWriteMethod *wwmethod, const char *pathname);
+static ssize_t dir_get_file_size(WalWriteMethod *wwmethod,
+								 const char *pathname);
+static char *dir_get_file_name(WalWriteMethod *wwmethod,
+							   const char *pathname, const char *temp_suffix);
+static ssize_t dir_write(Walfile *f, const void *buf, size_t count);
+static int dir_sync(Walfile *f);
+static bool dir_finish(WalWriteMethod *wwmethod);
+static void dir_free(WalWriteMethod *wwmethod);
+
+const WalWriteMethodOps WalDirectoryMethodOps = {
+	.open_for_write = dir_open_for_write,
+	.close = dir_close,
+	.existsfile = dir_existsfile,
+	.get_file_size = dir_get_file_size,
+	.get_file_name = dir_get_file_name,
+	.write = dir_write,
+	.sync = dir_sync,
+	.finish = dir_finish,
+	.free = dir_free
+};
+
 /*
  * Global static data for this method
  */
 typedef struct DirectoryMethodData
 {
+	WalWriteMethod	base;
 	char	   *basedir;
-	pg_compress_algorithm compression_algorithm;
-	int			compression_level;
-	bool		sync;
-	const char *lasterrstring;	/* if set, takes precedence over lasterrno */
-	int			lasterrno;
 } DirectoryMethodData;
-static DirectoryMethodData *dir_data = NULL;
 
 /*
  * Local file handle
@@ -76,36 +95,29 @@ typedef struct DirectoryMethodFile
 #endif
 } DirectoryMethodFile;
 
-#define dir_clear_error() \
-	(dir_data->lasterrstring = NULL, dir_data->lasterrno = 0)
-#define dir_set_error(msg) \
-	(dir_data->lasterrstring = _(msg))
-
-static const char *
-dir_getlasterror(void)
-{
-	if (dir_data->lasterrstring)
-		return dir_data->lasterrstring;
-	return strerror(dir_data->lasterrno);
-}
+#define clear_error(wwmethod) \
+	((wwmethod)->lasterrstring = NULL, (wwmethod)->lasterrno = 0)
 
 static char *
-dir_get_file_name(const char *pathname, const char *temp_suffix)
+dir_get_file_name(WalWriteMethod *wwmethod,
+				  const char *pathname, const char *temp_suffix)
 {
 	char	   *filename = pg_malloc0(MAXPGPATH * sizeof(char));
 
 	snprintf(filename, MAXPGPATH, "%s%s%s",
 			 pathname,
-			 dir_data->compression_algorithm == PG_COMPRESSION_GZIP ? ".gz" :
-			 dir_data->compression_algorithm == PG_COMPRESSION_LZ4 ? ".lz4" : "",
+			 wwmethod->compression_algorithm == PG_COMPRESSION_GZIP ? ".gz" :
+			 wwmethod->compression_algorithm == PG_COMPRESSION_LZ4 ? ".lz4" : "",
 			 temp_suffix ? temp_suffix : "");
 
 	return filename;
 }
 
 static Walfile *
-dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_size)
+dir_open_for_write(WalWriteMethod *wwmethod, const char *pathname,
+				   const char *temp_suffix, size_t pad_to_size)
 {
+	DirectoryMethodData *dir_data = (DirectoryMethodData *) wwmethod;
 	char		tmppath[MAXPGPATH];
 	char	   *filename;
 	int			fd;
@@ -119,9 +131,9 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 	void	   *lz4buf = NULL;
 #endif
 
-	dir_clear_error();
+	clear_error(wwmethod);
 
-	filename = dir_get_file_name(pathname, temp_suffix);
+	filename = dir_get_file_name(wwmethod, pathname, temp_suffix);
 	snprintf(tmppath, sizeof(tmppath), "%s/%s",
 			 dir_data->basedir, filename);
 	pg_free(filename);
@@ -135,32 +147,32 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 	fd = open(tmppath, O_WRONLY | O_CREAT | PG_BINARY, pg_file_create_mode);
 	if (fd < 0)
 	{
-		dir_data->lasterrno = errno;
+		wwmethod->lasterrno = errno;
 		return NULL;
 	}
 
 #ifdef HAVE_LIBZ
-	if (dir_data->compression_algorithm == PG_COMPRESSION_GZIP)
+	if (wwmethod->compression_algorithm == PG_COMPRESSION_GZIP)
 	{
 		gzfp = gzdopen(fd, "wb");
 		if (gzfp == NULL)
 		{
-			dir_data->lasterrno = errno;
+			wwmethod->lasterrno = errno;
 			close(fd);
 			return NULL;
 		}
 
-		if (gzsetparams(gzfp, dir_data->compression_level,
+		if (gzsetparams(gzfp, wwmethod->compression_level,
 						Z_DEFAULT_STRATEGY) != Z_OK)
 		{
-			dir_data->lasterrno = errno;
+			wwmethod->lasterrno = errno;
 			gzclose(gzfp);
 			return NULL;
 		}
 	}
 #endif
 #ifdef USE_LZ4
-	if (dir_data->compression_algorithm == PG_COMPRESSION_LZ4)
+	if (wwmethod->compression_algorithm == PG_COMPRESSION_LZ4)
 	{
 		size_t		ctx_out;
 		size_t		header_size;
@@ -169,7 +181,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 		ctx_out = LZ4F_createCompressionContext(&ctx, LZ4F_VERSION);
 		if (LZ4F_isError(ctx_out))
 		{
-			dir_data->lasterrstring = LZ4F_getErrorName(ctx_out);
+			wwmethod->lasterrstring = LZ4F_getErrorName(ctx_out);
 			close(fd);
 			return NULL;
 		}
@@ -179,13 +191,13 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 
 		/* assign the compression level, default is 0 */
 		memset(&prefs, 0, sizeof(prefs));
-		prefs.compressionLevel = dir_data->compression_level;
+		prefs.compressionLevel = wwmethod->compression_level;
 
 		/* add the header */
 		header_size = LZ4F_compressBegin(ctx, lz4buf, lz4bufsize, &prefs);
 		if (LZ4F_isError(header_size))
 		{
-			dir_data->lasterrstring = LZ4F_getErrorName(header_size);
+			wwmethod->lasterrstring = LZ4F_getErrorName(header_size);
 			(void) LZ4F_freeCompressionContext(ctx);
 			pg_free(lz4buf);
 			close(fd);
@@ -196,7 +208,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 		if (write(fd, lz4buf, header_size) != header_size)
 		{
 			/* If write didn't set errno, assume problem is no disk space */
-			dir_data->lasterrno = errno ? errno : ENOSPC;
+			wwmethod->lasterrno = errno ? errno : ENOSPC;
 			(void) LZ4F_freeCompressionContext(ctx);
 			pg_free(lz4buf);
 			close(fd);
@@ -206,7 +218,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 #endif
 
 	/* Do pre-padding on non-compressed files */
-	if (pad_to_size && dir_data->compression_algorithm == PG_COMPRESSION_NONE)
+	if (pad_to_size && wwmethod->compression_algorithm == PG_COMPRESSION_NONE)
 	{
 		PGAlignedXLogBlock zerobuf;
 		int			bytes;
@@ -218,7 +230,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 			if (write(fd, zerobuf.data, XLOG_BLCKSZ) != XLOG_BLCKSZ)
 			{
 				/* If write didn't set errno, assume problem is no disk space */
-				dir_data->lasterrno = errno ? errno : ENOSPC;
+				wwmethod->lasterrno = errno ? errno : ENOSPC;
 				close(fd);
 				return NULL;
 			}
@@ -226,7 +238,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 
 		if (lseek(fd, 0, SEEK_SET) != 0)
 		{
-			dir_data->lasterrno = errno;
+			wwmethod->lasterrno = errno;
 			close(fd);
 			return NULL;
 		}
@@ -238,19 +250,19 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 	 * important when using synchronous mode, where the file is modified and
 	 * fsynced in-place, without a directory fsync.
 	 */
-	if (dir_data->sync)
+	if (wwmethod->sync)
 	{
 		if (fsync_fname(tmppath, false) != 0 ||
 			fsync_parent_path(tmppath) != 0)
 		{
-			dir_data->lasterrno = errno;
+			wwmethod->lasterrno = errno;
 #ifdef HAVE_LIBZ
-			if (dir_data->compression_algorithm == PG_COMPRESSION_GZIP)
+			if (wwmethod->compression_algorithm == PG_COMPRESSION_GZIP)
 				gzclose(gzfp);
 			else
 #endif
 #ifdef USE_LZ4
-			if (dir_data->compression_algorithm == PG_COMPRESSION_LZ4)
+			if (wwmethod->compression_algorithm == PG_COMPRESSION_LZ4)
 			{
 				(void) LZ4F_compressEnd(ctx, lz4buf, lz4bufsize, NULL);
 				(void) LZ4F_freeCompressionContext(ctx);
@@ -266,11 +278,11 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 
 	f = pg_malloc0(sizeof(DirectoryMethodFile));
 #ifdef HAVE_LIBZ
-	if (dir_data->compression_algorithm == PG_COMPRESSION_GZIP)
+	if (wwmethod->compression_algorithm == PG_COMPRESSION_GZIP)
 		f->gzfp = gzfp;
 #endif
 #ifdef USE_LZ4
-	if (dir_data->compression_algorithm == PG_COMPRESSION_LZ4)
+	if (wwmethod->compression_algorithm == PG_COMPRESSION_LZ4)
 	{
 		f->ctx = ctx;
 		f->lz4buf = lz4buf;
@@ -278,6 +290,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 	}
 #endif
 
+	f->base.wwmethod = wwmethod;
 	f->base.currpos = 0;
 	f->base.pathname = pg_strdup(pathname);
 	f->fd = fd;
@@ -295,23 +308,23 @@ dir_write(Walfile *f, const void *buf, size_t count)
 	DirectoryMethodFile *df = (DirectoryMethodFile *) f;
 
 	Assert(f != NULL);
-	dir_clear_error();
+	clear_error(f->wwmethod);
 
 #ifdef HAVE_LIBZ
-	if (dir_data->compression_algorithm == PG_COMPRESSION_GZIP)
+	if (f->wwmethod->compression_algorithm == PG_COMPRESSION_GZIP)
 	{
 		errno = 0;
 		r = (ssize_t) gzwrite(df->gzfp, buf, count);
 		if (r != count)
 		{
 			/* If write didn't set errno, assume problem is no disk space */
-			dir_data->lasterrno = errno ? errno : ENOSPC;
+			f->wwmethod->lasterrno = errno ? errno : ENOSPC;
 		}
 	}
 	else
 #endif
 #ifdef USE_LZ4
-	if (dir_data->compression_algorithm == PG_COMPRESSION_LZ4)
+	if (f->wwmethod->compression_algorithm == PG_COMPRESSION_LZ4)
 	{
 		size_t		chunk;
 		size_t		remaining;
@@ -335,7 +348,7 @@ dir_write(Walfile *f, const void *buf, size_t count)
 
 			if (LZ4F_isError(compressed))
 			{
-				dir_data->lasterrstring = LZ4F_getErrorName(compressed);
+				f->wwmethod->lasterrstring = LZ4F_getErrorName(compressed);
 				return -1;
 			}
 
@@ -343,7 +356,7 @@ dir_write(Walfile *f, const void *buf, size_t count)
 			if (write(df->fd, df->lz4buf, compressed) != compressed)
 			{
 				/* If write didn't set errno, assume problem is no disk space */
-				dir_data->lasterrno = errno ? errno : ENOSPC;
+				f->wwmethod->lasterrno = errno ? errno : ENOSPC;
 				return -1;
 			}
 
@@ -361,7 +374,7 @@ dir_write(Walfile *f, const void *buf, size_t count)
 		if (r != count)
 		{
 			/* If write didn't set errno, assume problem is no disk space */
-			dir_data->lasterrno = errno ? errno : ENOSPC;
+			f->wwmethod->lasterrno = errno ? errno : ENOSPC;
 		}
 	}
 	if (r > 0)
@@ -374,14 +387,15 @@ dir_close(Walfile *f, WalCloseMethod method)
 {
 	int			r;
 	DirectoryMethodFile *df = (DirectoryMethodFile *) f;
+	DirectoryMethodData *dir_data = (DirectoryMethodData *) f->wwmethod;
 	char		tmppath[MAXPGPATH];
 	char		tmppath2[MAXPGPATH];
 
 	Assert(f != NULL);
-	dir_clear_error();
+	clear_error(f->wwmethod);
 
 #ifdef HAVE_LIBZ
-	if (dir_data->compression_algorithm == PG_COMPRESSION_GZIP)
+	if (f->wwmethod->compression_algorithm == PG_COMPRESSION_GZIP)
 	{
 		errno = 0;				/* in case gzclose() doesn't set it */
 		r = gzclose(df->gzfp);
@@ -389,7 +403,7 @@ dir_close(Walfile *f, WalCloseMethod method)
 	else
 #endif
 #ifdef USE_LZ4
-	if (dir_data->compression_algorithm == PG_COMPRESSION_LZ4)
+	if (f->wwmethod->compression_algorithm == PG_COMPRESSION_LZ4)
 	{
 		size_t		compressed;
 
@@ -399,7 +413,7 @@ dir_close(Walfile *f, WalCloseMethod method)
 
 		if (LZ4F_isError(compressed))
 		{
-			dir_data->lasterrstring = LZ4F_getErrorName(compressed);
+			f->wwmethod->lasterrstring = LZ4F_getErrorName(compressed);
 			return -1;
 		}
 
@@ -407,7 +421,7 @@ dir_close(Walfile *f, WalCloseMethod method)
 		if (write(df->fd, df->lz4buf, compressed) != compressed)
 		{
 			/* If write didn't set errno, assume problem is no disk space */
-			dir_data->lasterrno = errno ? errno : ENOSPC;
+			f->wwmethod->lasterrno = errno ? errno : ENOSPC;
 			return -1;
 		}
 
@@ -429,17 +443,18 @@ dir_close(Walfile *f, WalCloseMethod method)
 			 * If we have a temp prefix, normal operation is to rename the
 			 * file.
 			 */
-			filename = dir_get_file_name(df->base.pathname, df->temp_suffix);
+			filename = dir_get_file_name(f->wwmethod, df->base.pathname,
+										 df->temp_suffix);
 			snprintf(tmppath, sizeof(tmppath), "%s/%s",
 					 dir_data->basedir, filename);
 			pg_free(filename);
 
 			/* permanent name, so no need for the prefix */
-			filename2 = dir_get_file_name(df->base.pathname, NULL);
+			filename2 = dir_get_file_name(f->wwmethod, df->base.pathname, NULL);
 			snprintf(tmppath2, sizeof(tmppath2), "%s/%s",
 					 dir_data->basedir, filename2);
 			pg_free(filename2);
-			if (dir_data->sync)
+			if (f->wwmethod->sync)
 				r = durable_rename(tmppath, tmppath2);
 			else
 			{
@@ -456,7 +471,8 @@ dir_close(Walfile *f, WalCloseMethod method)
 			char	   *filename;
 
 			/* Unlink the file once it's closed */
-			filename = dir_get_file_name(df->base.pathname, df->temp_suffix);
+			filename = dir_get_file_name(f->wwmethod, df->base.pathname,
+										 df->temp_suffix);
 			snprintf(tmppath, sizeof(tmppath), "%s/%s",
 					 dir_data->basedir, filename);
 			pg_free(filename);
@@ -469,7 +485,7 @@ dir_close(Walfile *f, WalCloseMethod method)
 			 * CLOSE_NO_RENAME. In this case, fsync the file and containing
 			 * directory if sync mode is requested.
 			 */
-			if (dir_data->sync)
+			if (f->wwmethod->sync)
 			{
 				r = fsync_fname(df->fullpath, false);
 				if (r == 0)
@@ -479,7 +495,7 @@ dir_close(Walfile *f, WalCloseMethod method)
 	}
 
 	if (r != 0)
-		dir_data->lasterrno = errno;
+		f->wwmethod->lasterrno = errno;
 
 #ifdef USE_LZ4
 	pg_free(df->lz4buf);
@@ -501,23 +517,23 @@ dir_sync(Walfile *f)
 	int			r;
 
 	Assert(f != NULL);
-	dir_clear_error();
+	clear_error(f->wwmethod);
 
-	if (!dir_data->sync)
+	if (!f->wwmethod->sync)
 		return 0;
 
 #ifdef HAVE_LIBZ
-	if (dir_data->compression_algorithm == PG_COMPRESSION_GZIP)
+	if (f->wwmethod->compression_algorithm == PG_COMPRESSION_GZIP)
 	{
 		if (gzflush(((DirectoryMethodFile *) f)->gzfp, Z_SYNC_FLUSH) != Z_OK)
 		{
-			dir_data->lasterrno = errno;
+			f->wwmethod->lasterrno = errno;
 			return -1;
 		}
 	}
 #endif
 #ifdef USE_LZ4
-	if (dir_data->compression_algorithm == PG_COMPRESSION_LZ4)
+	if (f->wwmethod->compression_algorithm == PG_COMPRESSION_LZ4)
 	{
 		DirectoryMethodFile *df = (DirectoryMethodFile *) f;
 		size_t		compressed;
@@ -526,7 +542,7 @@ dir_sync(Walfile *f)
 		compressed = LZ4F_flush(df->ctx, df->lz4buf, df->lz4bufsize, NULL);
 		if (LZ4F_isError(compressed))
 		{
-			dir_data->lasterrstring = LZ4F_getErrorName(compressed);
+			f->wwmethod->lasterrstring = LZ4F_getErrorName(compressed);
 			return -1;
 		}
 
@@ -534,7 +550,7 @@ dir_sync(Walfile *f)
 		if (write(df->fd, df->lz4buf, compressed) != compressed)
 		{
 			/* If write didn't set errno, assume problem is no disk space */
-			dir_data->lasterrno = errno ? errno : ENOSPC;
+			f->wwmethod->lasterrno = errno ? errno : ENOSPC;
 			return -1;
 		}
 	}
@@ -542,13 +558,14 @@ dir_sync(Walfile *f)
 
 	r = fsync(((DirectoryMethodFile *) f)->fd);
 	if (r < 0)
-		dir_data->lasterrno = errno;
+		f->wwmethod->lasterrno = errno;
 	return r;
 }
 
 static ssize_t
-dir_get_file_size(const char *pathname)
+dir_get_file_size(WalWriteMethod *wwmethod, const char *pathname)
 {
+	DirectoryMethodData *dir_data = (DirectoryMethodData *) wwmethod;
 	struct stat statbuf;
 	char		tmppath[MAXPGPATH];
 
@@ -557,26 +574,21 @@ dir_get_file_size(const char *pathname)
 
 	if (stat(tmppath, &statbuf) != 0)
 	{
-		dir_data->lasterrno = errno;
+		wwmethod->lasterrno = errno;
 		return -1;
 	}
 
 	return statbuf.st_size;
 }
 
-static pg_compress_algorithm
-dir_compression_algorithm(void)
-{
-	return dir_data->compression_algorithm;
-}
-
 static bool
-dir_existsfile(const char *pathname)
+dir_existsfile(WalWriteMethod *wwmethod, const char *pathname)
 {
+	DirectoryMethodData *dir_data = (DirectoryMethodData *) wwmethod;
 	char		tmppath[MAXPGPATH];
 	int			fd;
 
-	dir_clear_error();
+	clear_error(wwmethod);
 
 	snprintf(tmppath, sizeof(tmppath), "%s/%s",
 			 dir_data->basedir, pathname);
@@ -589,60 +601,54 @@ dir_existsfile(const char *pathname)
 }
 
 static bool
-dir_finish(void)
+dir_finish(WalWriteMethod *wwmethod)
 {
-	dir_clear_error();
+	clear_error(wwmethod);
 
-	if (dir_data->sync)
+	if (wwmethod->sync)
 	{
+		DirectoryMethodData *dir_data = (DirectoryMethodData *) wwmethod;
+
 		/*
 		 * Files are fsynced when they are closed, but we need to fsync the
 		 * directory entry here as well.
 		 */
 		if (fsync_fname(dir_data->basedir, true) != 0)
 		{
-			dir_data->lasterrno = errno;
+			wwmethod->lasterrno = errno;
 			return false;
 		}
 	}
 	return true;
 }
 
+static void
+dir_free(WalWriteMethod *wwmethod)
+{
+	DirectoryMethodData *dir_data = (DirectoryMethodData *) wwmethod;
+
+	pg_free(dir_data->basedir);
+	pg_free(wwmethod);
+}
+
 
 WalWriteMethod *
 CreateWalDirectoryMethod(const char *basedir,
 						 pg_compress_algorithm compression_algorithm,
 						 int compression_level, bool sync)
 {
-	WalWriteMethod *method;
-
-	method = pg_malloc0(sizeof(WalWriteMethod));
-	method->open_for_write = dir_open_for_write;
-	method->write = dir_write;
-	method->get_file_size = dir_get_file_size;
-	method->get_file_name = dir_get_file_name;
-	method->compression_algorithm = dir_compression_algorithm;
-	method->close = dir_close;
-	method->sync = dir_sync;
-	method->existsfile = dir_existsfile;
-	method->finish = dir_finish;
-	method->getlasterror = dir_getlasterror;
-
-	dir_data = pg_malloc0(sizeof(DirectoryMethodData));
-	dir_data->compression_algorithm = compression_algorithm;
-	dir_data->compression_level = compression_level;
-	dir_data->basedir = pg_strdup(basedir);
-	dir_data->sync = sync;
-
-	return method;
-}
-
-void
-FreeWalDirectoryMethod(void)
-{
-	pg_free(dir_data->basedir);
-	pg_free(dir_data);
-	dir_data = NULL;
+	DirectoryMethodData *wwmethod;
+
+	wwmethod = pg_malloc0(sizeof(DirectoryMethodData));
+	*((const WalWriteMethodOps **) &wwmethod->base.ops) =
+		&WalDirectoryMethodOps;
+	wwmethod->base.compression_algorithm = compression_algorithm;
+	wwmethod->base.compression_level = compression_level;
+	wwmethod->base.sync = sync;
+	clear_error(&wwmethod->base);
+	wwmethod->basedir = pg_strdup(basedir);
+
+	return &wwmethod->base;
 }
 
 
@@ -651,6 +657,33 @@ FreeWalDirectoryMethod(void)
  *-------------------------------------------------------------------------
  */
 
+static Walfile *tar_open_for_write(WalWriteMethod *wwmethod,
+								   const char *pathname,
+								   const char *temp_suffix,
+								   size_t pad_to_size);
+static int tar_close(Walfile *f, WalCloseMethod method);
+static bool tar_existsfile(WalWriteMethod *wwmethod, const char *pathname);
+static ssize_t tar_get_file_size(WalWriteMethod *wwmethod,
+								 const char *pathname);
+static char *tar_get_file_name(WalWriteMethod *wwmethod,
+							   const char *pathname, const char *temp_suffix);
+static ssize_t tar_write(Walfile *f, const void *buf, size_t count);
+static int tar_sync(Walfile *f);
+static bool tar_finish(WalWriteMethod *wwmethod);
+static void tar_free(WalWriteMethod *wwmethod);
+
+const WalWriteMethodOps WalTarMethodOps = {
+	.open_for_write = tar_open_for_write,
+	.close = tar_close,
+	.existsfile = tar_existsfile,
+	.get_file_size = tar_get_file_size,
+	.get_file_name = tar_get_file_name,
+	.write = tar_write,
+	.sync = tar_sync,
+	.finish = tar_finish,
+	.free = tar_free
+};
+
 typedef struct TarMethodFile
 {
 	Walfile		base;
@@ -661,37 +694,20 @@ typedef struct TarMethodFile
 
 typedef struct TarMethodData
 {
+	WalWriteMethod	base;
 	char	   *tarfilename;
 	int			fd;
-	pg_compress_algorithm compression_algorithm;
-	int			compression_level;
-	bool		sync;
 	TarMethodFile *currentfile;
-	const char *lasterrstring;	/* if set, takes precedence over lasterrno */
-	int			lasterrno;
 #ifdef HAVE_LIBZ
 	z_streamp	zp;
 	void	   *zlibOut;
 #endif
 } TarMethodData;
-static TarMethodData *tar_data = NULL;
-
-#define tar_clear_error() \
-	(tar_data->lasterrstring = NULL, tar_data->lasterrno = 0)
-#define tar_set_error(msg) \
-	(tar_data->lasterrstring = _(msg))
-
-static const char *
-tar_getlasterror(void)
-{
-	if (tar_data->lasterrstring)
-		return tar_data->lasterrstring;
-	return strerror(tar_data->lasterrno);
-}
 
 #ifdef HAVE_LIBZ
 static bool
-tar_write_compressed_data(void *buf, size_t count, bool flush)
+tar_write_compressed_data(TarMethodData *tar_data, void *buf, size_t count,
+						  bool flush)
 {
 	tar_data->zp->next_in = buf;
 	tar_data->zp->avail_in = count;
@@ -703,7 +719,7 @@ tar_write_compressed_data(void *buf, size_t count, bool flush)
 		r = deflate(tar_data->zp, flush ? Z_FINISH : Z_NO_FLUSH);
 		if (r == Z_STREAM_ERROR)
 		{
-			tar_set_error("could not compress data");
+			tar_data->base.lasterrstring = "could not compress data";
 			return false;
 		}
 
@@ -715,7 +731,7 @@ tar_write_compressed_data(void *buf, size_t count, bool flush)
 			if (write(tar_data->fd, tar_data->zlibOut, len) != len)
 			{
 				/* If write didn't set errno, assume problem is no disk space */
-				tar_data->lasterrno = errno ? errno : ENOSPC;
+				tar_data->base.lasterrno = errno ? errno : ENOSPC;
 				return false;
 			}
 
@@ -732,7 +748,7 @@ tar_write_compressed_data(void *buf, size_t count, bool flush)
 		/* Reset the stream for writing */
 		if (deflateReset(tar_data->zp) != Z_OK)
 		{
-			tar_set_error("could not reset compression stream");
+			tar_data->base.lasterrstring = "could not reset compression stream";
 			return false;
 		}
 	}
@@ -744,29 +760,31 @@ tar_write_compressed_data(void *buf, size_t count, bool flush)
 static ssize_t
 tar_write(Walfile *f, const void *buf, size_t count)
 {
+	TarMethodData *tar_data = (TarMethodData *) f->wwmethod;
 	ssize_t		r;
 
 	Assert(f != NULL);
-	tar_clear_error();
+	clear_error(f->wwmethod);
 
 	/* Tarfile will always be positioned at the end */
-	if (tar_data->compression_algorithm == PG_COMPRESSION_NONE)
+	if (f->wwmethod->compression_algorithm == PG_COMPRESSION_NONE)
 	{
 		errno = 0;
 		r = write(tar_data->fd, buf, count);
 		if (r != count)
 		{
 			/* If write didn't set errno, assume problem is no disk space */
-			tar_data->lasterrno = errno ? errno : ENOSPC;
+			f->wwmethod->lasterrno = errno ? errno : ENOSPC;
 			return -1;
 		}
 		f->currpos += r;
 		return r;
 	}
 #ifdef HAVE_LIBZ
-	else if (tar_data->compression_algorithm == PG_COMPRESSION_GZIP)
+	else if (f->wwmethod->compression_algorithm == PG_COMPRESSION_GZIP)
 	{
-		if (!tar_write_compressed_data(unconstify(void *, buf), count, false))
+		if (!tar_write_compressed_data(tar_data, unconstify(void *, buf),
+									   count, false))
 			return -1;
 		f->currpos += count;
 		return count;
@@ -775,7 +793,7 @@ tar_write(Walfile *f, const void *buf, size_t count)
 	else
 	{
 		/* Can't happen - compression enabled with no method set */
-		tar_data->lasterrno = ENOSYS;
+		f->wwmethod->lasterrno = ENOSYS;
 		return -1;
 	}
 }
@@ -801,7 +819,8 @@ tar_write_padding_data(TarMethodFile *f, size_t bytes)
 }
 
 static char *
-tar_get_file_name(const char *pathname, const char *temp_suffix)
+tar_get_file_name(WalWriteMethod *wwmethod, const char *pathname,
+				  const char *temp_suffix)
 {
 	char	   *filename = pg_malloc0(MAXPGPATH * sizeof(char));
 
@@ -812,11 +831,13 @@ tar_get_file_name(const char *pathname, const char *temp_suffix)
 }
 
 static Walfile *
-tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_size)
+tar_open_for_write(WalWriteMethod *wwmethod, const char *pathname,
+				   const char *temp_suffix, size_t pad_to_size)
 {
+	TarMethodData *tar_data = (TarMethodData *) wwmethod;
 	char	   *tmppath;
 
-	tar_clear_error();
+	clear_error(wwmethod);
 
 	if (tar_data->fd < 0)
 	{
@@ -828,12 +849,12 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 							pg_file_create_mode);
 		if (tar_data->fd < 0)
 		{
-			tar_data->lasterrno = errno;
+			wwmethod->lasterrno = errno;
 			return NULL;
 		}
 
 #ifdef HAVE_LIBZ
-		if (tar_data->compression_algorithm == PG_COMPRESSION_GZIP)
+		if (wwmethod->compression_algorithm == PG_COMPRESSION_GZIP)
 		{
 			tar_data->zp = (z_streamp) pg_malloc(sizeof(z_stream));
 			tar_data->zp->zalloc = Z_NULL;
@@ -847,12 +868,13 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 			 * default 15 for the windowBits parameter makes the output be
 			 * gzip instead of zlib.
 			 */
-			if (deflateInit2(tar_data->zp, tar_data->compression_level,
+			if (deflateInit2(tar_data->zp, wwmethod->compression_level,
 							 Z_DEFLATED, 15 + 16, 8, Z_DEFAULT_STRATEGY) != Z_OK)
 			{
 				pg_free(tar_data->zp);
 				tar_data->zp = NULL;
-				tar_set_error("could not initialize compression library");
+				wwmethod->lasterrstring =
+					"could not initialize compression library";
 				return NULL;
 			}
 		}
@@ -863,13 +885,15 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 
 	if (tar_data->currentfile != NULL)
 	{
-		tar_set_error("implementation error: tar files can't have more than one open file");
+		wwmethod->lasterrstring =
+			"implementation error: tar files can't have more than one open file";
 		return NULL;
 	}
 
 	tar_data->currentfile = pg_malloc0(sizeof(TarMethodFile));
+	tar_data->currentfile->base.wwmethod = wwmethod;
 
-	tmppath = tar_get_file_name(pathname, temp_suffix);
+	tmppath = tar_get_file_name(wwmethod, 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)
@@ -877,23 +901,24 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 		pg_free(tar_data->currentfile);
 		pg_free(tmppath);
 		tar_data->currentfile = NULL;
-		tar_set_error("could not create tar header");
+		wwmethod->lasterrstring = "could not create tar header";
 		return NULL;
 	}
 
 	pg_free(tmppath);
 
 #ifdef HAVE_LIBZ
-	if (tar_data->compression_algorithm == PG_COMPRESSION_GZIP)
+	if (wwmethod->compression_algorithm == PG_COMPRESSION_GZIP)
 	{
 		/* Flush existing data */
-		if (!tar_write_compressed_data(NULL, 0, true))
+		if (!tar_write_compressed_data(tar_data, NULL, 0, true))
 			return NULL;
 
 		/* Turn off compression for header */
 		if (deflateParams(tar_data->zp, 0, 0) != Z_OK)
 		{
-			tar_set_error("could not change compression parameters");
+			wwmethod->lasterrstring =
+				"could not change compression parameters";
 			return NULL;
 		}
 	}
@@ -902,38 +927,38 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 	tar_data->currentfile->ofs_start = lseek(tar_data->fd, 0, SEEK_CUR);
 	if (tar_data->currentfile->ofs_start == -1)
 	{
-		tar_data->lasterrno = errno;
+		wwmethod->lasterrno = errno;
 		pg_free(tar_data->currentfile);
 		tar_data->currentfile = NULL;
 		return NULL;
 	}
 	tar_data->currentfile->base.currpos = 0;
 
-	if (tar_data->compression_algorithm == PG_COMPRESSION_NONE)
+	if (wwmethod->compression_algorithm == PG_COMPRESSION_NONE)
 	{
 		errno = 0;
 		if (write(tar_data->fd, tar_data->currentfile->header,
 				  TAR_BLOCK_SIZE) != TAR_BLOCK_SIZE)
 		{
 			/* If write didn't set errno, assume problem is no disk space */
-			tar_data->lasterrno = errno ? errno : ENOSPC;
+			wwmethod->lasterrno = errno ? errno : ENOSPC;
 			pg_free(tar_data->currentfile);
 			tar_data->currentfile = NULL;
 			return NULL;
 		}
 	}
 #ifdef HAVE_LIBZ
-	else if (tar_data->compression_algorithm == PG_COMPRESSION_GZIP)
+	else if (wwmethod->compression_algorithm == PG_COMPRESSION_GZIP)
 	{
 		/* Write header through the zlib APIs but with no compression */
-		if (!tar_write_compressed_data(tar_data->currentfile->header,
+		if (!tar_write_compressed_data(tar_data, tar_data->currentfile->header,
 									   TAR_BLOCK_SIZE, true))
 			return NULL;
 
 		/* Re-enable compression for the rest of the file */
-		if (deflateParams(tar_data->zp, tar_data->compression_level, 0) != Z_OK)
+		if (deflateParams(tar_data->zp, wwmethod->compression_level, 0) != Z_OK)
 		{
-			tar_set_error("could not change compression parameters");
+			wwmethod->lasterrstring = "could not change compression parameters";
 			return NULL;
 		}
 	}
@@ -953,7 +978,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_algorithm == PG_COMPRESSION_NONE)
+		if (wwmethod->compression_algorithm == PG_COMPRESSION_NONE)
 		{
 			/* Uncompressed, so pad now */
 			if (!tar_write_padding_data(tar_data->currentfile, pad_to_size))
@@ -963,7 +988,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 					  tar_data->currentfile->ofs_start + TAR_BLOCK_SIZE,
 					  SEEK_SET) != tar_data->currentfile->ofs_start + TAR_BLOCK_SIZE)
 			{
-				tar_data->lasterrno = errno;
+				wwmethod->lasterrno = errno;
 				return NULL;
 			}
 
@@ -975,42 +1000,37 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 }
 
 static ssize_t
-tar_get_file_size(const char *pathname)
+tar_get_file_size(WalWriteMethod *wwmethod, const char *pathname)
 {
-	tar_clear_error();
+	clear_error(wwmethod);
 
 	/* Currently not used, so not supported */
-	tar_data->lasterrno = ENOSYS;
+	wwmethod->lasterrno = ENOSYS;
 	return -1;
 }
 
-static pg_compress_algorithm
-tar_compression_algorithm(void)
-{
-	return tar_data->compression_algorithm;
-}
-
 static int
 tar_sync(Walfile *f)
 {
+	TarMethodData *tar_data = (TarMethodData *) f->wwmethod;
 	int			r;
 
 	Assert(f != NULL);
-	tar_clear_error();
+	clear_error(f->wwmethod);
 
-	if (!tar_data->sync)
+	if (!f->wwmethod->sync)
 		return 0;
 
 	/*
 	 * 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_algorithm != PG_COMPRESSION_NONE)
+	if (f->wwmethod->compression_algorithm != PG_COMPRESSION_NONE)
 		return 0;
 
 	r = fsync(tar_data->fd);
 	if (r < 0)
-		tar_data->lasterrno = errno;
+		f->wwmethod->lasterrno = errno;
 	return r;
 }
 
@@ -1019,16 +1039,17 @@ tar_close(Walfile *f, WalCloseMethod method)
 {
 	ssize_t		filesize;
 	int			padding;
+	TarMethodData *tar_data = (TarMethodData *) f->wwmethod;
 	TarMethodFile *tf = (TarMethodFile *) f;
 
 	Assert(f != NULL);
-	tar_clear_error();
+	clear_error(f->wwmethod);
 
 	if (method == CLOSE_UNLINK)
 	{
-		if (tar_data->compression_algorithm != PG_COMPRESSION_NONE)
+		if (f->wwmethod->compression_algorithm != PG_COMPRESSION_NONE)
 		{
-			tar_set_error("unlink not supported with compression");
+			f->wwmethod->lasterrstring = "unlink not supported with compression";
 			return -1;
 		}
 
@@ -1039,7 +1060,7 @@ tar_close(Walfile *f, WalCloseMethod method)
 		 */
 		if (ftruncate(tar_data->fd, tf->ofs_start) != 0)
 		{
-			tar_data->lasterrno = errno;
+			f->wwmethod->lasterrno = errno;
 			return -1;
 		}
 
@@ -1057,7 +1078,7 @@ tar_close(Walfile *f, WalCloseMethod method)
 	 */
 	if (tf->pad_to_size)
 	{
-		if (tar_data->compression_algorithm == PG_COMPRESSION_GZIP)
+		if (f->wwmethod->compression_algorithm == PG_COMPRESSION_GZIP)
 		{
 			/*
 			 * A compressed tarfile is padded on close since we cannot know
@@ -1097,10 +1118,10 @@ tar_close(Walfile *f, WalCloseMethod method)
 
 
 #ifdef HAVE_LIBZ
-	if (tar_data->compression_algorithm == PG_COMPRESSION_GZIP)
+	if (f->wwmethod->compression_algorithm == PG_COMPRESSION_GZIP)
 	{
 		/* Flush the current buffer */
-		if (!tar_write_compressed_data(NULL, 0, true))
+		if (!tar_write_compressed_data(tar_data, NULL, 0, true))
 			return -1;
 	}
 #endif
@@ -1123,38 +1144,38 @@ tar_close(Walfile *f, WalCloseMethod method)
 	print_tar_number(&(tf->header[148]), 8, tarChecksum(((TarMethodFile *) f)->header));
 	if (lseek(tar_data->fd, tf->ofs_start, SEEK_SET) != ((TarMethodFile *) f)->ofs_start)
 	{
-		tar_data->lasterrno = errno;
+		f->wwmethod->lasterrno = errno;
 		return -1;
 	}
-	if (tar_data->compression_algorithm == PG_COMPRESSION_NONE)
+	if (f->wwmethod->compression_algorithm == PG_COMPRESSION_NONE)
 	{
 		errno = 0;
 		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 */
-			tar_data->lasterrno = errno ? errno : ENOSPC;
+			f->wwmethod->lasterrno = errno ? errno : ENOSPC;
 			return -1;
 		}
 	}
 #ifdef HAVE_LIBZ
-	else if (tar_data->compression_algorithm == PG_COMPRESSION_GZIP)
+	else if (f->wwmethod->compression_algorithm == PG_COMPRESSION_GZIP)
 	{
 		/* Turn off compression */
 		if (deflateParams(tar_data->zp, 0, 0) != Z_OK)
 		{
-			tar_set_error("could not change compression parameters");
+			f->wwmethod->lasterrstring = "could not change compression parameters";
 			return -1;
 		}
 
 		/* Overwrite the header, assuming the size will be the same */
-		if (!tar_write_compressed_data(tar_data->currentfile->header,
+		if (!tar_write_compressed_data(tar_data, tar_data->currentfile->header,
 									   TAR_BLOCK_SIZE, true))
 			return -1;
 
 		/* Turn compression back on */
-		if (deflateParams(tar_data->zp, tar_data->compression_level, 0) != Z_OK)
+		if (deflateParams(tar_data->zp, f->wwmethod->compression_level, 0) != Z_OK)
 		{
-			tar_set_error("could not change compression parameters");
+			f->wwmethod->lasterrstring = "could not change compression parameters";
 			return -1;
 		}
 	}
@@ -1168,7 +1189,7 @@ tar_close(Walfile *f, WalCloseMethod method)
 	/* Move file pointer back down to end, so we can write the next file */
 	if (lseek(tar_data->fd, 0, SEEK_END) < 0)
 	{
-		tar_data->lasterrno = errno;
+		f->wwmethod->lasterrno = errno;
 		return -1;
 	}
 
@@ -1177,7 +1198,7 @@ tar_close(Walfile *f, WalCloseMethod method)
 	{
 		/* XXX this seems pretty bogus; why is only this case fatal? */
 		pg_fatal("could not fsync file \"%s\": %s",
-				 tf->base.pathname, tar_getlasterror());
+				 tf->base.pathname, GetLastWalMethodError(f->wwmethod));
 	}
 
 	/* Clean up and done */
@@ -1189,19 +1210,20 @@ tar_close(Walfile *f, WalCloseMethod method)
 }
 
 static bool
-tar_existsfile(const char *pathname)
+tar_existsfile(WalWriteMethod *wwmethod, const char *pathname)
 {
-	tar_clear_error();
+	clear_error(wwmethod);
 	/* We only deal with new tarfiles, so nothing externally created exists */
 	return false;
 }
 
 static bool
-tar_finish(void)
+tar_finish(WalWriteMethod *wwmethod)
 {
+	TarMethodData *tar_data = (TarMethodData *) wwmethod;
 	char		zerobuf[1024] = {0};
 
-	tar_clear_error();
+	clear_error(wwmethod);
 
 	if (tar_data->currentfile)
 	{
@@ -1210,20 +1232,21 @@ tar_finish(void)
 	}
 
 	/* A tarfile always ends with two empty blocks */
-	if (tar_data->compression_algorithm == PG_COMPRESSION_NONE)
+	if (wwmethod->compression_algorithm == PG_COMPRESSION_NONE)
 	{
 		errno = 0;
 		if (write(tar_data->fd, zerobuf, sizeof(zerobuf)) != sizeof(zerobuf))
 		{
 			/* If write didn't set errno, assume problem is no disk space */
-			tar_data->lasterrno = errno ? errno : ENOSPC;
+			wwmethod->lasterrno = errno ? errno : ENOSPC;
 			return false;
 		}
 	}
 #ifdef HAVE_LIBZ
-	else if (tar_data->compression_algorithm == PG_COMPRESSION_GZIP)
+	else if (wwmethod->compression_algorithm == PG_COMPRESSION_GZIP)
 	{
-		if (!tar_write_compressed_data(zerobuf, sizeof(zerobuf), false))
+		if (!tar_write_compressed_data(tar_data, zerobuf, sizeof(zerobuf),
+									   false))
 			return false;
 
 		/* Also flush all data to make sure the gzip stream is finished */
@@ -1237,7 +1260,7 @@ tar_finish(void)
 
 			if (r == Z_STREAM_ERROR)
 			{
-				tar_set_error("could not compress data");
+				wwmethod->lasterrstring = "could not compress data";
 				return false;
 			}
 			if (tar_data->zp->avail_out < ZLIB_OUT_SIZE)
@@ -1251,7 +1274,7 @@ tar_finish(void)
 					 * If write didn't set errno, assume problem is no disk
 					 * space.
 					 */
-					tar_data->lasterrno = errno ? errno : ENOSPC;
+					wwmethod->lasterrno = errno ? errno : ENOSPC;
 					return false;
 				}
 			}
@@ -1261,7 +1284,7 @@ tar_finish(void)
 
 		if (deflateEnd(tar_data->zp) != Z_OK)
 		{
-			tar_set_error("could not close compression stream");
+			wwmethod->lasterrstring = "could not close compression stream";
 			return false;
 		}
 	}
@@ -1273,29 +1296,29 @@ tar_finish(void)
 	}
 
 	/* sync the empty blocks as well, since they're after the last file */
-	if (tar_data->sync)
+	if (wwmethod->sync)
 	{
 		if (fsync(tar_data->fd) != 0)
 		{
-			tar_data->lasterrno = errno;
+			wwmethod->lasterrno = errno;
 			return false;
 		}
 	}
 
 	if (close(tar_data->fd) != 0)
 	{
-		tar_data->lasterrno = errno;
+		wwmethod->lasterrno = errno;
 		return false;
 	}
 
 	tar_data->fd = -1;
 
-	if (tar_data->sync)
+	if (wwmethod->sync)
 	{
 		if (fsync_fname(tar_data->tarfilename, false) != 0 ||
 			fsync_parent_path(tar_data->tarfilename) != 0)
 		{
-			tar_data->lasterrno = errno;
+			wwmethod->lasterrno = errno;
 			return false;
 		}
 	}
@@ -1303,6 +1326,19 @@ tar_finish(void)
 	return true;
 }
 
+static void
+tar_free(WalWriteMethod *wwmethod)
+{
+	TarMethodData *tar_data = (TarMethodData *) wwmethod;
+
+	pg_free(tar_data->tarfilename);
+#ifdef HAVE_LIBZ
+	if (wwmethod->compression_algorithm == PG_COMPRESSION_GZIP)
+		pg_free(tar_data->zlibOut);
+#endif
+	pg_free(wwmethod);
+}
+
 /*
  * The argument compression_algorithm is currently ignored. It is in place for
  * symmetry with CreateWalDirectoryMethod which uses it for distinguishing
@@ -1314,45 +1350,33 @@ CreateWalTarMethod(const char *tarbase,
 				   pg_compress_algorithm compression_algorithm,
 				   int compression_level, bool sync)
 {
-	WalWriteMethod *method;
+	TarMethodData *wwmethod;
 	const char *suffix = (compression_algorithm == PG_COMPRESSION_GZIP) ?
 	".tar.gz" : ".tar";
 
-	method = pg_malloc0(sizeof(WalWriteMethod));
-	method->open_for_write = tar_open_for_write;
-	method->write = tar_write;
-	method->get_file_size = tar_get_file_size;
-	method->get_file_name = tar_get_file_name;
-	method->compression_algorithm = tar_compression_algorithm;
-	method->close = tar_close;
-	method->sync = tar_sync;
-	method->existsfile = tar_existsfile;
-	method->finish = tar_finish;
-	method->getlasterror = tar_getlasterror;
-
-	tar_data = pg_malloc0(sizeof(TarMethodData));
-	tar_data->tarfilename = pg_malloc0(strlen(tarbase) + strlen(suffix) + 1);
-	sprintf(tar_data->tarfilename, "%s%s", tarbase, suffix);
-	tar_data->fd = -1;
-	tar_data->compression_algorithm = compression_algorithm;
-	tar_data->compression_level = compression_level;
-	tar_data->sync = sync;
+	wwmethod = pg_malloc0(sizeof(TarMethodData));
+	*((const WalWriteMethodOps **) &wwmethod->base.ops) =
+		&WalTarMethodOps;
+	wwmethod->base.compression_algorithm = compression_algorithm;
+	wwmethod->base.compression_level = compression_level;
+	wwmethod->base.sync = sync;
+	clear_error(&wwmethod->base);
+
+	wwmethod->tarfilename = pg_malloc0(strlen(tarbase) + strlen(suffix) + 1);
+	sprintf(wwmethod->tarfilename, "%s%s", tarbase, suffix);
+	wwmethod->fd = -1;
 #ifdef HAVE_LIBZ
 	if (compression_algorithm == PG_COMPRESSION_GZIP)
-		tar_data->zlibOut = (char *) pg_malloc(ZLIB_OUT_SIZE + 1);
+		wwmethod->zlibOut = (char *) pg_malloc(ZLIB_OUT_SIZE + 1);
 #endif
 
-	return method;
+	return &wwmethod->base;
 }
 
-void
-FreeWalTarMethod(void)
+const char *
+GetLastWalMethodError(WalWriteMethod *wwmethod)
 {
-	pg_free(tar_data->tarfilename);
-#ifdef HAVE_LIBZ
-	if (tar_data->compression_algorithm == PG_COMPRESSION_GZIP)
-		pg_free(tar_data->zlibOut);
-#endif
-	pg_free(tar_data);
-	tar_data = NULL;
+	if (wwmethod->lasterrstring)
+		return wwmethod->lasterrstring;
+	return strerror(wwmethod->lasterrno);
 }
diff --git a/src/bin/pg_basebackup/walmethods.h b/src/bin/pg_basebackup/walmethods.h
index cf5ed87fbe..0d7728d086 100644
--- a/src/bin/pg_basebackup/walmethods.h
+++ b/src/bin/pg_basebackup/walmethods.h
@@ -16,6 +16,7 @@ typedef struct WalWriteMethod WalWriteMethod;
 
 typedef struct
 {
+	WalWriteMethod *wwmethod;
 	off_t		currpos;
 	char	   *pathname;
 	/*
@@ -34,16 +35,9 @@ typedef enum
 } WalCloseMethod;
 
 /*
- * A WalWriteMethod structure represents the different methods used
- * to write the streaming WAL as it's received.
- *
- * All methods that have a failure return indicator will set state
- * allowing the getlasterror() method to return a suitable message.
- * Commonly, errno is this state (or part of it); so callers must take
- * care not to clobber errno between a failed method call and use of
- * getlasterror() to retrieve the message.
+ * Table of callbacks for a WalWriteMethod.
  */
-struct WalWriteMethod
+typedef struct WalWriteMethodOps
 {
 	/*
 	 * Open a target file. Returns Walfile, or NULL if open failed. If a temp
@@ -51,7 +45,7 @@ struct WalWriteMethod
 	 * automatically renamed in close(). If pad_to_size is specified, the file
 	 * will be padded with NUL up to that size, if supported by the Walmethod.
 	 */
-	Walfile	   *(*open_for_write) (const char *pathname, const char *temp_suffix, size_t pad_to_size);
+	Walfile	   *(*open_for_write) (WalWriteMethod *wwmethod, const char *pathname, const char *temp_suffix, size_t pad_to_size);
 
 	/*
 	 * Close an open Walfile, using one or more methods for handling automatic
@@ -60,19 +54,16 @@ struct WalWriteMethod
 	int			(*close) (Walfile *f, WalCloseMethod method);
 
 	/* Check if a file exist */
-	bool		(*existsfile) (const char *pathname);
+	bool		(*existsfile) (WalWriteMethod *wwmethod, const char *pathname);
 
 	/* Return the size of a file, or -1 on failure. */
-	ssize_t		(*get_file_size) (const char *pathname);
+	ssize_t		(*get_file_size) (WalWriteMethod *wwmethod, const char *pathname);
 
 	/*
 	 * Return the name of the current file to work on in pg_malloc()'d string,
 	 * without the base directory.  This is useful for logging.
 	 */
-	char	   *(*get_file_name) (const char *pathname, const char *temp_suffix);
-
-	/* Returns the compression method */
-	pg_compress_algorithm (*compression_algorithm) (void);
+	char	   *(*get_file_name) (WalWriteMethod *wwmethod, const char *pathname, const char *temp_suffix);
 
 	/*
 	 * Write count number of bytes to the file, and return the number of bytes
@@ -91,10 +82,37 @@ struct WalWriteMethod
 	 * close/write/sync of shared resources succeeded, otherwise returns false
 	 * (but the resources are still closed).
 	 */
-	bool		(*finish) (void);
+	bool		(*finish) (WalWriteMethod *wwmethod);
 
-	/* Return a text for the last error in this Walfile */
-	const char *(*getlasterror) (void);
+	/*
+	 * Free subsidiary data associated with the WalWriteMethod, and the
+	 * WalWriteMethod itself.
+	 */
+	void		(*free) (WalWriteMethod *wwmethod);
+} WalWriteMethodOps;
+
+/*
+ * A WalWriteMethod structure represents a way of writing streaming WAL as
+ * it's received.
+ *
+ * All methods that have a failure return indicator will set lasterrstring
+ * or lasterrno (the former takes precedence) so that the caller can signal
+ * a suitable error.
+ */
+struct WalWriteMethod
+{
+	const WalWriteMethodOps *ops;
+	pg_compress_algorithm compression_algorithm;
+	int			compression_level;
+	bool		sync;
+	const char *lasterrstring;	/* if set, takes precedence over lasterrno */
+	int			lasterrno;
+	/*
+	 * MORE DATA FOLLOWS AT END OF STRUCT
+	 *
+	 * Each WalWriteMethod is expected to embed this as the first member of
+	 * a larger struct with method-specific fields following.
+	 */
 };
 
 /*
@@ -111,6 +129,4 @@ WalWriteMethod *CreateWalTarMethod(const char *tarbase,
 								   pg_compress_algorithm compression_algo,
 								   int compression, bool sync);
 
-/* Cleanup routines for previously-created methods */
-void		FreeWalDirectoryMethod(void);
-void		FreeWalTarMethod(void);
+const char *GetLastWalMethodError(WalWriteMethod *wwmethod);
-- 
2.24.3 (Apple Git-128)

Peter Eisentraut

peter.eisentraut@enterprisedb.com

over 3 years ago

In reply to: Robert Haas (#1)

Re: walmethods.c/h are doing some strange things

On 02.09.22 17:52, Robert Haas wrote:

Attached are a couple of hastily-written patches implementing this.
There might be good arguments for more thoroughly renaming some of the
things these patches touch, but I thought that doing any more renaming
would make it less clear what the core of the change is, so I'm
posting it like this for now. One thing I noticed while writing these
patches is that the existing code isn't very clear about whether
"Walfile" is supposed to be an abstraction for a pointer to the
implementation-specific struct, or the struct itself. From looking at
walmethods.h, you'd think it's a pointer to the struct, because we
declare typedef void *Walfile. walmethods.c agrees, but receivelog.c
takes a different view, declaring all of its variables as type
"Walfile *". This doesn't cause a compiler error because void * is
just as interchangeable with void ** as it is with DirectoryMethodFile
* or TarMethodFile *, but I think it is clearly a mistake, and the
approach I'm proposing here makes such mistakes more difficult to
make.

This direction does make sense IMO.

velagandula sravan kumar

sravan_velag@yahoo.com

over 3 years ago

In reply to: Robert Haas (#1)

Re: walmethods.c/h are doing some strange things

From: Robert Haas <robertmhaas@gmail.com>
Date: Friday, 2 September 2022 at 9:23 PM
To: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: walmethods.c/h are doing some strange things
Hi,

+ 1 on being able to restrict making such mistakes. I had a quick look at the patch
and the refactoring makes sense.

Thanks,
Sravan Kumar
www.enterprisedb.com

Thanks,

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

Kyotaro Horiguchi

horikyota.ntt@gmail.com

over 3 years ago

In reply to: Robert Haas (#1)

Re: walmethods.c/h are doing some strange things

At Fri, 2 Sep 2022 11:52:38 -0400, Robert Haas <robertmhaas@gmail.com> wrote in

that type that can ever exist, and the pointer to that object is
stored in a global variable managed by walmethods.c. So whereas in
other cases we give you the object and then a way to get the
corresponding set of callbacks, here we only give you the callbacks,
and we therefore have to impose the artificial restriction that there
can only ever be one object.

Makes sense to me.

There is no real benefit in having the same variable in two different
structs and having to access it via a callback when we could just put
it into a common struct and access it directly. There's also a
compression_algorithm() method which has exactly the same issue,

though that is an overall property of the WalWriteMethod rather than a
per-Walfile property. There's also a getlasterr callback which is
basically just duplicate code across the two implementations; we could
unify that code. There's also a global variable current_walfile_name[]
in receivelog.c which only needs to exist because the file name is
inconveniently hidden inside the WalWriteMethod abstraction layer; we
can just make it visible.

Sounds sensible.

Attached are a couple of hastily-written patches implementing this.

patches is that the existing code isn't very clear about whether
"Walfile" is supposed to be an abstraction for a pointer to the
implementation-specific struct, or the struct itself. From looking at
walmethods.h, you'd think it's a pointer to the struct, because we
declare typedef void *Walfile. walmethods.c agrees, but receivelog.c
takes a different view, declaring all of its variables as type
"Walfile *". This doesn't cause a compiler error because void * is
just as interchangeable with void ** as it is with DirectoryMethodFile
* or TarMethodFile *, but I think it is clearly a mistake, and the
approach I'm proposing here makes such mistakes more difficult to
make.

+1. I remember I thought the same thing when I was faced with the
code before.

Aside from the stuff that I am complaining about here which is mostly
stylistic, I think that the division of labor between receivelog.c and
walmethods.c is questionable in a number of ways. There are things
which are specific to one walmethod or the other that are handled in
the common code (receivelog.c) rather than the type-specific code
(walmethod.c), and in general it feels like receivelog.c knows way too
much about what is really happening beneath the abstraction layer that
walmethods.c supposedly creates. This comment is one of the clearer
examples of this:

/*
* 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. 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.
*/

There's nothing generic here. We're not describing an algorithm that
could be used with any walmethod that might exist now or in the
future. We're describing something that will produce the right result
given the two walmethods we actually have and the actual behavior of
the callbacks of each one. I don't really know what to do about this
part of the problem; these pieces of code are deeply intertwined in
complex ways that don't seem simple to untangle. Maybe I'll have a

I agree to the view. That part seems to be a part of
open_for_write()'s body functions. But, I'm not sure how we untangle
them at a glance, too. In the first place, I'm not sure why we need
to do that despite the file going to be overwritten from the
beginning, though..

better idea later, or perhaps someone else will. For now, I'd like to
get some thoughts on the attached refactoring patches that deal with
some more superficial aspects of the problem.

regares.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Robert Haas

robertmhaas@gmail.com

over 3 years ago

In reply to: Kyotaro Horiguchi (#4)

Re: walmethods.c/h are doing some strange things

On Thu, Sep 15, 2022 at 9:39 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Fri, 2 Sep 2022 11:52:38 -0400, Robert Haas <robertmhaas@gmail.com> wrote in

that type that can ever exist, and the pointer to that object is
stored in a global variable managed by walmethods.c. So whereas in
other cases we give you the object and then a way to get the
corresponding set of callbacks, here we only give you the callbacks,
and we therefore have to impose the artificial restriction that there
can only ever be one object.

Makes sense to me.

OK, I have committed the patches. Before doing that, I fixed a couple
of bugs in the first one, and did a little bit of rebasing of the
second one.

I agree to the view. That part seems to be a part of
open_for_write()'s body functions. But, I'm not sure how we untangle
them at a glance, too. In the first place, I'm not sure why we need
to do that despite the file going to be overwritten from the
beginning, though..

I suspect that we're going to have to untangle things bit by bit.

One place where I think we might be able to improve things is with the
handling of compression suffixes (e.g. .gz, .lz4) and temp suffixes
(e.g. .partial, .tmp). At present, responsibility for adding these
suffixes to pathnames is spread across receivelog.c and walmethods.c
in a way that, to me, looks pretty random. It's not exactly clear to
me what the best design is here right now, but I think either (A)
receivelog.c should take full responsibility for computing the exact
filename and walmethods.c should just blindly write the data into the
exact filename it's given, or else (B) receivelog.c should take no
responsibility for pathname construction and the fact that there is
pathname munging happening should be hidden inside walmethods.c. Right
now, walmethods.c is doing pathname munging, but the munging is
visible from receivelog.c, so the responsibility is spread across the
two files rather than being the sole responsibility of either.

What's also a little bit aggravating about this is that it doesn't
feel like we're accomplishing all that much code reuse here.
walmethods.c and receivelog.c are shared only between pg_basebackup
and pg_receivewal, but the tar method is used only by pg_basebackup.
The directory mode is shared, but actually the two programs need a
bunch of different things. pg_recievelog needs a bunch of logic to
deal with the possibility that it is receiving data into an existing
directory and that this directory might at any time start to be used
to feed a standby, or used for PITR. That's not an issue for
pg_basebackup: if it fails, the whole directory will be removed, so
there is no need to worry about fsyncs or padding or overwriting
existing files. On the other hand, pg_basebackup needs a bunch of
logic to create a .done file for each WAL segment which is not
required in the case of pg_receivewal. It feels like we've got as much
conditional logic as we do common logic...

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