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....@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
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');
