Re: amcheck verification for GiST

Alvaro, Peter, thanks for working on this!

> 7 сент. 2019 г., в 6:26, Peter Geoghegan <pg(at)bowt(dot)ie> написал(а):
>
> On Fri, Sep 6, 2019 at 3:22 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
>> I'll take care of it, then.
>
> Attached is v10, which has some comment and style fix-ups, including
> the stuff Alvaro mentioned. It also adds line pointer sanitization to
> match what I added to verify_nbtree.c in commit a9ce839a (we use a
> custom PageGetItemIdCareful() for GiST instead of a simple
> PageGetItemId()). I also added a new file/TU for the routines that are
> now common to both nbtree and GiST verification, which I named
> amcheck.c. (I'm not sure about that, but I don't like verify_nbtree.c
> having generic/common functions.)
Maybe we should PageGetItemIdCareful() to amcheck.c too?
I think it can be reused later by GIN entry tree and to some extent by SP-GiST.
SP-GiST uses redirect tuples, but do not store this information in line pointer.

> I have only had a few hours to polish this, which doesn't seem like
> enough, though was enough to fix the noticeable stuff.
Cool, thanks!

> My main concern now is the heavyweight lock strength needed by the new
> function. I don't feel particularly qualified to sign off on the
> concurrency aspects of the patch. Heikki's v6 used a ShareLock, like
> bt_index_parent_check(), but you went back to an AccessShareLock,
> Andrey. Why is this safe? I see that you do gist_refind_parent() in
> your v9 a little differently to Heikki in his v6, which you seemed to
> suggest made this safe in your e-mail on March 28, but I don't
> understand that at all.

Heikki's version was reading childblkno instead of parentblkno, thus never refinding parent tuple.
Also, I've added check that internal page did not become leaf. In this case we would be comparing heap tids with index tids, and could possibly find incorrect match.
But, in current GiST implementation, inner page should never become leaf. We do not delete inner pages.
I think we could even convert it into sanity check, but I'm afraid that once upon a time we will implement delete for internal pages and forget to remove this check.

Current lock mode is AccessShareLock.
Before we find some discrepancy in tuples we follow standard locking protocol of GiST Index Scan.

When we suspect key consistency violation, we hold lock on page with some tuple. Then we take pin and lock on page that was parent for current some time before.
For example of validity see gistfinishsplit(). Comments state "On entry, the caller must hold a lock on stack->buffer", line 1330 acquires LockBuffer(stack->parent->buffer, GIST_EXCLUSIVE);
This function is used during inserts, but we are not going to modify data and place row locks, thus neither ROW EXCLUSIVE, not ROW SHARE is necessary.

PFA v11. There is one small comment in GistScanItem. Also, I've moved PageGetItemIdCareful() to amcheck.c. I'm not sure that this refactoring is beneficial for patch, but it reduces code size.

Thanks!

Best regards, Andrey Borodin.