Thread

Re: amcheck: support for GiST

Kirill Reshke <reshkekirill@gmail.com> — 2025-12-18T11:40:14Z

On Tue, 16 Dec 2025 at 20:24, Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> Hello,
>
> On Wed, Oct 22, 2025 at 11:58 AM Kirill Reshke <reshkekirill@gmail.com> 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              ~{:-)
> pj@illuminatedcomputing.com
>
>

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