Date: Fri, 15 Jan 2021 20:38:16 -0800
From: Noah Misch <noah@leadboat.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-hackers@postgresql.org, andres@anarazel.de
Subject: Re: Wrong usage of RelationNeedsWAL
Message-ID: <20210116043816.GA1644261@rfd.leadboat.com>
References: <20210113.160705.2225256954956139776.horikyota.ntt@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210113.160705.2225256954956139776.horikyota.ntt@gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

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?