From 3c2c369c495760d8f96d3cdeab623c4f0372792a Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Thu, 15 Sep 2022 13:39:53 -0500
Subject: [PATCH 2/2] f!justin

ci-os-only: freebsd
---
 src/bin/pg_waldump/meson.build            |   1 +
 src/bin/pg_waldump/pg_waldump.c           | 172 +++++++++++-----------
 src/bin/pg_waldump/t/002_save_fullpage.pl |   2 +-
 3 files changed, 86 insertions(+), 89 deletions(-)

diff --git a/src/bin/pg_waldump/meson.build b/src/bin/pg_waldump/meson.build
index 9605976870d..34e37bffc36 100644
--- a/src/bin/pg_waldump/meson.build
+++ b/src/bin/pg_waldump/meson.build
@@ -29,6 +29,7 @@ tests += {
   'tap': {
     'tests': [
       't/001_basic.pl',
+      't/002_save_fullpage.pl',
     ],
   },
 }
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 2b6f772dc71..e35d15132f4 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -76,7 +76,6 @@ typedef struct XLogDumpConfig
 	bool		filter_by_fpw;
 
 	/* save options */
-	bool		save_fpw;
 	bool		fixup_fpw;
 	char	   *save_fpw_path;
 } XLogDumpConfig;
@@ -458,96 +457,94 @@ XLogRecordSaveFPWs(XLogReaderState *record, const char *savepath, bool fixup)
 
 	for (block_id = 0; block_id <= XLogRecMaxBlockId(record); block_id++)
 	{
+		char		page[BLCKSZ] = {0};
+		char		filename[MAXPGPATH];
+		char		forkname[FORKNAMECHARS + 2];	/* _ + \0 */
+		FILE	   *OPF;
+		BlockNumber blk;
+		RelFileLocator rnode;
+		ForkNumber	fork;
+
 		if (!XLogRecHasBlockRef(record, block_id))
 			continue;
 
-		if (XLogRecHasBlockImage(record, block_id))
-		{
-			char		page[BLCKSZ];
-
-			memset(page, 0, BLCKSZ);
-
-			if (RestoreBlockImage(record, block_id, page))
-			{
-				/* we have our extracted FPI, let's save it now */
-				char		filename[MAXPGPATH];
-				char		forkname[FORKNAMECHARS + 2];	/* _ + \0 */
-				FILE	   *OPF;
-				BlockNumber blk;
-				RelFileLocator rnode;
-				ForkNumber	fork;
-
-				XLogRecGetBlockTagExtended(record, block_id,
-										   &rnode, &fork, &blk, NULL);
-
-				/*
-				 * The raw page as stored in the WAL record includes the LSN
-				 * of the block as it appeared when it was originally written,
-				 * however this differs than the effects of replaying this
-				 * same FPI in recovery, as recovery calls RestoreBlockImage()
-				 * and then sets the LSN as part of one action.  What this
-				 * means is that a page as recovered from WAL and the version
-				 * of the page saved here will differ by the LSN and the
-				 * checksum (if enabled).
-				 *
-				 * There are potentially use-cases for both versions (with and
-				 * without mentioned fixups), so allow this to be
-				 * user-selected, unless the restored page was empty, in which
-				 * case we leave it alone.
-				 */
-
-				if (fixup && !PageIsNew(page))
-				{
-					PageSetLSN(page, record->ReadRecPtr);
+		if (!XLogRecHasBlockImage(record, block_id))
+			continue;
 
-					/*
-					 * If checksum field is non-zero then we have checksums
-					 * enabled, so recalculate the checksum with new LSN
-					 * (whether this is considered a hack or heuristics is an
-					 * exercise for the reader).
-					 *
-					 * We make this choice to allow pages saved by this
-					 * function to work as expected with the checksum-related
-					 * functions in pageinspect without having to worry about
-					 * zero_damaged_pages or other considerations.
-					 *
-					 * FPIs in WAL do not have the checksum field updated in
-					 * the page image; in a checksums-enabled cluster, this
-					 * task is handled by FlushBuffer() when a dirty buffer is
-					 * written out to disk.  Since we are running outside of
-					 * Postmaster that won't work in this case, so we handle
-					 * ourselves.
-					 */
+		if (!RestoreBlockImage(record, block_id, page))
+			continue;
 
-					if (((PageHeader) page)->pd_checksum)
-						((PageHeader) page)->pd_checksum = pg_checksum_page((char *) page, blk);
-				}
+		/* we have our extracted FPI, let's save it now */
+
+		XLogRecGetBlockTagExtended(record, block_id,
+								   &rnode, &fork, &blk, NULL);
+
+		/*
+		 * The raw page as stored in the WAL record includes the LSN
+		 * of the block as it appeared when it was originally written,
+		 * however this differs than the effects of replaying this
+		 * same FPI in recovery, as recovery calls RestoreBlockImage()
+		 * and then sets the LSN as part of one action.  What this
+		 * means is that a page as recovered from WAL and the version
+		 * of the page saved here will differ by the LSN and the
+		 * checksum (if enabled).
+		 *
+		 * There are potentially use-cases for both versions (with and
+		 * without mentioned fixups), so allow this to be
+		 * user-selected, unless the restored page was empty, in which
+		 * case we leave it alone.
+		 */
+
+		if (fixup && !PageIsNew(page))
+		{
+			PageSetLSN(page, record->ReadRecPtr);
+
+			/*
+			 * If checksum field is non-zero then we have checksums
+			 * enabled, so recalculate the checksum with new LSN
+			 * (whether this is considered a hack or heuristics is an
+			 * exercise for the reader).
+			 *
+			 * We make this choice to allow pages saved by this
+			 * function to work as expected with the checksum-related
+			 * functions in pageinspect without having to worry about
+			 * zero_damaged_pages or other considerations.
+			 *
+			 * FPIs in WAL do not have the checksum field updated in
+			 * the page image; in a checksums-enabled cluster, this
+			 * task is handled by FlushBuffer() when a dirty buffer is
+			 * written out to disk.  Since we are running outside of
+			 * Postmaster that won't work in this case, so we handle
+			 * ourselves.
+			 */
+
+			if (((PageHeader) page)->pd_checksum)
+				((PageHeader) page)->pd_checksum = pg_checksum_page((char *) page, blk);
+		}
 
-				if (fork >= 0 && fork <= MAX_FORKNUM)
-				{
-					if (fork)
-						sprintf(forkname, "_%s", forkNames[fork]);
-					else
-						forkname[0] = 0;
-				}
-				else
-					pg_fatal("found invalid fork number: %u", fork);
+		if (fork >= 0 && fork <= MAX_FORKNUM)
+		{
+			if (fork)
+				sprintf(forkname, "_%s", forkNames[fork]);
+			else
+				forkname[0] = 0;
+		}
+		else
+			pg_fatal("found invalid fork number: %u", fork);
 
-				snprintf(filename, MAXPGPATH, "%s/%08X-%08X.%u.%u.%u.%u%s", savepath,
-						 LSN_FORMAT_ARGS(record->ReadRecPtr),
-						 rnode.spcOid, rnode.dbOid, rnode.relNumber, blk, forkname);
+		snprintf(filename, MAXPGPATH, "%s/%08X-%08X.%u.%u.%u.%u%s", savepath,
+				 LSN_FORMAT_ARGS(record->ReadRecPtr),
+				 rnode.spcOid, rnode.dbOid, rnode.relNumber, blk, forkname);
 
-				OPF = fopen(filename, PG_BINARY_W);
-				if (!OPF)
-					pg_fatal("couldn't open file for output: %s", filename);
+		OPF = fopen(filename, PG_BINARY_W);
+		if (!OPF)
+			pg_fatal("couldn't open file for output: %s", filename);
 
-				if (pg_pwrite(fileno(OPF), page, BLCKSZ, 0) != BLCKSZ)
-					pg_fatal("couldn't write out complete fullpage image to file: %s", filename);
+		if (pg_pwrite(fileno(OPF), page, BLCKSZ, 0) != BLCKSZ)
+			pg_fatal("couldn't write out complete fullpage image to file: %s", filename);
 
-				fsync(fileno(OPF));
-				fclose(OPF);
-			}
-		}
+		fsync(fileno(OPF));
+		fclose(OPF);
 	}
 }
 
