> On Jun 11, 2020, at 9:14 AM, Dilip Kumar <dilipbal...@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.
WIP_dilip_kumar_idea.patch
Description: Binary data
— Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
