Re: Wrong usage of RelationNeedsWAL - Mailing list pgsql-hackers

Thank you for the comments, Noah and Andres.

At Fri, 15 Jan 2021 20:38:16 -0800, Noah Misch <noah@leadboat.com> wrote in 
> On Wed, Jan 13, 2021 at 04:07:05PM +0900, Kyotaro Horiguchi wrote:
> > The definition of the macro RelationNeedsWAL has been changed by
> > c6b92041d3 to include conditions related to the WAL-skip optimization
> > but some uses of the macro are not relevant to the optimization. That
> > misuses are harmless for now as they are only executed while wal_level
> > >= replica or WAL-skipping is inactive. However, this should be
> > corrected to prevent future hazard.
> 
> I see user-visible consequences:
> 
> > --- 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 (!RelationIsWalLogged(targetrel))
> 
> Today, the following fails needlessly under wal_level=minimal:
> 
> BEGIN;
> SET client_min_messages = 'ERROR';
> CREATE TABLE skip_wal ();
> CREATE PUBLICATION p FOR TABLE skip_wal;
> ROLLBACK;
> 
> Could you add that test to one of the regress scripts?

Mmm. I thought that it cannot be used while wal_level=minimal. The
WARNING for insiffucient wal_level shows that it is intended to
work. A test is added in the attached.

> >          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 da322b453e..0500efcdb9 100644
> > --- a/src/backend/optimizer/util/plancat.c
> > +++ b/src/backend/optimizer/util/plancat.c
> > @@ -126,7 +126,7 @@ 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 (!RelationIsWalLogged(relation) && RecoveryInProgress())
> 
> This has no user-visible consequences, but I agree you've clarified it.
> 
> >          ereport(ERROR,
> >                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> >                   errmsg("cannot access temporary or unlogged relations during recovery")));
> > diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
> > index 10b63982c0..810806a542 100644
> > --- a/src/include/utils/rel.h
> > +++ b/src/include/utils/rel.h
> > @@ -552,16 +552,23 @@ typedef struct ViewOptions
> >          (relation)->rd_smgr->smgr_targblock = (targblock); \
> >      } while (0)
> >  
> > +/*
> > + * RelationIsPermanent
> > + *        True if relation is WAL-logged.
> > + */
> > +#define RelationIsWalLogged(relation)                                    \
> > +    ((relation)->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT)
> 
> To me, the names RelationIsWalLogged() and RelationNeedsWAL() don't convey
> their behavior difference.  How about one of the following spellings?

Yeah! I was concerned about that.

> - Name the new macro RelationEverNeedsWAL().
> - Just test "relpersistence == RELPERSISTENCE_PERMANENT", without a macro.

Yes. I wasn't confident on using the macro since as you pointed the
macro name is very confusing.  The reason for using a macro was
RelationUsesLocalBuffers().

I'm not sure the exact meaning the "ever" (doesn't seem to be
"always"). On the other hand there are many places where the second
line above takes place. So I chose to do that without a macro.

