> On Mar 4, 2026, at 08:49, Xuneng Zhou <[email protected]> wrote:
>
> Hi,
>
> On Thu, Feb 26, 2026 at 9:13 AM Chao Li <[email protected]> wrote:
>>
>>
>>
>>> On Feb 25, 2026, at 18:13, Kirill Reshke <[email protected]> wrote:
>>>
>>> On Wed, 25 Feb 2026 at 11:59, Chao Li <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>>> On Feb 25, 2026, at 14:43, Kirill Reshke <[email protected]> wrote:
>>>>>
>>>>> On Wed, 25 Feb 2026 at 08:12, Chao Li <[email protected]> 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.
>>>>>
>>>>> 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/
