Re: [Patch] Optimize dropping of relation buffers using dlist

Oops! Sorry for the mistake.

At Fri, 09 Oct 2020 11:12:01 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Fri, 9 Oct 2020 00:41:24 +0000, "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com> wrote in 
> > From: Jamison, Kirk/ジャミソン カーク <k.jamison@fujitsu.com>
> > > > (6)
> > > > +                    bufHdr->tag.blockNum >=
> > > > firstDelBlock[j])
> > > > +                    InvalidateBuffer(bufHdr);    /*
> > > > releases spinlock */
> > > >
> > > > The right side of >= should be cur_block.
> > > 
> > > Fixed.
> > 
> > >= should be =, shouldn't it?
> > 
> > Please measure and let us see just the recovery performance again because the critical part of the patch was
modified. If the performance is good as the previous one, and there's no review interaction with others in progress,
I'llmark the patch as ready for committer in a few days.
 
> 
> The performance is expected to be kept since smgrnblocks() is called
> in a non-hot code path and actually it is called at most four times
> per a buffer drop in this patch. But it's better making it sure.
> 
> I have some comments on the latest patch.
> 
> @@ -445,6 +445,7 @@ BlockNumber
>  visibilitymap_prepare_truncate(Relation rel, BlockNumber nheapblocks)
>  {
>      BlockNumber newnblocks;
> +    bool    cached;
> 
> All the added variables added by 0002 is useless because all the
> caller sites are not interested in the value. smgrnblocks should
> accept NULL as isCached. (I'm agree with Tsunakawa-san that the
> camel-case name is not common there.)
> 
> +        nForkBlocks[i] = smgrnblocks(smgr_reln, forkNum[i], &isCached);
> +
> +        if (!isCached)
> 
> "is cached" is not the property that code is interested in. No other callers to smgrnblocks are interested in that
property.The need for caching is purely internal of smgrnblocks().
 
> 
> On the other hand, we are going to utilize the property of "accuracy"
> that is a biproduct of reducing fseek calls, and, again, not
> interested in how it is achieved.
> 
> So I suggest that the name should be "accurite" or something that is
> not suggest the mechanism used under the hood.
> 
> +    if (nTotalBlocks != InvalidBlockNumber &&
> +        nBlocksToInvalidate < BUF_DROP_FULL_SCAN_THRESHOLD)
> 
> I don't think nTotalBlocks is useful. What we need here is only total
> blocks for every forks (nForkBlocks[]) and the total number of buffers
> to be invalidated for all forks (nBlocksToInvalidate).
> 
> 
> > > > The right side of >= should be cur_block.
> > > 
> > > Fixed.
> > 
> > >= should be =, shouldn't it?
> 
> It's just from a paranoia. What we are going to invalidate is blocks
> blockNum of which >= curBlock. Although actually there's no chance of

Sorry. What we are going to invalidate is blocks that are blocNum >=
firstDelBlock[i]. So what I wanted to suggest was the condition should
be

+                if (RelFileNodeEquals(bufHdr->tag.rnode, rnode.node) &&
+                    bufHdr->tag.forkNum == forkNum[j] &&
+                    bufHdr->tag.blockNum >= firstDelBlock[j])

> any other processes having replaced the buffer with another page (with
> lower blockid) of the same relation after BugTableLookup(), that
> condition makes it sure not to leave blocks to be invalidated left
> alone.

And I forgot to mention the patch names. I think many of us name the
patches using -v option of git-format-patch, and assign the version to
a patch-set thus the version number of all files that are posted at
once is same.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center