> > +
> >  /*
> >   * RelationNeedsWAL
> > - *        True if relation needs WAL.
> > + *        True if relation needs WAL at the time.
> >   *
> >   * Returns false if wal_level = minimal and this relation is created or
> >   * truncated in the current transaction.  See "Skipping WAL for New
> >   * RelFileNode" in src/backend/access/transam/README.
> >   */
> >  #define RelationNeedsWAL(relation)                                        \
> > -    ((relation)->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT &&    \
> > +    (RelationIsWalLogged(relation) &&                                    \
> >       (XLogIsNeeded() ||                                                    \
> >        (relation->rd_createSubid == InvalidSubTransactionId &&            \
> >         relation->rd_firstRelfilenodeSubid == InvalidSubTransactionId)))
> > @@ -619,7 +626,7 @@ typedef struct ViewOptions
> >   */
> >  #define RelationIsAccessibleInLogicalDecoding(relation) \
> >      (XLogLogicalInfoActive() && \
> > -     RelationNeedsWAL(relation) && \
> > +     RelationIsWalLogged(relation) && \
> 
> Today's condition expands to:
> 
>   wal_level >= WAL_LEVEL_LOGICAL &&
>   relation->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT &&
>   (wal_level >= WAL_LEVEL_REPLICA || ...)
> 
> The proposed change doesn't affect semantics, and it likely doesn't change
> optimized code.  I slightly prefer to leave this line unchanged.

Yes. The macro is correct whether we do the change or not. And I din't
make my mind that it is the right fix consdering back-patch
burden. The reason for the change was simply that the macro is
irrelevant to the rest of the old RelationNeedsWAL() macro other than
relpersistence.

So.. (?) I reverted it in this version.

> >       (IsCatalogRelation(relation) || RelationIsUsedAsCatalogTable(relation)))
> >  
> >  /*
> > @@ -635,7 +642,7 @@ typedef struct ViewOptions
> >   */
> >  #define RelationIsLogicallyLogged(relation) \
> >      (XLogLogicalInfoActive() && \
> > -     RelationNeedsWAL(relation) && \
> > +     RelationIsWalLogged(relation) && \
> 
> Likewise.
> 
> >       !IsCatalogRelation(relation))
> >  
> >  /* routines in utils/cache/relcache.c */
> > diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
> > index 579be352c5..7be922a9f1 100644
> > --- a/src/include/utils/snapmgr.h
> > +++ b/src/include/utils/snapmgr.h
> > @@ -37,7 +37,7 @@
> >   */
> >  #define RelationAllowsEarlyPruning(rel) \
> >  ( \
> > -     RelationNeedsWAL(rel) \
> > +     RelationIsWalLogged(rel) \
> 
> I suspect this is user-visible for a scenario like:
> 
> CREATE TABLE t AS SELECT ...; DELETE FROM t;
> -- ... time passes, rows of t are now eligible for early pruning ...
> BEGIN;
> ALTER TABLE t SET TABLESPACE something;  -- start skipping WAL
> SELECT count(*) FROM t;
> 
> After this patch, the SELECT would do early pruning, as it does in the absence
> of the ALTER TABLE.  When pruning doesn't update the page LSN,
> TestForOldSnapshot() will be unable to detect that early pruning changed the
> query results.  Hence, RelationAllowsEarlyPruning() must not change this way.
> Does that sound right?

Mmm, maybe no. The pruning works on XID (or timestamp), not on LSN, so
it seems to work well even if pruning happened at the SELECT.
Conversely that should work after old_snapshot_threshold elapsed.

Am I missing something?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 21d6af723fcba80a8db330ef7ec9af386755fac6 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Mon, 18 Jan 2021 14:47:21 +0900
Subject: [PATCH v2] Do not use RelationNeedsWAL to identify relation
 persistence

RelationNeedsWAL() may return false for a permanent relation when
wal_level=minimal and the relation is created or truncated in the
current transaction. Directly examine relpersistence instead of using
the function to know relation persistence.
---
 src/backend/catalog/pg_publication.c       |  2 +-
 src/backend/optimizer/util/plancat.c       |  3 ++-
 src/include/utils/rel.h                    |  9 ++++++++-
 src/include/utils/snapmgr.h                |  2 +-
 src/test/subscription/t/001_rep_changes.pl | 20 +++++++++++++++++++-
 5 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 5f8e1c64e1..84d2efcfd2 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 da322b453e..177e6e336a 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/include/utils/rel.h b/src/include/utils/rel.h
index 10b63982c0..1e2c11fdd3 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -552,9 +552,16 @@ typedef struct ViewOptions
         (relation)->rd_smgr->smgr_targblock = (targblock); \
     } while (0)
 
+/*
+ * RelationIsPermanent
+ *        True if relation is WAL-logged.
+ */
+#define RelationIsWalLogged(relation)                                    \
+    ((relation)->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT)
+
 /*
  * RelationNeedsWAL
- *        True if relation needs WAL.
+ *        True if relation needs WAL at the time.
  *
  * Returns false if wal_level = minimal and this relation is created or
  * truncated in the current transaction.  See "Skipping WAL for New
diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
index 579be352c5..35acccb29c 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 0680f44a1a..ed9b48e8bc 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');
-- 
2.27.0