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: Wed, 27 Jan 2021 02:48:48 -0800
Message-ID: <20210127104848.GA2508213@rfd.leadboat.com> (raw)
In-Reply-To: <20210121092336.GB2008067@rfd.leadboat.com>
References: <20210120.173444.1427112689198751884.horikyota.ntt@gmail.com>
<20210121.002844.690876870629206205.horikyota.ntt@gmail.com>
<20210121081958.GA2008067@rfd.leadboat.com>
<20210121.180211.1817909680822521903.horikyota.ntt@gmail.com>
<20210121092336.GB2008067@rfd.leadboat.com>
On Thu, Jan 21, 2021 at 01:23:36AM -0800, Noah Misch wrote:
> On Thu, Jan 21, 2021 at 06:02:11PM +0900, Kyotaro Horiguchi wrote:
> > At Thu, 21 Jan 2021 00:19:58 -0800, Noah Misch <noah@leadboat.com> wrote in
> > > On Thu, Jan 21, 2021 at 12:28:44AM +0900, Kyotaro Horiguchi wrote:
> > > > 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.
> >
> > Perhaps I'm missing something, but the patch doesn't pass the v5-0001
> > test with wal_level=minimal?
>
> Correct. The case we must avoid is letting an old snapshot read an
> early-pruned page without error. v5-0001 expects "ERROR: snapshot too old".
> The patch suspends early pruning, so that error is not applicable.
I think the attached version is ready. The changes since v6nm are cosmetic:
- Wrote log messages
- Split into two patches, since the user-visible bugs are materially different
- Fixed typos
- Ran perltidy
Is it okay if I push these on Saturday, or would you like more time to
investigate?
Attachments:download all (2 files, .tar.gz)RelationNeedsWAL-to-relpersistence-pub-v8nm.patch (text/plain, 3.1K) download
Author: Noah Misch <noah@leadboat.com>
Commit: Noah Misch <noah@leadboat.com>
Fix error with CREATE PUBLICATION, wal_level=minimal, and new tables.
CREATE PUBLICATION has failed spuriously when applied to a permanent
relation created or rewritten in the current transaction. Make the same
change to another site having the same semantic intent; the second
instance has no user-visible consequences. Back-patch to v13, where
commit c6b92041d38512a4176ed76ad06f713d2e6c01a8 broke this.
Kyotaro Horiguchi
Discussion: https://postgr.es/m/20210113.160705.2225256954956139776.horikyota.ntt@gmail.com
diff --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/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.plindex 0680f44..3d90f81 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=minimal 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/,+ 'CREATE PUBLICATION while wal_level=minimal');RelationNeedsWAL-to-relpersistence-sto-v8nm.patch (text/plain, 2.1K) download
Author: Noah Misch <noah@leadboat.com>
Commit: Noah Misch <noah@leadboat.com>
Revive "snapshot too old" with wal_level=minimal and SET TABLESPACE.
Given a permanent relation rewritten in the current transaction, the
old_snapshot_threshold mechanism assumed the relation had never been
subject to early pruning. Hence, a query could fail to report "snapshot
too old" when the rewrite followed an early truncation. ALTER TABLE SET
TABLESPACE is probably the only rewrite mechanism capable of exposing
this bug. REINDEX sets indcheckxmin, avoiding the problem. CLUSTER has
zeroed page LSNs since before old_snapshot_threshold existed, so
old_snapshot_threshold has never cooperated with it. ALTER TABLE
... SET DATA TYPE makes the table look empty to every past snapshot,
which is strictly worse. Back-patch to v13, where commit
c6b92041d38512a4176ed76ad06f713d2e6c01a8 broke this.
Kyotaro Horiguchi and Noah Misch
Discussion: https://postgr.es/m/20210113.160705.2225256954956139776.horikyota.ntt@gmail.com
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) \
)
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: <20210127104848.GA2508213@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