Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\))
Subject: Re: new heapcheck contrib module
From: Mark Dilger <mark.dilger@enterprisedb.com>
In-Reply-To: 
 <CAFiTN-tTg0R5AHvXC1ro9XphroX1wUtc5t91hU_+7DeGTpE5VQ@mail.gmail.com>
Date: Sun, 28 Jun 2020 10:48:38 -0700
Cc: Robert Haas <robertmhaas@gmail.com>,
 Peter Geoghegan <pg@bowt.ie>,
 Andres Freund <andres@anarazel.de>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <0C0349EB-DD85-4DA8-82C7-DF70AB714592@enterprisedb.com>
References: <12ED3DA8-25F0-4B68-937D-D907CFBF08E7@enterprisedb.com>
 <20200420194210.sevnpi4lyghoanxi@alap3.anarazel.de>
 <23E60C3C-C20A-4508-84AC-E0CEA59E784A@enterprisedb.com>
 <CA+TgmobAm8vB1MNQ1taO1et+HxTN8ULnQvEG+grNDqNQTBjbtA@mail.gmail.com>
 <F845AC50-A479-43F3-ADFB-861A4D2AD287@enterprisedb.com>
 <5F743835-3399-419C-8324-2D424237E999@enterprisedb.com>
 <CAFiTN-syyHc3jZoou51v0SR8z0POoNfktqEO6MaGig4YS8mosA@mail.gmail.com>
 <E6F4258A-B661-4F2C-8A35-278C9436930A@enterprisedb.com>
 <CAFiTN-s-VX3DLfqv7OOo0zGCTOZ3xVU9UZ6GgzBZdJTPNnYFNg@mail.gmail.com>
 <885D2EC9-7D06-4903-A17D-72C184A1E1C5@enterprisedb.com>
 <CAFiTN-uYg_PiZ854=5R-D9skratapAHKhaDAG7fhJv-Vn5UhaQ@mail.gmail.com>
 <5CC85F28-F210-47AC-8DA8-EC4E82AF3FD5@enterprisedb.com>
 <CAFiTN-sP5iFJGERTq4a4OLTCzjZKhLC5P6_k2bxQKXnuvdmzGA@mail.gmail.com>
 <CAFiTN-tTg0R5AHvXC1ro9XphroX1wUtc5t91hU_+7DeGTpE5VQ@mail.gmail.com>
To: Dilip Kumar <dilipbalaut@gmail.com>
Precedence: bulk


> On Jun 28, 2020, at 9:05 AM, Dilip Kumar <dilipbalaut@gmail.com> =
wrote:
>=20
> Some more comments on v9_0001.
> 1.
> + LWLockAcquire(XidGenLock, LW_SHARED);
> + nextFullXid =3D ShmemVariableCache->nextFullXid;
> + ctx.oldestValidXid =3D ShmemVariableCache->oldestXid;
> + LWLockRelease(XidGenLock);
> + ctx.nextKnownValidXid =3D XidFromFullTransactionId(nextFullXid);
> ...
> ...
> +
> + for (ctx.blkno =3D 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 =3D (int32) visibilitymap_get_status(ctx.rel, ctx.blkno,
> +    &vmbuffer);
> + if (skip_all_visible && (mapbits & VISIBILITYMAP_ALL_VISIBLE) !=3D =
0)
> + continue;
> + if (skip_all_frozen && (mapbits & VISIBILITYMAP_ALL_FROZEN) !=3D 0)
> + continue;
> + }
> +
> + /* Read and lock the next page. */
> + ctx.buffer =3D ReadBufferExtended(ctx.rel, MAIN_FORKNUM, ctx.blkno,
> + RBM_NORMAL, ctx.bstrategy);
> + LockBuffer(ctx.buffer, BUFFER_LOCK_SHARE);
>=20
> 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.

We know the oldest valid Xid cannot advance, because we hold a lock that =
would prevent it from doing so.  We cannot know that the newest Xid will =
not advance, but when we see an Xid beyond the end of the known valid =
range, we check its validity, and either report it as a corruption or =
advance our idea of the newest valid Xid, depending on that check.  That =
logic is in TransactionIdValidInRel.

> 2.
> /*
> * Helper function to construct the TupleDesc needed by verify_heapam.
> */
> static TupleDesc
> verify_heapam_tupdesc(void)
>=20
> =46rom function name, it appeared that it is verifying tuple =
descriptor
> but this is just creating the tuple descriptor.

In amcheck--1.2--1.3.sql we define a function named verify_heapam which =
returns a set of records.  This is the tuple descriptor for that =
function.  I understand that the name can be parsed as =
verify_(heapam_tupdesc), but it is meant as (verify_heapam)_tupdesc.  Do =
you have a name you would prefer?

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

If the caller has specified that the corruption check should skip over =
all-frozen or all-visible data, then we cannot load the page that the VM =
claims is all-frozen or all-visible without defeating the purpose of the =
caller having specified these options.  Without loading the page, we =
cannot check the page's header bits.

When not skipping all-visible or all-frozen blocks, we might like to pin =
both the heap page and the visibility map page in order to compare the =
two, being careful not to hold a pin on the one while performing I/O on =
the other.  See for example the logic in heap_delete().  But I'm not =
sure what guarantees the system makes about agreement between these two =
bits.  Certainly, the VM should not claim a page is all visible when it =
isn't, but are we guaranteed that a page that is all-visible will always =
have its all-visible bit set?  I don't know if (possibly transient) =
disagreement between these two bits constitutes corruption.  Perhaps =
others following this thread can advise?

> 4. One cosmetic comment
>=20
> + /* Skip non-varlena values, but update offset first */
> ..
> +
> + /* Ok, we're looking at a varlena attribute. */
>=20
> 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?

I try to use a "full stop" at the end of sentences, but not at the end =
of sentence fragments.  To me, a "full stop" means that a sentence has =
reached its conclusion.  I don't intentionally use one at the end of a =
fragment, unless the fragment precedes a full sentence, in which case =
the "full stop" is needed to separate the two.  Of course, I may have =
violated my own rule in a few places, but before I submit a v10 patch =
with comment punctuation changes, perhaps we can agree on what the rule =
is?  (This has probably been discussed before and agreed before.  A link =
to the appropriate email thread would be sufficient.)

For example:

	/* red, green, or blue */
	/* set to pink */
	/* set to blue.  We have not closed the file. */
	/* At this point, we have chosen the color. */

The first comment is not a sentence, but the fourth is.  The third =
comment is a fragment followed by a full sentence, and a "full stop" =
separates the two.  As for the second comment, as I recall, verb phrases =
can be interpreted as a full sentence, as in "Close the door!", when =
they are meant as commands to the listener, but not otherwise.  "set to =
pink" is not a command to the reader, but rather a description of what =
the code is doing at that point, so I think of it as a mere verb phrase =
and not a full sentence.

Making matters even more complicated, portions of the logic in =
verify_heapam were taken from sections of code that would ereport(), =
elog(), or Assert() on corruption, and when I took such code, I =
sometimes also took the comments in unmodified form.  That means that my =
normal commenting rules don't apply, as I'm not the comment author in =
such cases.

=E2=80=94
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company