Re: amcheck: fix bug of missing corruption in allequalimage validation - Mailing list pgsql-hackers

> On Mar 4, 2026, at 08:49, Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> On Thu, Feb 26, 2026 at 9:13 AM Chao Li <li.evan.chao@gmail.com> wrote:
>>
>>
>>
>>> On Feb 25, 2026, at 18:13, Kirill Reshke <reshkekirill@gmail.com> wrote:
>>>
>>> On Wed, 25 Feb 2026 at 11:59, Chao Li <li.evan.chao@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>>> On Feb 25, 2026, at 14:43, Kirill Reshke <reshkekirill@gmail.com> wrote:
>>>>>
>>>>> On Wed, 25 Feb 2026 at 08:12, Chao Li <li.evan.chao@gmail.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
deduplicationis safe", 
>>>>>>                                                              RelationGetRelationName(indrel)),
>>>>>>                                               has_interval_ops
>>>>>>                                               ? errhint("This is known of \"interval\" indexes last built on a
versionpredating 2023-11.") 
>>>>>>                                               : 0));
>>>>>>                      }
>>>>>>      }
>>>>>> ```
>>>>>>
>>>>>> My initial impression was that has_interval_ops was unneeded and could be removed, as it is always true at the
pointof 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
allequalimageis set in the metapage but _bt_allequalimage says it’s unsafe, we should report corruption regardless of
thecolumn types. In the current code, if the index does not contain an interval opfamily, the loop finishes without
reachingthe ereport, thus silencing the corruption. 
>>>>>>
>>>>>> This patch moves the ereport out of the for loop. This ensures that corruption is reported unconditionally,
whilekeeping 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.
>>>>>
>>>>> Before d70b176 it was like this:
>>>>>
>>>>>
https://github.com/postgres/postgres/blame/fb9dff76635d4c32198f30a3cb503588d557d156/contrib/amcheck/verify_nbtree.c#L386-L399
>>>>>
>>>>>
>>>>
>>>> Thanks for pointing out the origin code that seems to prove my fix is correct. But my patch adds “break” in the
“for”loop, which makes it slightly better than the original version. 
>
> I think that the break is a small improvement.
>
> Once has_interval_ops becomes true, the loop has already learned
> everything it needs for the optional hint, so scanning the remaining
> key attributes does not change behavior. In that sense, your version
> is slightly better than the pre-d70b176 code.
>
> AFAICS, the bug is that the corruption report was incorrectly gated on
> finding INTERVAL_BTREE_FAM_OID; moving the ereport(ERROR) out of the
> loop would fix it.
>
> LGTM from my side.
>

Cool.

A gentle ping as this fixes a regression from d70b176.

PFA v2 - just rebase, nothing changed.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/