On Thu, Jul 16, 2020 at 10:24 AM Anastasia Lubennikova <a.lubennik...@postgrespro.ru> wrote: > It's impressive that this check helped to find several bugs.
While it's definitely true that it *could* have detected the bug fixed by commit b0229f26, it's kind of debatable whether or not the bugs I fixed in commit fa7ff642 and commit 7154aa16 (which actually were found using this new instrumentation) were truly bugs. The behavior in question was probably safe, since only the special/opaque page area was accessed -- and with at least a buffer pin held. But it's not worth having a debate about whether or not it should be considered safe. There is no downside to not having a simple strict rule that's easy to enforce. Also, I myself spotted some bugs in the skip scan patch series at one point that would also be caught by the new instrumentation. > I only noticed small inconsistency in the new comment for > _bt_conditionallockbuf(). > > It says "Note: Caller is responsible for calling _bt_checkpage() on > success.", while in _bt_getbuf() the call is not followed by > _bt_checkpage(). > Moreover, _bt_page_recyclable() contradicts _bt_checkpage() checks. Nice catch. > Other than that, patches look good to me, so move them to "Ready For > Committer". Pushed the first patch just now, and intend to push the other one soon. Thanks! > Are you planning to add same checks for other access methods? Not at the moment, but it does seem like my approach could be generalized to other index access methods. -- Peter Geoghegan
