Date: Thu, 21 Jan 2021 00:19:58 -0800
From: Noah Misch <noah@leadboat.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-hackers@postgresql.org, andres@anarazel.de
Subject: Re: Wrong usage of RelationNeedsWAL
Message-ID: <20210121081958.GA2008067@rfd.leadboat.com>
References: <20210119.134831.1956292555969369177.horikyota.ntt@gmail.com>
 <20210119093152.GA1831573@rfd.leadboat.com>
 <20210120.173444.1427112689198751884.horikyota.ntt@gmail.com>
 <20210121.002844.690876870629206205.horikyota.ntt@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="h31gzZEtNLTqOjlF"
Content-Disposition: inline
In-Reply-To: <20210121.002844.690876870629206205.horikyota.ntt@gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--h31gzZEtNLTqOjlF
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Jan 21, 2021 at 12:28:44AM +0900, Kyotaro Horiguchi wrote:
> At Wed, 20 Jan 2021 17:34:44 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > Anyway, it seems actually dangerous that cause pruning on wal-skipped
> > relation.
> > 
> > > with your patch versions.  Could you try implementing both test procedures in
> > > src/test/modules/snapshot_too_old?  There's no need to make the test use
> > > wal_level=minimal explicitly; it's sufficient to catch these bugs when
> > > wal_level=minimal is in the TEMP_CONFIG file.
> > 
> > In the attached, TestForOldSnapshot() considers XLogIsNeeded(),
> > instead of moving the condition on LSN to TestForOldSnapshot_impl for
> > performance.
> > 
> > I'll add the test part in the next version.

That test helped me.  I now see "there's not a single tuple removed due to
old_snapshot_threshold in src/test/modules/snapshot_too_old"[1], which limits
our ability to test using this infrastructure.

> However, with the previous patch, two existing tests sto_using_cursor
> and sto_using_select behaves differently from the master.  That change
> is coming from the omission of actual LSN check in TestForOldSnapshot
> while wal_level=minimal. So we have no choice other than actually
> updating page LSN.
> 
> In the scenario under discussion there are two places we need to do
> that. one is heap_page_prune, and the other is RelationCopyStorge. As
> a PoC, I used gistXLogAssignLSN() as is for thie purpose. See the
> attached third file.

Fake LSNs make the system harder to understand, so I prefer not to spread fake
LSNs to more access methods.  What I had in mind is to simply suppress early
pruning when the relation is skipping WAL.  Attached.  Is this reasonable?  It
passes the older tests.  While it changes the sto_wal_optimized.spec output, I
think it preserves the old_snapshot_threshold behavior contract.

[1] https://www.postgresql.org/message-id/20200403001235.e6jfdll3gh2ygbuc%40alap3.anarazel.de

--h31gzZEtNLTqOjlF
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
 filename="RelationNeedsWAL-to-relpersistence-v6nm.patch"

diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 5f8e1c6..84d2efc 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -67,7 +67,7 @@ check_publication_add_relation(Relation targetrel)
 				 errdetail("System tables cannot be added to publications.")));
 
 	/* UNLOGGED and TEMP relations cannot be part of publication. */
-	if (!RelationNeedsWAL(targetrel))
+	if (targetrel->rd_rel->relpersistence != RELPERSISTENCE_PERMANENT)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 				 errmsg("table \"%s\" cannot be replicated",
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index da322b4..177e6e3 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -126,7 +126,8 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
 	relation = table_open(relationObjectId, NoLock);
 
 	/* Temporary and unlogged relations are inaccessible during recovery. */
-	if (!RelationNeedsWAL(relation) && RecoveryInProgress())
+	if (relation->rd_rel->relpersistence != RELPERSISTENCE_PERMANENT &&
+		RecoveryInProgress())
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("cannot access temporary or unlogged relations during recovery")));
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index ae16c3e..9570426 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1764,7 +1764,11 @@ TransactionIdLimitedForOldSnapshots(TransactionId recentXmin,
 	Assert(OldSnapshotThresholdActive());
 	Assert(limit_ts != NULL && limit_xid != NULL);
 
-	if (!RelationAllowsEarlyPruning(relation))
+	/*
+	 * TestForOldSnapshot() assumes early pruning advances the page LSN, so we
+	 * can't prune early when skipping WAL.
+	 */
+	if (!RelationAllowsEarlyPruning(relation) || !RelationNeedsWAL(relation))
 		return false;
 
 	ts = GetSnapshotCurrentTimestamp();
diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
index 579be35..c21ee3c 100644
--- a/src/include/utils/snapmgr.h
+++ b/src/include/utils/snapmgr.h
@@ -37,7 +37,7 @@
  */
 #define RelationAllowsEarlyPruning(rel) \
 ( \
-	 RelationNeedsWAL(rel) \
+	 (rel)->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT	\
   && !IsCatalogRelation(rel) \
   && !RelationIsAccessibleInLogicalDecoding(rel) \
 )
diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
index 0680f44..ed9b48e 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -3,7 +3,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 23;
+use Test::More tests => 24;
 
 # Initialize publisher node
 my $node_publisher = get_new_node('publisher');
@@ -358,3 +358,21 @@ is($result, qq(0), 'check replication origin was dropped on subscriber');
 
 $node_subscriber->stop('fast');
 $node_publisher->stop('fast');
+
+#
+# CREATE PUBLICATION while wal_level=mimal should succeed, with a WARNING
+$node_publisher->append_conf(
+	'postgresql.conf', qq(
+wal_level=minimal
+max_wal_senders=0
+));
+$node_publisher->start;
+($result, my $retout, my $reterr) = $node_publisher->psql(
+	'postgres', qq{
+BEGIN;
+CREATE TABLE skip_wal();
+CREATE PUBLICATION tap_pub2 FOR TABLE skip_wal;
+ROLLBACK;
+});
+ok($reterr =~ m/WARNING:  wal_level is insufficient to publish logical changes/,
+   'test CREATE PUBLICATION can be done while wal_leve=minimal');

--h31gzZEtNLTqOjlF--