v1-0001-Always-use-an-end-of-recovery-record-rather-than-.patch

From 1d1e605ef0915553c6eec40ebf5e048bf2272836 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Mon, 9 Aug 2021 14:49:21 -0400
Subject: [PATCH v1] Always use an end-of-recovery record rather than a
 checkpoint.

Insert awful hack to allow LogStandbySnapshot() to not log a
standby snapshot, because it's unprepared to handle the case
where latestCompletedXid is 2, which can happen when starting
from the very first ever checkpoint.

Nuke 018_wal_optimize.pl. Otherwise, the "wal_level = minimal, SET
TABLESPACE commit subtransaction" test fails. The reason for the
failure is that CREATE TABLESPACE nukes the entire directory and
then recreates it, which is a really bad thing to do when
wal_level=minimal. Without the changes made by this patch the
problem is masked because each server restart necessarily
involves a checkpoint, so the CREATE TABLESPACE and the creation
of a table in that tablespace don't manage to happen in the same
checkpoint cycle.

This is not to be taken seriously in its current form; it's just
an illustration of (some of?) the problems that would need to be
solved to make this kind of change viable.
---
 src/backend/access/transam/xlog.c       |  63 ++--
 src/backend/postmaster/bgwriter.c       |   1 +
 src/backend/replication/slot.c          |   1 +
 src/backend/storage/ipc/standby.c       |   9 +
 src/test/recovery/t/018_wal_optimize.pl | 376 ------------------------
 5 files changed, 29 insertions(+), 421 deletions(-)
 delete mode 100644 src/test/recovery/t/018_wal_optimize.pl

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index d0ec6a834b..627d6e619d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6503,7 +6503,6 @@ StartupXLOG(void)
 	DBState		dbstate_at_startup;
 	XLogReaderState *xlogreader;
 	XLogPageReadPrivate private;
-	bool		promoted = false;
 	struct stat st;
 
 	/*
@@ -7876,43 +7875,8 @@ StartupXLOG(void)
 
 	if (InRecovery)
 	{
-		/*
-		 * Perform a checkpoint to update all our recovery activity to disk.
-		 *
-		 * Note that we write a shutdown checkpoint rather than an on-line
-		 * one. This is not particularly critical, but since we may be
-		 * assigning a new TLI, using a shutdown checkpoint allows us to have
-		 * the rule that TLI only changes in shutdown checkpoints, which
-		 * allows some extra error checking in xlog_redo.
-		 *
-		 * In promotion, only create a lightweight end-of-recovery record
-		 * instead of a full checkpoint. A checkpoint is requested later,
-		 * after we're fully out of recovery mode and already accepting
-		 * queries.
-		 */
-		if (ArchiveRecoveryRequested && IsUnderPostmaster &&
-			LocalPromoteIsTriggered)
-		{
-			promoted = true;
-
-			/*
-			 * Insert a special WAL record to mark the end of recovery, since
-			 * we aren't doing a checkpoint. That means that the checkpointer
-			 * process may likely be in the middle of a time-smoothed
-			 * restartpoint and could continue to be for minutes after this.
-			 * That sounds strange, but the effect is roughly the same and it
-			 * would be stranger to try to come out of the restartpoint and
-			 * then checkpoint. We request a checkpoint later anyway, just for
-			 * safety.
-			 */
-			CreateEndOfRecoveryRecord();
-		}
-		else
-		{
-			RequestCheckpoint(CHECKPOINT_END_OF_RECOVERY |
-							  CHECKPOINT_IMMEDIATE |
-							  CHECKPOINT_WAIT);
-		}
+		/* Insert a special WAL record to mark the end of recovery. */
+		CreateEndOfRecoveryRecord();
 	}
 
 	if (ArchiveRecoveryRequested)
