Re: Amcheck verification of GiST and GIN - Mailing list pgsql-hackers

Hi Arseniy!

Thanks for finding these problems.
I had several attempts to wrap my head around original patch with fixes, but when it was broken into several steps it
finallybecame easier for me. 
Here are some thought about patches.



> On 10 Jun 2025, at 13:18, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> wrote:
> <0001-amcheck-Add-gin_index_check-on-a-multicolumn-index.patch>

The test seems harmless and nice to have. I understand that this test is needed to extend coverage.
Perhaps, we could verify that some code is actually triggered. Personally, I would be happy if we could some add
injectionpoints with notices at tested branches. But, AFAIK, it's too much of a burden to have injection points in
contribextensions. We had very similar problem with sort patch in btree_gist and eventually gave up. elog(DEBUG) was
nota good solution too, because it was unstable. 
See 'gin-finish-incomplete-split' or 'hash-aggregate-enter-spill-mode' for reference.


> On 10 Jun 2025, at 13:18, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> wrote:
> <0002-patch-1-gin_check_parent_keys_consistency.patch>

Well, we inherited ginCompareEntries() from the very first patch version from 2020. I can't really say anything about
differenceshere, but your proposed change seems correct. 

Kirill excluded rightmost keys in v33 and that was kind of a fix. Kirill, do you remember if was particular problem of
internalpages? Is it safe to enable tuple order check for rightmost tuples on leaf pages? 

You wrote this comment:
+            /*
+             * First block is metadata, skip order check. Also, never check
+             * for high key on rightmost page, as this key is not really
+             * stored explicitly.
+             */

I agree that exclusion (stack->blkno != GIN_ROOT_BLKNO) make no sense. It was with us from the original version from
2020.As I understand some checks on root page will be used in test invalid_entry_columns_order_test. 


Having some TAP tests sounds like a very good idea.

I'm a bit surprised by excluding some letters from random_string(), but perhaps it's fine for this test.

Somewhere here:
+        INSERT INTO $relname (a) VALUES (('{' || 'pppppppppp' || random_string(1870) ||'}')::text[]);
I'd like to have a comment explaining number 1870. And, probably, you expect exactly 2 tuples on root page, right?

Are we 100% certain that 'rrrrrrrrr' will always be on root page?

I do not see much value in having variables $relname and $indexname. I'd just substitute its usages with literals. But
I'mnot sure, maybe this structure will be used in your tests later... 

In this function
+sub string_replace_block
I'd suggest a little bit of comments. Also, perhaps, fsync of files, but 001_verify_heapam.pl does not do fsync. So,
maybeit's OK here too. 

Also, I have a wild idea. Maybe add an assert that block size if 8192 and just exit otherwise?

And, maybe instead of gin_clean_pending_list() you can just create an index with fastupdate=off.

> On 10 Jun 2025, at 13:18, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> wrote:
> <0003-patch-2-gin_check_parent_keys_consistency.patch>

The patch seems correct to me.
Except this
+    my $blkno = 5;  # leaf
in test reads scary. Will it be stable on buildfarm?


> On 10 Jun 2025, at 13:18, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> wrote:
> <0004-patch-3-gin_check_posting_tree_parent_keys_consisten.patch>

I generally agree with direction of this patch.
But please also check the approach of PageGetItemIdCareful() in verify_nbtree.c. It goes extra mile to avoid coredump
incase of bogus ItemId. Should we do something like that here too? 


> On 10 Jun 2025, at 13:18, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> wrote:
>
> <0005-patch-4-remove-unused-parentlsn.patch>

LGTM.


> On 9 May 2025, at 17:43, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> wrote:
>
> 10) README says "Vacuum never deletes tuples or pages from the entry
> tree." But check assumes that it's possible to have
> deleted leaf page with 0 entries.
>
>    if (GinPageIsDeleted(page))
>    {
>       if (!GinPageIsLeaf(page))
>          ereport(ERROR,
>                (errcode(ERRCODE_INDEX_CORRUPTED),
>                 errmsg("index \"%s\" has deleted internal page %u",
>                      RelationGetRelationName(rel), blockNo)));
>       if (PageGetMaxOffsetNumber(page) > InvalidOffsetNumber)
>          ereport(ERROR,
>                (errcode(ERRCODE_INDEX_CORRUPTED),
>                 errmsg("index \"%s\" has deleted page %u with tuples",
>                      RelationGetRelationName(rel), blockNo)));
>    }

To enforce such an invariant we must be sure that GIN never deleted entry pages in older versions. I do not have enough
knowledgeof the history for this. 

> 11) When we compare entry tree max page key with parent key:
>
>             if (ginCompareAttEntries(&state, attnum, current_key,
>                              current_key_category, parent_key_attnum,
>                                      parent_key, parent_key_category) > 0)
>             {
>                /*
>                 * There was a discrepancy between parent and child
>                 * tuples. We need to verify it is not a result of
>                 * concurrent call of gistplacetopage(). So, lock parent
>                 * and try to find downlink for current page. It may be
>                 * missing due to concurrent page split, this is OK.
>                 */
>                pfree(stack->parenttup);
>                stack->parenttup = gin_refind_parent(rel, stack->parentblk,
>                                            stack->blkno, strategy);
>
> I think we can remove gin_refind_parent() and do ereport right away here.
> The same logic as with 3). AFAIK it's impossible to have a child item
> with a key that is higher than the cached parent key.
> Parent key bounds what keys we can insert into the child page, so it
> seems there is no way how they can appear there.
>

This logic was copied from GiST check. In GiST "Area of responsibility" of internal tuple can be extended in any
direction.That's why we need to lock parent page. 
If in GIN internal tuple keyspace is never extended - it's OK to avoid gin_refind_parent().
But reasoning about GIN concurrency is rather difficult. Unfortunately, we do not have such checks in B-tree
verificationwithout ShareLock. Either way we could peep some idea from there. 

Thank you!


Best regards, Andrey Borodin.