Re: new heapcheck contrib module

> On Jul 30, 2020, at 1:47 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> On 2020-07-30 13:18:01 -0700, Mark Dilger wrote:
>> Per tuple, tuple_is_visible() potentially checks whether the xmin or xmax committed via TransactionIdDidCommit. I am worried about concurrent truncation of clog entries causing I/O errors on SLRU lookup when performing that check. The three strategies I had for dealing with that were taking the XactTruncationLock (formerly known as CLogTruncationLock, for those reading this thread from the beginning), locking out vacuum, and the idea upthread from Andres about setting PROC_IN_VACUUM and such. Maybe I'm being dense and don't need to worry about this. But I haven't convinced myself of that, yet.
>
> I think it's not at all ok to look in the procarray or clog for xids
> that are older than what you're announcing you may read. IOW I don't
> think it's OK to just ignore the problem, or try to work around it by
> holding XactTruncationLock.

The current state of the patch is that concurrent vacuums are kept out of the table being checked by means of taking a ShareUpdateExclusive lock on the table being checked. In response to Robert's review, I was contemplating whether that was necessary, but you raise the interesting question of whether it is even sufficient. The logic in verify_heapam is currently relying on the ShareUpdateExclusive lock to prevent any of the xids in the range relfrozenxid..nextFullXid from being invalid arguments to TransactionIdDidCommit. Ignoring whether that is a good choice vis-a-vis performance, is that even a valid strategy? It sounds like you are saying it is not.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company