@@ -8093,13 +8057,22 @@ StartupXLOG(void)
 	WalSndWakeup();
 
 	/*
-	 * If this was a promotion, request an (online) checkpoint now. This isn't
-	 * required for consistency, but the last restartpoint might be far back,
-	 * and in case of a crash, recovering from it might take a longer than is
-	 * appropriate now that we're not in standby mode anymore.
+	 * Request an (online) checkpoint now. Note that, until this is complete,
+	 * a crash would start replay from the same WAL location we did, or from
+	 * the last restartpoint that completed. We don't want to let that
+	 * situation persist for longer than necessary, since users don't like
+	 * long recovery times. On the other hand, they also want to be able to
+	 * start doing useful work again as quickly as possible. Therfore, we
+	 * don't pass CHECKPOINT_IMMEDIATE to avoid bogging down the system.
+	 *
+	 * Note that the consequence of requesting a checkpoint here only after
+	 * we've allowed WAL writes is that a single checkpoint cycle can span
+	 * multiple server lifetimes. So for example if you want to something to
+	 * happen at least once per checkpoint cycle or at most once per
+	 * checkpoint cycle, you have to consider what happens if the server
+	 * is restarted someplace in the middle.
 	 */
-	if (promoted)
-		RequestCheckpoint(CHECKPOINT_FORCE);
+	RequestCheckpoint(CHECKPOINT_FORCE);
 }
 
 /*
@@ -8914,7 +8887,7 @@ CreateCheckPoint(int flags)
 		shutdown = false;
 
 	/* sanity check */
-	if (RecoveryInProgress() && (flags & CHECKPOINT_END_OF_RECOVERY) == 0)
+	if (RecoveryInProgress())
 		elog(ERROR, "can't create a checkpoint during recovery");
 
 	/*
diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c
index 5584f4bc24..3eeacad50a 100644
--- a/src/backend/postmaster/bgwriter.c
+++ b/src/backend/postmaster/bgwriter.c
@@ -294,6 +294,7 @@ BackgroundWriterMain(void)
 				last_snapshot_lsn <= GetLastImportantRecPtr())
 			{
 				last_snapshot_lsn = LogStandbySnapshot();
+				Assert(!XLogRecPtrIsInvalid(last_snapshot_lsn));
 				last_snapshot_ts = now;
 			}
 		}
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 33e9acab4a..4b649d69e6 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1108,6 +1108,7 @@ ReplicationSlotReserveWal(void)
 
 			/* make sure we have enough information to start */
 			flushptr = LogStandbySnapshot();
+			Assert(!XLogRecPtrIsInvalid(flushptr));
 
 			/* and make sure it's fsynced to disk */
 			XLogFlush(flushptr);
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 077251c1a6..b718c30fa5 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -1226,6 +1226,15 @@ LogStandbySnapshot(void)
 
 	Assert(XLogStandbyInfoActive());
 
+	/*
+	 * HACK: If we do this in single user mode, we'll fail an assertion,
+	 * because until the first checkpoint completes, latestCompletedXid will
+	 * be FrozenTransactionId rather than a normal XID, and
+	 * GetRunningTransactionData() doesn't think that's OK.
+	 */
+	if (!IsUnderPostmaster)
+		return InvalidXLogRecPtr;
+
 	/*
 	 * Get details of any AccessExclusiveLocks being held at the moment.
 	 */
