On Thu, Oct 29, 2020 at 2:17 PM Andres Freund <and...@anarazel.de> wrote: > I think this needs to be quickly reworked or reverted.
I don't like this patch, either. In addition to what Andres mentioned, CheckBuffer() is a completely-special case mechanism which can't be reused by anything else. In particular, the amcheck for heap stuff which I recently committed (866e24d47db1743dfcff5bd595b57e3a143f2cb1) would really like a way to examine a buffer without risking an error if PageIsVerified() should happen to fail, but this patch is of absolutely no use in getting there, because CheckBuffer() doesn't give the caller any way to access the contents of the buffer. It can only do the checks that it knows how to do, and that's it. That doesn't seem like a good design. I don't like the fact that CheckBuffer() silently skips dirty buffers, either. The comment should really say that it checks the state of a buffer without loading it into shared buffers, except sometimes it doesn't actually check it. That doesn't seem like the behavior users really want, and it's not clear that there is any really good reason for it. If the buffer is in shared buffers, we could take a share-lock on the buffer and copy the contents of the page as it exists on disk, and then still check it. It feels really confusing to me that the user-exposed function here is called pg_relation_check_pages(). How is the user supposed to understand the difference between what this function does and what the new verify_heapam() in amcheck does? The answer is that the latter does far more extensive checks, but this isn't obvious from the SGML documentation, which says only that the blocks are "verified," as if an end-user can reasonably be expected to know what that means. It seems likely to lead users to the belief that if this function passes, they are in good shape, which is extremely far from being true. Just look at what PageIsVerified() checks compared to what verify_heapam() checks. In fact, I would argue that this functionality ought to live in amcheck rather than core, though there could usefully be enabling functions in core. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
