Fix reversed check of return value from sync

While at it also update the comments in walmethods.h to make it less
likely for mistakes like this to appear in the future (thanks to Tom for
improvements to the comments).

And finally, in passing change the return type of walmethod.getlasterror
to being const, also per suggestion from Tom.

Author: mhagander

src/bin/pg_basebackup/receivelog.c

@@ -132,8 +132,11 @@ open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
 			}
 
 			/* fsync file in case of a previous crash */
-			if (!stream->walmethod->sync(f))
+			if (stream->walmethod->sync(f) != 0)
 			{
+				fprintf(stderr,
+						_("%s: could not sync existing transaction log file \"%s\": %s\n"),
+						progname, fn, stream->walmethod->getlasterror());
 				stream->walmethod->close(f, CLOSE_UNLINK);
 				return false;
 			}

src/bin/pg_basebackup/walmethods.c

@@ -61,7 +61,7 @@ typedef struct DirectoryMethodFile
 #endif
 }	DirectoryMethodFile;
 
-static char *
+static const char *
 dir_getlasterror(void)
 {
 	/* Directory method always sets errno, so just use strerror */
@@ -406,7 +406,7 @@ static TarMethodData *tar_data = NULL;
 #define tar_clear_error() tar_data->lasterror[0] = '\0'
 #define tar_set_error(msg) strlcpy(tar_data->lasterror, msg, sizeof(tar_data->lasterror))
 
-static char *
+static const char *
 tar_getlasterror(void)
 {
 	/*

src/bin/pg_basebackup/walmethods.h

@@ -19,19 +19,63 @@ typedef enum
 	CLOSE_NO_RENAME
 }	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.
+ */
 typedef struct WalWriteMethod WalWriteMethod;
 struct WalWriteMethod
 {
+	/*
+	 * Open a target file. Returns Walfile, or NULL if open failed. If a temp
+	 * suffix is specified, a file with that name will be opened, and then
+	 * 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);
+
+	/*
+	 * 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);
+
+	/* Check if a file exist */
 	bool		(*existsfile) (const char *pathname);
+
+	/* Return the size of a file, or -1 on failure. */
 	ssize_t		(*get_file_size) (const char *pathname);
 
+	/*
+	 * 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);
+
+	/*
+	 * fsync the contents of the specified file. Returns 0 on success.
+	 */
 	int			(*sync) (Walfile f);
+
+	/*
+	 * Clean up the Walmethod, closing any shared resources. For methods like
+	 * tar, this includes writing updated headers. Returns true if the
+	 * close/write/sync of shared resources succeeded, otherwise returns false
+	 * (but the resources are still closed).
+	 */
 	bool		(*finish) (void);
-	char	   *(*getlasterror) (void);
+
+	/* Return a text for the last error in this Walfile */
+	const char *(*getlasterror) (void);
 };
 
 /*