Re: Online checksums verification in the backend

On Fri, Sep 25, 2020 at 06:11:47PM +0800, Julien Rouhaud wrote:
> Thanks a lot for the tests!  I'm not surprised that forcing the lock
> will slow down the pg_check_relation() execution, but I'm a bit
> surprised that holding the buffer mapping lock longer in a workload
> that has a lot of evictions actually makes things faster.  Do you have
> any idea why that's the case?

That's still a bit unclear to me, but I have not spent much time
thinking about this particular point either.

> I'm assuming that you prefer to remove both the optimization and the
> throttling part?  I'll do that with the next version unless there's
> objections.

Yeah, any tests I have done tends to show that.  It would be good to
also check some perf profiles here, at least for the process running
the relation check in a loop.

> I agree that putting the code nearby ReadBuffer_common() would be a
> good idea.  However, that means that I can't move all the code to
> contrib/  I'm wondering what you'd like to see going there.  I can see
> some values in also having the SQL functions available in core rather
> than contrib, e.g. if you need to quickly check a relation on a
> standby, so without requiring to create the extension on the primary
> node first.

Good point.  This could make the user experience worse.

>  Then, I'm a bit worried about adding this code in ReadBuffer_common.
> What this code does is quite different, and I'm afraid that it'll make
> ReadBuffer_common more complex than needed, which  is maybe not a good
> idea for something as critical as this function.
> 
> What do you think?

Yeah, I have been looking at ReadBuffer_common() and it is true that
it is complicated enough so we may not really need an extra mode or
more options, for a final logic that is actually different than what a
buffer read does: we just want to know if a page has a valid checksum
or not.  An idea that I got here would be to add a new, separate
function to do the page check directly in bufmgr.c, but that's what
you mean.  Now only the prefetch routine and ReadBuffer_common use
partition locks, but getting that done in the same file looks like a
good compromise to me.  It would be also possible to keep the BLCKSZ
buffer used to check the page directly in this routine, so as any
caller willing to do a check don't need to worry about any
allocation.
--
Michael