Re: new heapcheck contrib module

On Fri, Jun 12, 2020 at 12:40 AM Mark Dilger
<mark(dot)dilger(at)enterprisedb(dot)com> wrote:
>
>
>
> > On Jun 11, 2020, at 9:14 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)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.

Ok

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?

Yeah, it may not hit assert or crash but it could lead to a wrong
result. But I agree that it could be an assertion in the backend
code. What about the other check, like hint bit is saying the
transaction is committed but actually as per the clog the status is
something else. I think in general processing it is hard to check
such things in backend no? because if the hint bit is set saying that
the transaction is committed then we will directly check its
visibility with the snapshot. I think a corruption checker may be a
good tool for catching such anomalies.

> 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 verifies _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.

Ok.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com