Re: pg_amcheck contrib application

On Mon, Mar 29, 2021 at 1:45 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> Thanks!  The attached patch addresses your comments here and in your prior email.  In particular, this patch changes
thetuple visibility logic to not check tuples for which the inserting transaction aborted or is still in progress, and
tonot check toast for tuples deleted in transactions older than our transaction snapshot's xmin.  A list of toasted
attributeswhich are safe to check is compiled per main table page during the scan of the page, then checked after the
bufferlock on the main page is released. 
>
> In the perhaps unusual case where verify_heapam() is called in a transaction which has also added tuples to the table
beingchecked, this patch's visibility logic chooses not to check such tuples.  I'm on the fence about this choice, and
ammostly following your lead.  I like that this decision maintains the invariant that we never check tuples which have
notyet been committed. 
>
> The patch includes a bit of refactoring.  In the old code, heap_check() performed clog bounds checking on xmin and
xmaxprior to calling check_tuple_header_and_visibilty(), but I think that's not such a great choice.  If the tuple
headeris garbled to have random bytes in the xmin and xmax fields, and we can detect that situation because other tuple
headerfields are garbled in detectable ways, I'd rather get a report about the header being garbled than a report about
thexmin or xmax being out of bounds.  In the new code, the tuple header is checked first, then the visibility is
checked,then the tuple is checked against the current relation description, then the tuple attributes are checked.  I
thinkthe layout is easier to follow, too. 

Hmm, so this got ~10x bigger from my version. Could you perhaps
separate it out into a series of patches for easier review? Say, one
that just fixes the visibility logic, and then a second to avoid doing
the TOAST check with a buffer lock held, and then more than that if
there are other pieces that make sense to separate out?

--
Robert Haas
EDB: http://www.enterprisedb.com