> On Mar 23, 2026, at 08:42, Chao Li <[email protected]> wrote: > > > >> 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. >
Sorry, forgot to attach the patch file. Here comes it. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
v2-0001-amcheck-fix-missing-corruption-error-in-allequali.patch
Description: Binary data
