Horton

Re: new heapcheck contrib module

Mark Dilger <mark.dilger@enterprisedb.com>

View thread View on postgr.es
From: Mark Dilger <mark.dilger@enterprisedb.com>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Peter Geoghegan <pg@bowt.ie>, Andres Freund <andres@anarazel.de>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2020-06-11T19:10:06Z
Lists: pgsql-hackers

Attachments


> On Jun 11, 2020, at 9:14 AM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> 
> I have just browsed through the patch and the idea is quite
> interesting.  I think we can expand it to check that whether the flags
> set in the infomask are sane or not w.r.t other flags and xid status.
> Some examples are
> 
> - If HEAP_XMAX_LOCK_ONLY is set in infomask then HEAP_KEYS_UPDATED
> should not be set in new_infomask2.
> - If HEAP_XMIN(XMAX)_COMMITTED is set in the infomask then can we
> actually cross verify the transaction status from the CLOG and check
> whether is matching the hint bit or not.
> 
> While browsing through the code I could not find that we are doing
> this kind of check,  ignore if we are already checking this.

Thanks for taking a look!

Having both of those bits set simultaneously appears to fall into a different category than what I wrote verify_heapam.c to detect.  It doesn't violate any assertion in the backend, nor does it cause the code to crash.  (At least, I don't immediately see how it does either of those things.)  At first glance it appears invalid to have those bits both set simultaneously, but I'm hesitant to enforce that without good reason.  If it is a good thing to enforce, should we also change the backend code to Assert?

I integrated your idea into one of the regression tests.  It now sets these two bits in the header of one of the rows in a table.  The verify_heapam check output (which includes all detected corruptions) does not change, which verifies your observation that verify_heapam is not checking for this.  I've attached that as a patch to this email.  Note that this patch should be applied atop the v6 patch recently posted in another email.

Commits

  1. Add pg_amcheck, a CLI for contrib/amcheck.

  2. Refactor and generalize the ParallelSlot machinery.

  3. Generalize parallel slot result handling.

  4. Move some code from src/bin/scripts to src/fe_utils to permit reuse.

  5. Factor pattern-construction logic out of processSQLNamePattern.

  6. Doc: clean up verify_heapam() documentation.

  7. Fix more portability issues in new amcheck code.

  8. Fix portability issues in new amcheck test.

  9. Try to avoid a compiler warning about using fxid uninitialized.

  10. Extend amcheck to check heap pages.

  11. Adjust walsender usage of xlogreader, simplify APIs

  12. Improve checking of child pages in contrib/amcheck.

  13. Sanitize line pointers within contrib/amcheck.

  14. Fix possible sorting error when aborting use of abbreviated keys.