Re: Wrong usage of RelationNeedsWAL

At Thu, 21 Jan 2021 00:19:58 -0800, Noah Misch <noah@leadboat.com> wrote in 
> On Thu, Jan 21, 2021 at 12:28:44AM +0900, Kyotaro Horiguchi wrote:
> > At Wed, 20 Jan 2021 17:34:44 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > > 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.
> 
> That test helped me.  I now see "there's not a single tuple removed due to
> old_snapshot_threshold in src/test/modules/snapshot_too_old"[1], which limits
> our ability to test using this infrastructure.

Yes.

> > However, with the previous patch, two existing tests sto_using_cursor
> > and sto_using_select behaves differently from the master.  That change
> > is coming from the omission of actual LSN check in TestForOldSnapshot
> > while wal_level=minimal. So we have no choice other than actually
> > updating page LSN.
> > 
> > In the scenario under discussion there are two places we need to do
> > that. one is heap_page_prune, and the other is RelationCopyStorge. As
> > a PoC, I used gistXLogAssignLSN() as is for thie purpose. See the
> > attached third file.
> 
> Fake LSNs make the system harder to understand, so I prefer not to spread fake
> LSNs to more access methods.  What I had in mind is to simply suppress early
> pruning when the relation is skipping WAL.  Attached.  Is this reasonable?  It
> passes the older tests.  While it changes the sto_wal_optimized.spec output, I
> think it preserves the old_snapshot_threshold behavior contract.

Perhaps I'm missing something, but the patch doesn't pass the v5-0001
test with wal_level=minimal?

> [1] https://www.postgresql.org/message-id/20200403001235.e6jfdll3gh2ygbuc%40alap3.anarazel.de

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center