Re: Amcheck verification of GiST and GIN

On Tue, 26 Nov 2024 at 11:50, Kirill Reshke <reshkekirill(at)gmail(dot)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(at)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(at)vondra(dot)me, as @enterprisedb one no
longer exists.

--
Best regards,
Kirill Reshke