Hi, I've looked at this patch today, mostly to educate myself, so this probably should not count as a full review. Anyway, the patch seems in excellent shape - it'd be great if all patches (including those written by me) had this level of comments/docs.
On Mon, 2016-02-29 at 16:09 -0800, Peter Geoghegan wrote: > I was assigned an "action point" during the FOSDEM developer meeting: > "Post new version of btree consistency checker patch". I attach a new > WIP version of my consistency checker tool, amcheck. ... > To recap, the extension adds some SQL-callable functions that verify > certain invariant conditions hold within some particular B-Tree index. > These are the conditions that index scans rely on always being true. > The tool's scope may eventually cover other AMs, including heapam, but > nbtree seems like the best place to start. > > Note that no function currently checks that the index is consistent > with the heap, which would be very useful (that's probably how I'd > eventually target the heapam, actually). I'm afraid I don't understand what "target the heapam" means. Could you elaborate? > Invariants > ======== ... > Obviously, this tool is all about distrusting the structure of a > B-Tree index. That being the case, it's not always totally clear where > to draw the line. I think I have the balance about right, though. I agree. It'd be nice to have a tool that also checks for data corruption the a lower level (e.g. that varlena headers are not corrupted etc.) but that seems like a task for some other tool. > Interface > ======= > > There are only 2 SQL callable functions in the extension, which are > very similar: > > bt_index_check(index regclass) > > bt_index_parent_check(index regclass) > Can we come up with names that more clearly identify the difference between those two functions? I mean, _parent_ does not make it particularly obvious that the second function acquires exclusive lock and performs more thorough checks. This also applies to the name of the contrib module - it's not particularly obvious what "amcheck" unless you're familiar with the concept of access methods. Which is quite unlikely for regular users. Maybe something like "check_index" would be more illustrative? > Locking > ====== ... > > We do, on the other hand, have a detailed rationale for why it's okay > that we don't have an ExclusiveLock on the index relation for checks > that span the key space of more than one page by following right links > to compare items across sibling pages. This isn't the same thing as > having an explicit interlock like a pin -- our interlock is one > against *recycling* by vacuum, which is based on recentGlobalXmin. > This rationale requires expert review. Well, I wouldn't count myself as an expert here, but do we actually need such protection? I mean, we only do such checks when holding an exclusive lock on the parent relation, no? And even if we don't vacuum can only remove entries from the pages - why should that cause violations of any invariants? A few minor comments: 1) This should probably say "one block per level": /* * A B-Tree cannot possibly have this many levels, since there must be * one block per page, and that is bound by the range of BlockNumber: */ 2) comment before bt_check_every_level() says: ... assumed to prevent any kind of physically modification ... probably should be "physical modification" instead. 3) s/targecontext/targetcontext/ in BtreeCheckState comment regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers