Re: Wrong usage of RelationNeedsWAL

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()?