From: Noah Misch <noah@leadboat.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-hackers@postgresql.org
Cc: andres@anarazel.de
Subject: Re: Wrong usage of RelationNeedsWAL
Date: Thu, 21 Jan 2021 00:19:58 -0800
Message-ID: <20210121081958.GA2008067@rfd.leadboat.com> (raw)
In-Reply-To: <20210121.002844.690876870629206205.horikyota.ntt@gmail.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>
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.deAttachments:RelationNeedsWAL-to-relpersistence-v6nm.patch (text/plain, 3.5K) downloaddiff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.cindex 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.cindex 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.cindex 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.hindex 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.plindex 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');
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com, horikyota.ntt@gmail.com, andres@anarazel.de
Subject: Re: Wrong usage of RelationNeedsWAL
In-Reply-To: <20210121081958.GA2008067@rfd.leadboat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox