Wrong usage of RelationNeedsWAL - Mailing list pgsql-hackers

Hello.

Commit c6b92041d3 changed the definition of RelationNeedsWAL().

-#define RelationNeedsWAL(relation) \
-    ((relation)->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT)
+#define RelationNeedsWAL(relation)                                        \
+    ((relation)->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT &&    \
+     (XLogIsNeeded() ||                                                    \
+      (relation->rd_createSubid == InvalidSubTransactionId &&            \
+       relation->rd_firstRelfilenodeSubid == InvalidSubTransactionId)))

On the other hand I found this usage.

plancat.c:128 get_relation_info()
>    /* Temporary and unlogged relations are inaccessible during recovery. */
>    if (!RelationNeedsWAL(relation) && RecoveryInProgress())
>        ereport(ERROR,
>                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>                 errmsg("cannot access temporary or unlogged relations during recovery")));

It works as expected accidentally, but the meaning is off.
WAL-skipping optmization is irrelevant to the condition for the error.

I found five misues in the tree. Please find the attached.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 2912e1db38ff034e27e4010eff5b2e5afcce3f85 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Wed, 13 Jan 2021 15:52:03 +0900
Subject: [PATCH] Fix misuses of RelationNeedsWAL

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.
---
 src/backend/catalog/pg_publication.c |  2 +-
 src/backend/optimizer/util/plancat.c |  2 +-
 src/include/utils/rel.h              | 15 +++++++++++----
 src/include/utils/snapmgr.h          |  2 +-
 4 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 5f8e1c64e1..f3060a4cf1 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 (!RelationIsWalLogged(targetrel))
         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())
         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)
+
 /*
  * 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) && \
      (IsCatalogRelation(relation) || RelationIsUsedAsCatalogTable(relation)))
 
 /*
@@ -635,7 +642,7 @@ typedef struct ViewOptions
  */
 #define RelationIsLogicallyLogged(relation) \
     (XLogLogicalInfoActive() && \
-     RelationNeedsWAL(relation) && \
+     RelationIsWalLogged(relation) && \
      !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) \
   && !IsCatalogRelation(rel) \
   && !RelationIsAccessibleInLogicalDecoding(rel) \
 )
-- 
2.27.0