On Thu, Jul 16, 2015 at 11:21 AM, Haribabu Kommi <
kommi.haribabu@gmail.com> wrote:
>
> On Wed, Jul 15, 2015 at 11:19 PM, Amit Kapila <
amit.kapila16@gmail.com> wrote:
> >
> > One case where this patch can behave differently then current
> > code is, when before taking Exclusive lock on relation for truncation,
> > if some backend clears the vm bit and then inserts-deletes-prune that
> > page. I think with patch it will not consider to truncate pages whereas
> > current code will allow to truncate it as it re-verifies each page. I think
> > such a case would be rare and we might not want to bother about it,
> > but still worth to think about it once.
>
> Thanks for your review.
>
> corrected the code as instead of returning the blkno after visibility map
> check failure, the code just continue to verify the contents as
> earlier approach.
>
Okay, I think this should work.
>
> Here I attached updated patches,
> 1) without prefetch logic.
> 2) with combined vm and prefetch logic.
>
I think it is better to just get the first patch in as that in itself is a
clear win and then we can evaluate the second one (prefetching during
truncation) separately. I think after first patch, the use case for doing
prefetch seems to be less beneficial and I think it could hurt by loading
not required pages (assume you have prefetched 32 pages and after
inspecting first page, it decides to quit the loop as that is non-empty page
and other is when it has prefetched just because for page the visibility map
bit was cleared, but for others it is set) in OS buffer cache.
Having said that, I am not against prefetching in this scenario as that
can help in more cases then it could hurt, but I think it will be better
to evaluate that separately.