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 finally became easier for me.
Here are some thought about patches.
> On 10 Jun 2025, at 13:18, Arseniy Mukhin <arseniy.mukhin....@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 injection points with notices at tested
branches. But, AFAIK, it's too much of a burden to have injection points in
contrib extensions. We had very similar problem with sort patch in btree_gist
and eventually gave up. elog(DEBUG) was not a 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....@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 differences here, 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 internal pages? 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'm not 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, maybe it'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....@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....@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 in case of bogus ItemId.
Should we do something like that here too?
> On 10 Jun 2025, at 13:18, Arseniy Mukhin <arseniy.mukhin....@gmail.com> wrote:
>
> <0005-patch-4-remove-unused-parentlsn.patch>
LGTM.
> On 9 May 2025, at 17:43, Arseniy Mukhin <arseniy.mukhin....@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 knowledge of 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 verification without ShareLock. Either way we
could peep some idea from there.
Thank you!
Best regards, Andrey Borodin.
