Re: Wrong usage of RelationNeedsWAL

Thank you for the comments, Noah and Andres.

At Fri, 15 Jan 2021 20:38:16 -0800, Noah Misch <noah(at)leadboat(dot)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