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@leadboat.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()?