On Thu, Jul 16, 2020 at 10:24 AM Anastasia Lubennikova
<a.lubennikova@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