Обсуждение: Re: amcheck: support for GiST

On Sat, 10 Jan 2026 at 19:56, Kirill Reshke <reshkekirill@gmail.com> wrote:
> On Thu, 1 Jan 2026 at 17:05, Kirill Reshke <reshkekirill@gmail.com> wrote:
>>
>> CF bot was unhappy about the last version due to obvious bug, PFA new
>> version with fixes.
>>
>> The problem was "DROP TABLE toast_bug;" missing in expected regression output.
>>
>>
>> [0] https://cirrus-ci.com/task/6378051304423424
>>
>>
>> --
>> Best regards,
>> Kirill Reshke
>
>
> Attached new version with commit message polishing, and address CF
> feedback, which was unhappy due to headercheck
>

After a quick preliminary review, here are some comments.

v2026-01-10-0001
================

1.
I'm pretty sure access/heaptoast.h is not needed by verify_nbtree.c.


v2026-01-10-0002
================

1.
+    if (GistPageGetOpaque(page)->gist_page_id != GIST_PAGE_ID)
+        ereport(ERROR,
+                (errcode(ERRCODE_INDEX_CORRUPTED),
+                 errmsg("index \"%s\" has corrupted page %d",
+                        RelationGetRelationName(rel), blockNo)));
+
+    if (GistPageIsDeleted(page))
+    {
+        if (!GistPageIsLeaf(page))
+            ereport(ERROR,
+                    (errcode(ERRCODE_INDEX_CORRUPTED),
+                     errmsg("index \"%s\" has deleted internal page %d",
+                            RelationGetRelationName(rel), blockNo)));
+        if (PageGetMaxOffsetNumber(page) > InvalidOffsetNumber)
+            ereport(ERROR,
+                    (errcode(ERRCODE_INDEX_CORRUPTED),
+                     errmsg("index \"%s\" has deleted page %d with tuples",
+                            RelationGetRelationName(rel), blockNo)));

blockNo is unsigned integer, so we should use %u in the format string.

2.
+        ereport(ERROR,
+                (errcode(ERRCODE_INDEX_CORRUPTED),
+                 errmsg("index \"%s\" internal page %d became leaf",
+                        RelationGetRelationName(rel), parentblkno)));

The same goes for parentblkno — it should also use %u.

> --
> Best regards,
> Kirill Reshke
>
> [2. text/x-diff; v2026-01-10-0001-Move-normalize-tuple-logic-from-nbtcheck.patch]...
>
> [3. text/x-diff; v2026-01-10-0002-Add-gist_index_check-function-to-verify-.patch]...

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

Hi!
Thanks for your review!

We discussed offline about the patch, and I decided to make a review of it and fixes
up to your comments.


>
> v2026-01-10-0001
> ================
>
> 1.
> I'm pretty sure access/heaptoast.h is not needed by verify_nbtree.c.
>

In fact this include is necessary because of line verify_common.c:236 which
uses TOAST_INDEX_TARGET


>
> v2026-01-10-0002
> ================
>
> blockNo is unsigned integer, so we should use %u in the format string.
>
> The same goes for parentblkno — it should also use %u.
>

Yes seems like there was misusage of format symbol.. I attached the v2026-01-12 with
fixed symbol. Thanks!

--
Best regards,
Roman Khapov

Hi, Roman Khapov

On Mon, 12 Jan 2026 at 01:36, Roman Khapov <rkhapov@yandex-team.ru> wrote:
> Hi!
> Thanks for your review!
>
> We discussed offline about the patch, and I decided to make a review of it and fixes
> up to your comments.
>

Thank you for updating the patches.

>> 
>> v2026-01-10-0001
>> ================
>> 
>> 1.
>> I'm pretty sure access/heaptoast.h is not needed by verify_nbtree.c.
>> 
>
> In fact this include is necessary because of line verify_common.c:236 which
> uses TOAST_INDEX_TARGET
>

Yeah, verify_common.c does require the header, but what I meant was that
verify_nbtree.c no longer needs it.

diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index c1e24338361..426e23d2960 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -23,7 +23,6 @@
  */
 #include "postgres.h"
 
-#include "access/heaptoast.h"
 #include "access/htup_details.h"
 #include "access/nbtree.h"
 #include "access/table.h"


-- 
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

> 
> Yeah, verify_common.c does require the header, but what I meant was that
> verify_nbtree.c no longer needs it.

Oh, thanks, now I see..

Updated the patches.

--
Best regards,
Roman Khapov