Re: Amcheck verification of GiST and GIN

On Tue, 26 Nov 2024 at 11:50, Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> ====
> Overall I think 0001 & 0002 are ready as-is. 0003 is maybe ok.  Other
> patches need more review rounds.
>
> --
> Best regards,
> Kirill Reshke

=== Patch changes.

Polishing:
1)

> + /* last tuple in layer has no high key */
> + if (i != maxoff && !GinPageGetOpaque(page)->rightlink)
> + {
> + ptr->parenttup = CopyIndexTuple(idxtuple);
> + }
> + else
> + {
> + ptr->parenttup = NULL;
> + }

This coding does not align with PostgreSQL style. I removed
parentheses here and in other few places :

```
reshke@ygp-jammy:~/postgres$ git diff contrib/amcheck/verify_gin.c
diff --git a/contrib/amcheck/verify_gin.c b/contrib/amcheck/verify_gin.c
index 47b6e81fbc4..be44cc724f8 100644
--- a/contrib/amcheck/verify_gin.c
+++ b/contrib/amcheck/verify_gin.c
@@ -111,9 +111,7 @@ ginReadTupleWithoutState(IndexTuple itup, int *nitems)
                                         nipd, ndecoded);
                }
                else
-               {
                        ipd = palloc(0);
-               }
        }
        else
        {
@@ -194,7 +192,6 @@
gin_check_posting_tree_parent_keys_consistency(Relation rel,
BlockNumber posting
                        list = GinDataLeafPageGetItems(page, &nlist, minItem);

                        if (nlist > 0)
-                       {
                                snprintf(tidrange_buf, sizeof(tidrange_buf),
                                                 "%d tids (%u, %u) - (%u, %u)",
                                                 nlist,
@@ -202,11 +199,8 @@
gin_check_posting_tree_parent_keys_consistency(Relation rel,
BlockNumber posting

ItemPointerGetOffsetNumberNoCheck(&list[0]),

ItemPointerGetBlockNumberNoCheck(&list[nlist - 1]),

ItemPointerGetOffsetNumberNoCheck(&list[nlist - 1]));
-                       }
                        else
-                       {
                                snprintf(tidrange_buf,
sizeof(tidrange_buf), "0 tids");
-                       }

                        if (stack->parentblk != InvalidBlockNumber)
                        {
@@ -218,11 +212,9 @@
gin_check_posting_tree_parent_keys_consistency(Relation rel,
BlockNumber posting
                                         tidrange_buf);
                        }
                        else
-                       {
                                elog(DEBUG3, "blk %u: root leaf, %s",
                                         stack->blkno,
                                         tidrange_buf);
-                       }

                        if (stack->parentblk != InvalidBlockNumber &&

ItemPointerGetOffsetNumberNoCheck(&stack->parentkey) !=
InvalidOffsetNumber &&
@@ -576,13 +568,9 @@ gin_check_parent_keys_consistency(Relation rel,
                                ptr->depth = stack->depth + 1;
                                /* last tuple in layer has no high key */
                                if (i != maxoff &&
!GinPageGetOpaque(page)->rightlink)
-                               {
                                        ptr->parenttup =
CopyIndexTuple(idxtuple);
-                               }
                                else
-                               {
                                        ptr->parenttup = NULL;
-                               }
                                ptr->parentblk = stack->blkno;
                                ptr->blkno = GinGetDownlink(idxtuple);
                                ptr->parentlsn = lsn;

```

2)

>+                                               else
>+                                               {
>+                                                       /*
>+                                                        * But now it is properly adjusted - nothing to do
>+                                                        * here.
>+                                                        */
>+                                               }

if (...) ... else {/* comment */} is a strange pattern.


PFA v6.
No other changes from v5 except for mandatory rebase of v5-0005 due to 18954ce.

=== CC changes
I changed Tomas's email to tomas@vondra.me, as @enterprisedb one no
longer exists.

-- 
Best regards,
Kirill Reshke