@@ -791,9 +788,9 @@ usage(void)
 			 "                         (default: 1 or the value used in STARTSEG)\n"));
 	printf(_("  -V, --version          output version information, then exit\n"));
 	printf(_("  -w, --fullpage         only show records with a full page write\n"));
-	printf(_("  -W, --save-fpi=path    save found full page images to given path\n"
-			 "                         as LSN.T.D.R.B_F\n"));
-	printf(_("  -X, --fixup-fpi=path   like --save-fpi but apply LSN fixups to \n"));
+	printf(_("  -W, --save-fpi=path    save full page images to given path as\n"
+			 "                         LSN.T.D.R.B_F\n"));
+	printf(_("  -X, --fixup-fpi=path   like --save-fpi but apply LSN fixups to saved page\n"));
 	printf(_("  -x, --xid=XID          only show records with transaction ID XID\n"));
 	printf(_("  -z, --stats[=record]   show statistics instead of records\n"
 			 "                         (optionally, show per-record statistics)\n"));
@@ -889,7 +886,6 @@ main(int argc, char **argv)
 	config.filter_by_fpw = false;
 	config.stats = false;
 	config.stats_per_record = false;
-	config.save_fpw = false;
 	config.fixup_fpw = false;
 	config.save_fpw_path = NULL;
 
