On Thu, Oct 29, 2020 at 2:17 PM Andres Freund <andres@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