diff --git a/src/test/recovery/t/018_wal_optimize.pl b/src/test/recovery/t/018_wal_optimize.pl
deleted file mode 100644
index 4aa1bf8f54..0000000000
--- a/src/test/recovery/t/018_wal_optimize.pl
+++ /dev/null
@@ -1,376 +0,0 @@
-
-# Copyright (c) 2021, PostgreSQL Global Development Group
-
-# Test WAL replay when some operation has skipped WAL.
-#
-# These tests exercise code that once violated the mandate described in
-# src/backend/access/transam/README section "Skipping WAL for New
-# RelFileNode".  The tests work by committing some transactions, initiating an
-# immediate shutdown, and confirming that the expected data survives recovery.
-# For many years, individual commands made the decision to skip WAL, hence the
-# frequent appearance of COPY in these tests.
-use strict;
-use warnings;
-
-use PostgresNode;
-use TestLib;
-use Test::More tests => 34;
-
-sub check_orphan_relfilenodes
-{
-	my ($node, $test_name) = @_;
-
-	my $db_oid = $node->safe_psql('postgres',
-		"SELECT oid FROM pg_database WHERE datname = 'postgres'");
-	my $prefix               = "base/$db_oid/";
-	my $filepaths_referenced = $node->safe_psql(
-		'postgres', "
-	   SELECT pg_relation_filepath(oid) FROM pg_class
-	   WHERE reltablespace = 0 AND relpersistence <> 't' AND
-	   pg_relation_filepath(oid) IS NOT NULL;");
-	is_deeply(
-		[
-			sort(map { "$prefix$_" }
-				  grep(/^[0-9]+$/, slurp_dir($node->data_dir . "/$prefix")))
-		],
-		[ sort split /\n/, $filepaths_referenced ],
-		$test_name);
-	return;
-}
-
-# We run this same test suite for both wal_level=minimal and replica.
-sub run_wal_optimize
-{
-	my $wal_level = shift;
-
-	my $node = PostgresNode->new("node_$wal_level");
-	$node->init;
-	$node->append_conf(
-		'postgresql.conf', qq(
-wal_level = $wal_level
-max_prepared_transactions = 1
-wal_log_hints = on
-wal_skip_threshold = 0
-#wal_debug = on
-));
-	$node->start;
-
-	# Setup
-	my $tablespace_dir = $node->basedir . '/tablespace_other';
-	mkdir($tablespace_dir);
-	$tablespace_dir = TestLib::perl2host($tablespace_dir);
-	$node->safe_psql('postgres',
-		"CREATE TABLESPACE other LOCATION '$tablespace_dir';");
-
-	# Test direct truncation optimization.  No tuples.
-	$node->safe_psql(
-		'postgres', "
-		BEGIN;
-		CREATE TABLE trunc (id serial PRIMARY KEY);
-		TRUNCATE trunc;
-		COMMIT;");
-	$node->stop('immediate');
-	$node->start;
-	my $result = $node->safe_psql('postgres', "SELECT count(*) FROM trunc;");
-	is($result, qq(0), "wal_level = $wal_level, TRUNCATE with empty table");
-
-	# Test truncation with inserted tuples within the same transaction.
-	# Tuples inserted after the truncation should be seen.
-	$node->safe_psql(
-		'postgres', "
-		BEGIN;
-		CREATE TABLE trunc_ins (id serial PRIMARY KEY);
-		INSERT INTO trunc_ins VALUES (DEFAULT);
-		TRUNCATE trunc_ins;
-		INSERT INTO trunc_ins VALUES (DEFAULT);
-		COMMIT;");
-	$node->stop('immediate');
-	$node->start;
-	$result = $node->safe_psql('postgres',
-		"SELECT count(*), min(id) FROM trunc_ins;");
-	is($result, qq(1|2), "wal_level = $wal_level, TRUNCATE INSERT");
-
-	# Same for prepared transaction.
-	# Tuples inserted after the truncation should be seen.
-	$node->safe_psql(
-		'postgres', "
-		BEGIN;
-		CREATE TABLE twophase (id serial PRIMARY KEY);
-		INSERT INTO twophase VALUES (DEFAULT);
-		TRUNCATE twophase;
-		INSERT INTO twophase VALUES (DEFAULT);
-		PREPARE TRANSACTION 't';
-		COMMIT PREPARED 't';");
-	$node->stop('immediate');
-	$node->start;
-	$result = $node->safe_psql('postgres',
-		"SELECT count(*), min(id) FROM trunc_ins;");
-	is($result, qq(1|2), "wal_level = $wal_level, TRUNCATE INSERT PREPARE");
-
-	# Writing WAL at end of xact, instead of syncing.
-	$node->safe_psql(
-		'postgres', "
-		SET wal_skip_threshold = '1GB';
-		BEGIN;
-		CREATE TABLE noskip (id serial PRIMARY KEY);
-		INSERT INTO noskip (SELECT FROM generate_series(1, 20000) a) ;
-		COMMIT;");
-	$node->stop('immediate');
-	$node->start;
-	$result = $node->safe_psql('postgres', "SELECT count(*) FROM noskip;");
-	is($result, qq(20000), "wal_level = $wal_level, end-of-xact WAL");
-
-	# Data file for COPY query in subsequent tests
-	my $basedir   = $node->basedir;
-	my $copy_file = "$basedir/copy_data.txt";
-	TestLib::append_to_file(
-		$copy_file, qq(20000,30000
-20001,30001
-20002,30002));
-	$copy_file = TestLib::perl2host($copy_file);
-
-	# Test truncation with inserted tuples using both INSERT and COPY.  Tuples
-	# inserted after the truncation should be seen.
-	$node->safe_psql(
-		'postgres', "
-		BEGIN;
-		CREATE TABLE ins_trunc (id serial PRIMARY KEY, id2 int);
-		INSERT INTO ins_trunc VALUES (DEFAULT, generate_series(1,10000));
-		TRUNCATE ins_trunc;
-		INSERT INTO ins_trunc (id, id2) VALUES (DEFAULT, 10000);
-		COPY ins_trunc FROM '$copy_file' DELIMITER ',';
-		INSERT INTO ins_trunc (id, id2) VALUES (DEFAULT, 10000);
-		COMMIT;");
-	$node->stop('immediate');
-	$node->start;
-	$result = $node->safe_psql('postgres', "SELECT count(*) FROM ins_trunc;");
-	is($result, qq(5), "wal_level = $wal_level, TRUNCATE COPY INSERT");
-
-	# Test truncation with inserted tuples using COPY.  Tuples copied after
-	# the truncation should be seen.
-	$node->safe_psql(
-		'postgres', "
-		BEGIN;
-		CREATE TABLE trunc_copy (id serial PRIMARY KEY, id2 int);
-		INSERT INTO trunc_copy VALUES (DEFAULT, generate_series(1,3000));
-		TRUNCATE trunc_copy;
-		COPY trunc_copy FROM '$copy_file' DELIMITER ',';
-		COMMIT;");
-	$node->stop('immediate');
-	$node->start;
-	$result =
-	  $node->safe_psql('postgres', "SELECT count(*) FROM trunc_copy;");
-	is($result, qq(3), "wal_level = $wal_level, TRUNCATE COPY");
-
-	# Like previous test, but rollback SET TABLESPACE in a subtransaction.
-	$node->safe_psql(
-		'postgres', "
-		BEGIN;
-		CREATE TABLE spc_abort (id serial PRIMARY KEY, id2 int);
-		INSERT INTO spc_abort VALUES (DEFAULT, generate_series(1,3000));
-		TRUNCATE spc_abort;
-		SAVEPOINT s;
-		  ALTER TABLE spc_abort SET TABLESPACE other; ROLLBACK TO s;
-		COPY spc_abort FROM '$copy_file' DELIMITER ',';
-		COMMIT;");
-	$node->stop('immediate');
-	$node->start;
-	$result = $node->safe_psql('postgres', "SELECT count(*) FROM spc_abort;");
-	is($result, qq(3),
-		"wal_level = $wal_level, SET TABLESPACE abort subtransaction");
-
-	# in different subtransaction patterns
-	$node->safe_psql(
-		'postgres', "
-		BEGIN;
-		CREATE TABLE spc_commit (id serial PRIMARY KEY, id2 int);
-		INSERT INTO spc_commit VALUES (DEFAULT, generate_series(1,3000));
-		TRUNCATE spc_commit;
-		SAVEPOINT s; ALTER TABLE spc_commit SET TABLESPACE other; RELEASE s;
-		COPY spc_commit FROM '$copy_file' DELIMITER ',';
-		COMMIT;");
-	$node->stop('immediate');
-	$node->start;
-	$result =
-	  $node->safe_psql('postgres', "SELECT count(*) FROM spc_commit;");
-	is($result, qq(3),
-		"wal_level = $wal_level, SET TABLESPACE commit subtransaction");
-
-	$node->safe_psql(
-		'postgres', "
-		BEGIN;
-		CREATE TABLE spc_nest (id serial PRIMARY KEY, id2 int);
-		INSERT INTO spc_nest VALUES (DEFAULT, generate_series(1,3000));
-		TRUNCATE spc_nest;
-		SAVEPOINT s;
-			ALTER TABLE spc_nest SET TABLESPACE other;
-			SAVEPOINT s2;
-				ALTER TABLE spc_nest SET TABLESPACE pg_default;
-			ROLLBACK TO s2;
-			SAVEPOINT s2;
-				ALTER TABLE spc_nest SET TABLESPACE pg_default;
-			RELEASE s2;
-		ROLLBACK TO s;
-		COPY spc_nest FROM '$copy_file' DELIMITER ',';
-		COMMIT;");
-	$node->stop('immediate');
-	$node->start;
-	$result = $node->safe_psql('postgres', "SELECT count(*) FROM spc_nest;");
-	is($result, qq(3),
-		"wal_level = $wal_level, SET TABLESPACE nested subtransaction");
-
-	$node->safe_psql(
-		'postgres', "
-		CREATE TABLE spc_hint (id int);
-		INSERT INTO spc_hint VALUES (1);
-		BEGIN;
-		ALTER TABLE spc_hint SET TABLESPACE other;
-		CHECKPOINT;
-		SELECT * FROM spc_hint;  -- set hint bit
-		INSERT INTO spc_hint VALUES (2);
-		COMMIT;");
-	$node->stop('immediate');
-	$node->start;
-	$result = $node->safe_psql('postgres', "SELECT count(*) FROM spc_hint;");
-	is($result, qq(2), "wal_level = $wal_level, SET TABLESPACE, hint bit");
-
-	$node->safe_psql(
-		'postgres', "
-		BEGIN;
-		CREATE TABLE idx_hint (c int PRIMARY KEY);
-		SAVEPOINT q; INSERT INTO idx_hint VALUES (1); ROLLBACK TO q;
-		CHECKPOINT;
-		INSERT INTO idx_hint VALUES (1);  -- set index hint bit
-		INSERT INTO idx_hint VALUES (2);
-		COMMIT;");
-	$node->stop('immediate');
-	$node->start;
-	$result = $node->psql('postgres',);
-	my ($ret, $stdout, $stderr) =
-	  $node->psql('postgres', "INSERT INTO idx_hint VALUES (2);");
-	is($ret, qq(3), "wal_level = $wal_level, unique index LP_DEAD");
-	like(
-		$stderr,
-		qr/violates unique/,
-		"wal_level = $wal_level, unique index LP_DEAD message");
-
-	# UPDATE touches two buffers for one row.
-	$node->safe_psql(
-		'postgres', "
-		BEGIN;
-		CREATE TABLE upd (id serial PRIMARY KEY, id2 int);
-		INSERT INTO upd (id, id2) VALUES (DEFAULT, generate_series(1,10000));
-		COPY upd FROM '$copy_file' DELIMITER ',';
-		UPDATE upd SET id2 = id2 + 1;
-		DELETE FROM upd;
-		COMMIT;");
-	$node->stop('immediate');
-	$node->start;
-	$result = $node->safe_psql('postgres', "SELECT count(*) FROM upd;");
-	is($result, qq(0),
-		"wal_level = $wal_level, UPDATE touches two buffers for one row");
-
-	# Test consistency of COPY with INSERT for table created in the same
-	# transaction.
-	$node->safe_psql(
-		'postgres', "
-		BEGIN;
-		CREATE TABLE ins_copy (id serial PRIMARY KEY, id2 int);
-		INSERT INTO ins_copy VALUES (DEFAULT, 1);
-		COPY ins_copy FROM '$copy_file' DELIMITER ',';
-		COMMIT;");
-	$node->stop('immediate');
-	$node->start;
-	$result = $node->safe_psql('postgres', "SELECT count(*) FROM ins_copy;");
-	is($result, qq(4), "wal_level = $wal_level, INSERT COPY");
-
-	# Test consistency of COPY that inserts more to the same table using
-	# triggers.  If the INSERTS from the trigger go to the same block data
-	# is copied to, and the INSERTs are WAL-logged, WAL replay will fail when
-	# it tries to replay the WAL record but the "before" image doesn't match,
-	# because not all changes were WAL-logged.
-	$node->safe_psql(
-		'postgres', "
-		BEGIN;
-		CREATE TABLE ins_trig (id serial PRIMARY KEY, id2 text);
-		CREATE FUNCTION ins_trig_before_row_trig() RETURNS trigger
-		  LANGUAGE plpgsql as \$\$
-		  BEGIN
-			IF new.id2 NOT LIKE 'triggered%' THEN
-			  INSERT INTO ins_trig
-				VALUES (DEFAULT, 'triggered row before' || NEW.id2);
-			END IF;
-			RETURN NEW;
-		  END; \$\$;
-		CREATE FUNCTION ins_trig_after_row_trig() RETURNS trigger
-		  LANGUAGE plpgsql as \$\$
-		  BEGIN
-			IF new.id2 NOT LIKE 'triggered%' THEN
-			  INSERT INTO ins_trig
-				VALUES (DEFAULT, 'triggered row after' || NEW.id2);
-			END IF;
-			RETURN NEW;
-		  END; \$\$;
-		CREATE TRIGGER ins_trig_before_row_insert
-		  BEFORE INSERT ON ins_trig
-		  FOR EACH ROW EXECUTE PROCEDURE ins_trig_before_row_trig();
-		CREATE TRIGGER ins_trig_after_row_insert
-		  AFTER INSERT ON ins_trig
-		  FOR EACH ROW EXECUTE PROCEDURE ins_trig_after_row_trig();
-		COPY ins_trig FROM '$copy_file' DELIMITER ',';
-		COMMIT;");
-	$node->stop('immediate');
-	$node->start;
-	$result = $node->safe_psql('postgres', "SELECT count(*) FROM ins_trig;");
-	is($result, qq(9), "wal_level = $wal_level, COPY with INSERT triggers");
-
-	# Test consistency of INSERT, COPY and TRUNCATE in same transaction block
-	# with TRUNCATE triggers.
-	$node->safe_psql(
-		'postgres', "
-		BEGIN;
-		CREATE TABLE trunc_trig (id serial PRIMARY KEY, id2 text);
-		CREATE FUNCTION trunc_trig_before_stat_trig() RETURNS trigger
-		  LANGUAGE plpgsql as \$\$
-		  BEGIN
-			INSERT INTO trunc_trig VALUES (DEFAULT, 'triggered stat before');
-			RETURN NULL;
-		  END; \$\$;
-		CREATE FUNCTION trunc_trig_after_stat_trig() RETURNS trigger
-		  LANGUAGE plpgsql as \$\$
-		  BEGIN
-			INSERT INTO trunc_trig VALUES (DEFAULT, 'triggered stat before');
-			RETURN NULL;
-		  END; \$\$;
-		CREATE TRIGGER trunc_trig_before_stat_truncate
-		  BEFORE TRUNCATE ON trunc_trig
-		  FOR EACH STATEMENT EXECUTE PROCEDURE trunc_trig_before_stat_trig();
-		CREATE TRIGGER trunc_trig_after_stat_truncate
-		  AFTER TRUNCATE ON trunc_trig
-		  FOR EACH STATEMENT EXECUTE PROCEDURE trunc_trig_after_stat_trig();
-		INSERT INTO trunc_trig VALUES (DEFAULT, 1);
-		TRUNCATE trunc_trig;
-		COPY trunc_trig FROM '$copy_file' DELIMITER ',';
-		COMMIT;");
-	$node->stop('immediate');
-	$node->start;
-	$result =
-	  $node->safe_psql('postgres', "SELECT count(*) FROM trunc_trig;");
-	is($result, qq(4),
-		"wal_level = $wal_level, TRUNCATE COPY with TRUNCATE triggers");
-
-	# Test redo of temp table creation.
-	$node->safe_psql(
-		'postgres', "
-		CREATE TEMP TABLE temp (id serial PRIMARY KEY, id2 text);");
-	$node->stop('immediate');
-	$node->start;
-	check_orphan_relfilenodes($node,
-		"wal_level = $wal_level, no orphan relfilenode remains");
-
-	return;
-}
-
-# Run same test suite for multiple wal_level values.
-run_wal_optimize("minimal");
-run_wal_optimize("replica");
-- 
2.24.3 (Apple Git-128)