@@ -1040,7 +1036,6 @@ main(int argc, char **argv)
 				break;
 			case 'W':
 			case 'X':
-				config.save_fpw = true;
 				config.fixup_fpw = (option == 'X');
 				config.save_fpw_path = pg_strdup(optarg);
 				break;
@@ -1108,7 +1103,8 @@ main(int argc, char **argv)
 			goto bad_argument;
 		}
 
-		if (!dir_status && mkdir(config.save_fpw_path, 0700) < 0)
+		/* Create the dir if it doesn't exist */
+		if (dir_status == 0 && mkdir(config.save_fpw_path, 0700) < 0)
 		{
 			pg_log_error("could not create output directory \"%s\": %m",
 						 config.save_fpw_path);
@@ -1294,7 +1290,7 @@ main(int argc, char **argv)
 				XLogRecStoreStats(&stats, xlogreader_state);
 				stats.endptr = xlogreader_state->EndRecPtr;
 			}
-			else if (config.save_fpw)
+			else if (config.save_fpw_path)
 			{
 				if (XLogRecordHasFPW(xlogreader_state))
 					XLogRecordSaveFPWs(xlogreader_state, config.save_fpw_path, config.fixup_fpw);
diff --git a/src/bin/pg_waldump/t/002_save_fullpage.pl b/src/bin/pg_waldump/t/002_save_fullpage.pl
index f605b4f6adb..6df670c39ba 100644
--- a/src/bin/pg_waldump/t/002_save_fullpage.pl
+++ b/src/bin/pg_waldump/t/002_save_fullpage.pl
@@ -60,7 +60,7 @@ EOF
 
 # get the relation node, etc for the new table
 my $relation = $primary->safe_psql('db1',
-	q{SELECT CASE WHEN reltablespace = 0 THEN dattablespace ELSE reltablespace END || '/' || pg_database.oid || '/' || relfilenode FROM pg_class, pg_database WHERE relname = 'test_table' AND datname = current_database()}
+	q{SELECT format('%s/%s/%s', CASE WHEN reltablespace = 0 THEN dattablespace ELSE reltablespace END, pg_database.oid, relfilenode) FROM pg_class, pg_database WHERE relname = 'test_table' AND datname = current_database()}
 );
 
 diag $relation;
-- 
2.25.1

