Re: amcheck verification for GiST

On Sun, Feb 17, 2019 at 12:55 AM Andrey Borodin <x4mmm(at)yandex-team(dot)ru> wrote:
> That's true, we cannot avoid locking parent and child page simultaneously to check correctness of tuples.

Right.

Some further questions/comments:

* I think that this should be an error:

> + if (GistPageIsLeaf(page))
> + {
> + /* should never happen unless it is root */
> + Assert(stack->blkno == GIST_ROOT_BLKNO);
> + }

I use assertions in the verify_nbtree.c, but only for things that are
programming errors, and state that amcheck "owns". If they fail, then
it's a bug in amcheck specifically (they're really obvious assertions
about local state). Whereas this case could in theory happen with a
corrupt index, for example due to a page written in the wrong place.
I'm sure that the root block looks very similar to a leaf, so we won't
detect this any other way.

It's good to be paranoid, and to even think adversarially, provided it
doesn't make the design more difficult and has low runtime overhead.
We could debate whether or not this corruption is realistic, but it's
easy to make it an error and the cost is low, so you should just do
it.

* Couldn't leaf-like root pages also get some of the testing that we
do for other pages within check_index_tuple()? Ideally it wouldn't be
too much of a special case, though.

* I think that you need to add an
errcode(ERRCODE_FEATURE_NOT_SUPPORTED) to this:

> + /*
> + * Check that it's not a leftover invalid tuple from pre-9.1
> + * See also gistdoinsert() and gistbulkdelete() handlding of such tuples.
> + * We do not consider it error here, but warn operator.
> + */
> + if (GistTupleIsInvalid(idxtuple))
> + ereport(ERROR,
> + (errmsg("index \"%s\" contains an inner tuple marked as invalid",
> + RelationGetRelationName(rel)),
> + errdetail("This is caused by an incomplete page split at crash recovery before upgrading to PostgreSQL 9.1."),
> + errhint("Please REINDEX it.")));

> Currently, we do not check index tuples against heap. Should we do this or leave this for another patch?

To address this question: I would leave this for now. It can probably
work based on the same principle as nbtree's heapallindexed
verification, with few or no changes, but I don't think that we need
to make that happen in this release.

* You still hold multiple buffer locks at once, starting with the
parent and moving to the child. Only 2 buffer locks. Why is this
necessary, given that you now hold a ShareLock on both heap and index
relations? Couldn't you just copy the parent into local memory, in the
style of verify_nbtree.c?

* Note that this "parent then child" lock order seems to not be
consistent with the general rule for holding concurrent buffer locks
that is described in the GiST README:

"""
Concurrency control
-------------------
As a rule of thumb, if you need to hold a lock on multiple pages at the
same time, the locks should be acquired in the following order: child page
before parent, and left-to-right at the same level. Always acquiring the
locks in the same order avoids deadlocks.
"""

* The main point of entry for GiST verification is
gist_check_parent_keys_consistency(). It would be nice to have
comments that describe what it does, and in what order. I understand
that English is not your first language, which makes it harder, but I
would appreciate it if you made the effort to explain the theory. I
don't want to assume that I understand your intent -- I could be
wrong.

* Suggest that you use function prototypes consistently, even for
static functions. That is the style that we prefer. Also, please make
the comments consistent with project guidelines on indentation and
style.

* It would be nice to know if the code in
gist_check_parent_keys_consistency() finds problems in the parent, or
in the child. The nbtree check has an idea of a "target" page: every
page gets to be the target exactly once, and we only ever find
problems in the current target. Maybe that is arbitrary in some cases,
because the relationship between the two is where the problem actually
is. I still think that it is a good idea to make it work that way if
possible. It makes it easier to describe complicated relationships in
comments.

* Why does it make sense to use range_gist_picksplit()/operator class
"union" support function to verify parent/child relationships? Does it
handle NULL values correctly, given the special rules?

* Why not use the "consistent" support function instead of the "union"
support function? Could you write new code that was based on
gistindex_keytest() to do this?

* The GiST docs ("63.3. Extensibility") say of the "distance" support
function: "For an internal tree node, the distance returned must not
be greater than the distance to any of the child nodes". Is there an
opportunity to test this relationship, too, by making sure that
distance is sane? Perhaps this is a very naive question -- I am not a
GiST expert.

* Is it right that gist_check_internal_page() should both return a
value that says "this internal page has child pages that are
themselves internal", while testing the child pages?

* Do we need to worry about F_DELETED pages? Why or why not?

* Do we need to worry about F_HAS_GARBAGE pages? Why or why not?

--
Peter Geoghegan