Re: Wrong usage of RelationNeedsWAL

At Tue, 19 Jan 2021 01:31:52 -0800, Noah Misch <noah@leadboat.com> wrote in 
> On Tue, Jan 19, 2021 at 01:48:31PM +0900, Kyotaro Horiguchi wrote:
> > 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.
> 
> I agree the above procedure doesn't have a problem under your patch versions.
> See https://postgr.es/m/20210116043816.GA1644261@rfd.leadboat.com for a
> different test case.  In more detail:
> 
A> setup: CREATE TABLE t AS SELECT ...;
B> xact1: BEGIN; DELETE FROM t;
C> xact2: BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT 1;  -- take snapshot
D> xact1: COMMIT;
> (plenty of time passes, rows of t are now eligible for early pruning)
E> xact3: BEGIN;
F> xact3: ALTER TABLE t SET TABLESPACE something;  -- start skipping WAL
G> xact3: SELECT count(*) FROM t;  -- early pruning w/o WAL, w/o LSN changes

H> xact3: COMMIT;
J> xact2: SELECT count(*) FROM t;  -- no error, wanted "snapshot too old"
> 
> I expect that shows no bug for git master, but I expect it does show a bug

I didn't see "snapshot too old" at J with the patch, but at the same
time the SELECT at G didn't prune tuples almost all of the trial. (the
last SELECT returns the correct answer.) I saw it get pruned only once
among many trials but I'm not sure how I could get to the situation
and haven't succeed to reproduce that.

The reason that the SELECT at G doesn't prune is that limited_xmin in
heap_page_prune_opt at G is limited up to the oldest xid among active
sessions. In this case the xmin of the session 2 is the same with the
xid of the session 1. So xmin of the session 3 at G is the same with
the xmin of the session 2, which is the same with the xid of the
session 1.  So pruning doesn't happen. However that is very fragile as
the base for asserting that pruning won't happen.

Anyway, it seems actually dangerous that cause pruning on wal-skipped
relation.

> with your patch versions.  Could you try implementing both test procedures in
> src/test/modules/snapshot_too_old?  There's no need to make the test use
> wal_level=minimal explicitly; it's sufficient to catch these bugs when
> wal_level=minimal is in the TEMP_CONFIG file.

In the attached, TestForOldSnapshot() considers XLogIsNeeded(),
instead of moving the condition on LSN to TestForOldSnapshot_impl for
performance.

I'll add the test part in the next version.

regards.
-- 
Kyotaro Horiguchi
NTT Open Source Software Center