Re: amcheck: support for GiST

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