Re: amcheck: support for GiST

From: Japin Li <japinli(at)hotmail(dot)com>
To: Kirill Reshke <reshkekirill(at)gmail(dot)com>
Cc: Paul A Jungwirth <pj(at)illuminatedcomputing(dot)com>, Andrey Borodin <x4mmm(at)yandex-team(dot)ru>, Miłosz Bieniek <bieniek(dot)milosz(at)proton(dot)me>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: amcheck: support for GiST
Date: 2026-01-10 16:20:19
Message-ID: MEAPR01MB30311C5AE2220142641DA0C2B683A@MEAPR01MB3031.ausprd01.prod.outlook.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Sat, 10 Jan 2026 at 19:56, Kirill Reshke <reshkekirill(at)gmail(dot)com> wrote:
> On Thu, 1 Jan 2026 at 17:05, Kirill Reshke <reshkekirill(at)gmail(dot)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.

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Marco Matteucci 2026-01-10 16:28:03 [RFC] SLIM Data Type - Compact JSON Alternative (17-62% smaller)
Previous Message Kirill Reshke 2026-01-10 14:56:28 Re: amcheck: support for GiST