Re: new heapcheck contrib module

On Sun, Jun 28, 2020 at 8:59 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Jun 22, 2020 at 5:44 AM Mark Dilger
> <mark.dilger@enterprisedb.com> wrote:
> >
> >
> >
> > > On Jun 21, 2020, at 2:54 AM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > I have looked into 0001 patch and I have a few comments.
> > >
> > > 1.
> > > +
> > > + /* Skip over unused/dead/redirected line pointers */
> > > + if (!ItemIdIsUsed(ctx.itemid) ||
> > > + ItemIdIsDead(ctx.itemid) ||
> > > + ItemIdIsRedirected(ctx.itemid))
> > > + continue;
> > >
> > > Isn't it a good idea to verify the Redirected Itemtid?  Because we
> > > will still access the redirected item id to find the
> > > actual tuple from the index scan.  Maybe not exactly at this level,
> > > but we can verify that the link itemid store in that
> > > is within the itemid range of the page or not.
> >
> > Good idea.  I've added checks that the redirection is valid, both in terms of being within bounds and in terms of alignment.
> >
> > > 2.
> > >
> > > + /* Check for tuple header corruption */
> > > + if (ctx->tuphdr->t_hoff < SizeofHeapTupleHeader)
> > > + {
> > > + confess(ctx,
> > > + psprintf("t_hoff < SizeofHeapTupleHeader (%u < %u)",
> > > + ctx->tuphdr->t_hoff,
> > > + (unsigned) SizeofHeapTupleHeader));
> > > + fatal = true;
> > > + }
> > >
> > > I think we can also check that if there is no NULL attributes (if
> > > (!(t_infomask & HEAP_HASNULL)) then
> > > ctx->tuphdr->t_hoff should be equal to SizeofHeapTupleHeader.
> >
> > You have to take alignment padding into account, but otherwise yes, and I've added a check for that.
> >
> > > 3.
> > > + ctx->offset = 0;
> > > + for (ctx->attnum = 0; ctx->attnum < ctx->natts; ctx->attnum++)
> > > + {
> > > + if (!check_tuple_attribute(ctx))
> > > + break;
> > > + }
> > > + ctx->offset = -1;
> > > + ctx->attnum = -1;
> > >
> > > So we are first setting ctx->offset to 0, then inside
> > > check_tuple_attribute, we will keep updating the offset as we process
> > > the attributes and after the loop is over we set ctx->offset to -1,  I
> > > did not understand that why we need to reset it to -1, do we ever
> > > check for that.  We don't even initialize the ctx->offset to -1 while
> > > initializing the context for the tuple so I do not understand what is
> > > the meaning of the random value -1.
> >
> > Ahh, right, those are left over from a previous design of the code.  Thanks for pointing them out.  They are now removed.
> >
> > > 4.
> > > + if (!VARATT_IS_EXTENDED(chunk))
> > > + {
> > > + chunksize = VARSIZE(chunk) - VARHDRSZ;
> > > + chunkdata = VARDATA(chunk);
> > > + }
> > > + else if (VARATT_IS_SHORT(chunk))
> > > + {
> > > + /*
> > > + * could happen due to heap_form_tuple doing its thing
> > > + */
> > > + chunksize = VARSIZE_SHORT(chunk) - VARHDRSZ_SHORT;
> > > + chunkdata = VARDATA_SHORT(chunk);
> > > + }
> > > + else
> > > + {
> > > + /* should never happen */
> > > + confess(ctx,
> > > + pstrdup("toast chunk is neither short nor extended"));
> > > + return;
> > > + }
> > >
> > > I think the error message "toast chunk is neither short nor extended".
> > > Because ideally, the toast chunk should not be further toasted.
> > > So I think the check is correct, but the error message is not correct.
> >
> > I agree the error message was wrongly stated, and I've changed it, but you might suggest a better wording than what I came up with, "corrupt toast chunk va_header".
> >
> > > 5.
> > >
> > > + ctx.rel = relation_open(relid, ShareUpdateExclusiveLock);
> > > + check_relation_relkind_and_relam(ctx.rel);
> > > +
> > > + /*
> > > + * Open the toast relation, if any, also protected from concurrent
> > > + * vacuums.
> > > + */
> > > + if (ctx.rel->rd_rel->reltoastrelid)
> > > + {
> > > + int offset;
> > > +
> > > + /* Main relation has associated toast relation */
> > > + ctx.toastrel = table_open(ctx.rel->rd_rel->reltoastrelid,
> > > +   ShareUpdateExclusiveLock);
> > > + offset = toast_open_indexes(ctx.toastrel,
> > > ....
> > > + if (TransactionIdIsNormal(ctx.relfrozenxid) &&
> > > + TransactionIdPrecedes(ctx.relfrozenxid, ctx.oldestValidXid))
> > > + {
> > > + confess(&ctx, psprintf("relfrozenxid %u precedes global "
> > > +    "oldest valid xid %u ",
> > > +    ctx.relfrozenxid, ctx.oldestValidXid));
> > > + PG_RETURN_NULL();
> > > + }
> > >
> > > Don't we need to close the relation/toastrel/toastindexrel in such
> > > return which is without an abort? IIRC, we
> > > will get relcache leak WARNING on commit if we left them open in commit path.
> >
> > Ok, I've added logic to close them.
> >
> > All changes inspired by your review are included in the v9-0001 patch.  The differences since v8 are pulled out into v9_diffs for easier review.
>
> I have reviewed the changes in v9_diffs and looks fine to me.

Some more comments on v9_0001.
1.
+ LWLockAcquire(XidGenLock, LW_SHARED);
+ nextFullXid = ShmemVariableCache->nextFullXid;
+ ctx.oldestValidXid = ShmemVariableCache->oldestXid;
+ LWLockRelease(XidGenLock);
+ ctx.nextKnownValidXid = XidFromFullTransactionId(nextFullXid);
...
...
+
+ for (ctx.blkno = startblock; ctx.blkno < endblock; ctx.blkno++)
+ {
+ int32 mapbits;
+ OffsetNumber maxoff;
+ PageHeader ph;
+
+ /* Optionally skip over all-frozen or all-visible blocks */
+ if (skip_all_frozen || skip_all_visible)
+ {
+ mapbits = (int32) visibilitymap_get_status(ctx.rel, ctx.blkno,
+    &vmbuffer);
+ if (skip_all_visible && (mapbits & VISIBILITYMAP_ALL_VISIBLE) != 0)
+ continue;
+ if (skip_all_frozen && (mapbits & VISIBILITYMAP_ALL_FROZEN) != 0)
+ continue;
+ }
+
+ /* Read and lock the next page. */
+ ctx.buffer = ReadBufferExtended(ctx.rel, MAIN_FORKNUM, ctx.blkno,
+ RBM_NORMAL, ctx.bstrategy);
+ LockBuffer(ctx.buffer, BUFFER_LOCK_SHARE);

I might be missing something, but it appears that first we are getting
the nextFullXid and after that, we are scanning the block by block.
So while we are scanning the block if the nextXid is advanced and it
has updated some tuple in the heap pages,  then it seems the current
logic will complain about out of range xid.  I did not test this
behavior so please point me to the logic which is protecting this.

2.
/*
 * Helper function to construct the TupleDesc needed by verify_heapam.
 */
static TupleDesc
verify_heapam_tupdesc(void)

From function name, it appeared that it is verifying tuple descriptor
but this is just creating the tuple descriptor.

3.
+ /* Optionally skip over all-frozen or all-visible blocks */
+ if (skip_all_frozen || skip_all_visible)
+ {
+ mapbits = (int32) visibilitymap_get_status(ctx.rel, ctx.blkno,
+    &vmbuffer);
+ if (skip_all_visible && (mapbits & VISIBILITYMAP_ALL_VISIBLE) != 0)
+ continue;
+ if (skip_all_frozen && (mapbits & VISIBILITYMAP_ALL_FROZEN) != 0)
+ continue;
+ }

Here, do we want to test that in VM the all visible bit is set whereas
on the page it is not set?  That can lead to a wrong result in an
index-only scan.

4. One cosmetic comment

+ /* Skip non-varlena values, but update offset first */
..
+
+ /* Ok, we're looking at a varlena attribute. */

Throughout the patch, I have noticed that some of your single-line
comments have "full stop" whereas other don't.  Can we keep them
consistent?

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