Wrong usage of RelationNeedsWAL

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Wrong usage of RelationNeedsWAL

Kyotaro Horiguchi-4
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 <[hidden email]>
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

Reply | Threaded
Open this post in threaded view
|

Re: Wrong usage of RelationNeedsWAL

Andres Freund
Hi,

On 2021-01-13 16:07:05 +0900, Kyotaro Horiguchi wrote:

> 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.

Noah?

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Wrong usage of RelationNeedsWAL

Noah Misch-2
In reply to this post by Kyotaro Horiguchi-4
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?

>   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?

- Name the new macro RelationEverNeedsWAL().
- Just test "relpersistence == RELPERSISTENCE_PERMANENT", 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.

>   (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?