Re: Wrong usage of RelationNeedsWAL

On Sun, Jan 17, 2021 at 10:36:31PM -0800, Noah Misch wrote:
> On Mon, Jan 18, 2021 at 03:08:38PM +0900, Kyotaro Horiguchi wrote:
> > 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:
> > > > --- 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?
>
> 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:

setup: CREATE TABLE t AS SELECT ...;
xact1: BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT 1; -- take snapshot
xact2: DELETE FROM t;
(plenty of time passes)
xact3: SELECT count(*) FROM t; -- early pruning
xact1: SAVEPOINT q; SELECT count(*) FROM t; ROLLBACK TO q; -- "snapshot too old"
xact1: ALTER TABLE t SET TABLESPACE something; -- start skipping WAL
xact1: SELECT count(*) FROM t; -- no error, wanted "snapshot too old"

Is that plausible?