Re: amcheck: fix bug of missing corruption in allequalimage validation

From: Kirill Reshke <reshkekirill(at)gmail(dot)com>
To: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: amcheck: fix bug of missing corruption in allequalimage validation
Date: 2026-02-25 06:43:49
Message-ID: CALdSSPj_ZDnr_V7whM46QBO3aQWt+jka-5BUA+kW_MnGf1=k7A@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, 25 Feb 2026 at 08:12, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
> Hi,
>
> While poking around the code in contrib/amcheck/verify_nbtree.c, I noticed the following block:
> ```
> if (allequalimage && !_bt_allequalimage(indrel, false))
> {
> bool has_interval_ops = false;
>
> for (int i = 0; i < IndexRelationGetNumberOfKeyAttributes(indrel); i++)
> if (indrel->rd_opfamily[i] == INTERVAL_BTREE_FAM_OID)
> {
> has_interval_ops = true;
> ereport(ERROR,
> (errcode(ERRCODE_INDEX_CORRUPTED),
> errmsg("index \"%s\" metapage incorrectly indicates that deduplication is safe",
> RelationGetRelationName(indrel)),
> has_interval_ops
> ? errhint("This is known of \"interval\" indexes last built on a version predating 2023-11.")
> : 0));
> }
> }
> ```
>
> My initial impression was that has_interval_ops was unneeded and could be removed, as it is always true at the point of use. I originally thought this would just be a tiny refactoring.
>
> However, on second thought, I realized that having the ereport inside the for loop is actually a bug. If allequalimage is set in the metapage but _bt_allequalimage says it’s unsafe, we should report corruption regardless of the column types. In the current code, if the index does not contain an interval opfamily, the loop finishes without reaching the ereport, thus silencing the corruption.
>
> This patch moves the ereport out of the for loop. This ensures that corruption is reported unconditionally, while keeping the interval-specific hint optional.
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>

uff, this looks like a clear oversight of d70b176.

> commit d70b17636ddf1ea2c71d1c7bc477372b36ccb66b
> Author: Tomas Vondra <tomas(dot)vondra(at)postgresql(dot)org>
> Date: Sat Mar 29 15:14:47 2025 +0100

> amcheck: Move common routines into a separate module
...
> Reviewed-By: Kirill Reshke <reshkekirill(at)gmail(dot)com>
> Discussion: https://postgr.es/m/45AC9B0A-2B45-40EE-B08F-BDCF5739D1E1%40yandex-team.ru

Oops.

Before d70b176 it was like this:

https://github.com/postgres/postgres/blame/fb9dff76635d4c32198f30a3cb503588d557d156/contrib/amcheck/verify_nbtree.c#L386-L399

--
Best regards,
Kirill Reshke

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Chao Li 2026-02-25 06:52:35 Re: Checkpointer write combining
Previous Message Ashutosh Bapat 2026-02-25 06:40:55 Re: SQL Property Graph Queries (SQL/PGQ)