On Tue, 16 Dec 2025 at 20:24, Paul A Jungwirth <[email protected]> wrote: > > Hello, > > On Wed, Oct 22, 2025 at 11:58 AM Kirill Reshke <[email protected]> wrote: > > > > > 1) There are several typos in verify_gist.c: > > > > > > downlinks -> downlink (header comment) > > > discrepencies -> discrepancies > > > Correctess -> Correctness > > > hande -> handle > > > Initaliaze -> Initialize > > > numbmer -> number > > > replcaed -> replaced > > > aquire -> aqcuire > > > > > > 2) Copyright year is 2023 in the patch. Time flies:) > > > > These two are (trivially) fixed. > > I found a few more typos. Maybe one is left over from Arseniy's > review. Referencing the latest patch files from Andrey, here is what I > see: > > in v20251216-0002-Add-gist_index_check-function-to-verify-Gi.patch: > > > This function traverses GiST with a depth-fisrt search and checks > > "fisrt" should be "first". > > > This traverse takes lock of any page until some discapency found. > > "discapency" should be "discrepancy" > > > To re-check suspicious pair of parent and child tuples it aqcuires > > "aqcuires" should be "acquires" > > amcheck.sgml: > > + require tuple adjustement) and page graph respects balanced-tree > > "adjustement" should be "adjustment" > > Also the Makefile ordering is not quite right: > > --- a/contrib/amcheck/Makefile > +++ b/contrib/amcheck/Makefile > @@ -4,16 +4,17 @@ MODULE_big = amcheck > OBJS = \ > $(WIN32RES) \ > verify_common.o \ > + verify_gist.o \ > verify_gin.o \ > verify_heapam.o \ > verify_nbtree.o > > We should put verify_gist.o after verify_gin.o. > > Yours, > > -- > Paul ~{:-) > [email protected] > >
Hi!
Thank you for taking a look. Sending new version which is Andreys's
[0] + 0003 applied + your review comments addressed + my changes,
including:
Commit message:
This function traverses GiST with a depth-first search and checks
that all downlink tuples are included into parent tuple keyspace.
This traverse takes lock of any page until some discrepancy found.
To re-check suspicious pair of parent and child tuples it acquires
locks on both parent and child pages in the same order as page
split does.
" discrepancy found" -> " discrepancy is found"
" re-check suspicious " -> " re-check a suspicious "
I also added you, Arseniy and Miłosz in commit message, in reviewed-by section
+ /* Pointer to a next stack item. */
+ struct GistScanItem *next;
+} GistScanItem;
+
a next -> the next
+ /*
+ * It's possible that the page was split since we looked at the
+ * parent, so that we didn't missed the downlink of the right sibling
+ * when we scanned the parent. If so, add the right sibling to the
+ * stack now.
+ */
"didn't miss" not "didn't missed "
+ /*
+ * 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.
"find a downlink"
also this:
--- a/contrib/amcheck/verify_gist.c
+++ b/contrib/amcheck/verify_gist.c
@@ -583,7 +583,8 @@ gist_refind_parent(Relation rel,
{
Buffer parentbuf;
Page parentpage;
- OffsetNumber parent_maxoff;
+ OffsetNumber parent_maxoff,
+ off;
IndexTuple result = NULL;
parentbuf = ReadBufferExtended(rel, MAIN_FORKNUM, parentblkno,
RBM_NORMAL,
@@ -605,9 +606,9 @@ gist_refind_parent(Relation rel,
}
parent_maxoff = PageGetMaxOffsetNumber(parentpage);
- for (OffsetNumber o = FirstOffsetNumber; o <= parent_maxoff; o
= OffsetNumberNext(o))
+ for (off = FirstOffsetNumber; off <= parent_maxoff; off =
OffsetNumberNext(off))
{
- ItemId p_iid = PageGetItemIdCareful(rel,
parentblkno, parentpage, o);
+ ItemId p_iid = PageGetItemIdCareful(rel,
parentblkno, parentpage, off);
IndexTuple itup = (IndexTuple)
PageGetItem(parentpage, p_iid);
if (ItemPointerGetBlockNumber(&(itup->t_tid)) == childblkno)
--
Best regards,
Kirill Reshke
v20251218-0001-Move-normalize-tuple-logic-from-nbtcheck-t.patch
Description: Binary data
v20251218-4-0002-Add-gist_index_check-function-to-verify-.patch
Description: Binary data
