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
signature.asc
Description: PGP signature