Re: Wrong usage of RelationNeedsWAL

At Mon, 18 Jan 2021 17:30:22 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Sun, 17 Jan 2021 23:02:18 -0800, Noah Misch <noah@leadboat.com> wrote in 
> > On Sun, Jan 17, 2021 at 10:36:31PM -0800, Noah Misch wrote:
> > > I wrote the above based on the "PageGetLSN(page) > (snapshot)->lsn" check in
> > > TestForOldSnapshot().  If the LSN isn't important, what else explains
> > > RelationAllowsEarlyPruning() checking RelationNeedsWAL()?
> > 
> > Thinking about it more, some RelationAllowsEarlyPruning() callers need
> > different treatment.  Above, I was writing about the case of deciding whether
> > to do early pruning.  The other RelationAllowsEarlyPruning() call sites are
> > deciding whether the relation might be lacking old data.  For that case, we
> > should check RELPERSISTENCE_PERMANENT, not RelationNeedsWAL().  Otherwise, we
> > could fail to report an old-snapshot error in a case like this:
> > 
A> > setup: CREATE TABLE t AS SELECT ...;
B> > xact1: BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT 1;  -- take snapshot
C> > xact2: DELETE FROM t;
D> > (plenty of time passes)
E> > xact3: SELECT count(*) FROM t;  -- early pruning
F> > xact1: SAVEPOINT q; SELECT count(*) FROM t; ROLLBACK TO q;  -- "snapshot too old"
G> > xact1: ALTER TABLE t SET TABLESPACE something;  -- start skipping WAL
H> > xact1: SELECT count(*) FROM t;  -- no error, wanted "snapshot too old"
> > 
> > Is that plausible?
> 
> Thank you for the consideration and yes. But I get "snapshot too old"
> from the last query with the patched version. maybe I'm missing
> something. I'm going to investigate the case.

Ah. I took that in reverse way. (caught by the discussion on page
LSN.) Ok, the "current" code works that way. So we need to perform the
check the new way in RelationAllowsEarlyPruning. So in a short answer
for the last question in regard to the reference side is yes.

I understand that you are suggesting that at least
TransactionIdLimitedForOldSnapshots should follow not only relation
persistence but RelationNeedsWAL, based on the discussion on the
check on LSN of TestForOldSnapshot().

I still don't think that LSN in the WAL-skipping-created relfilenode
harm.  ALTER TABLE SET TABLESPACE always makes a dead copy of every
block (except checksum) including page LSN regardless of wal_level. In
the scenario above, the last select at H correctly reads page LSN set
by E then copied by G, which is larger than the snapshot LSN at B. So
doesn't go the fast-return path before actual check performed by
RelationAllowsEarlyPruning.

As the result still RelationAllowsEarlyPruning is changed to check
only for the persistence of the table in this version. (In other
words, all the callers of the function works based on the same
criteria.)

- Removed RelationIsWalLoggeed which was forgotten to remove in the
  last version.

- Enclosed parameter of RelationAllowsEarlyPruning.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center