On Fri, Mar 11, 2016 at 3:50 PM, Jim Nasby <jim.na...@bluetreble.com> wrote: > I also agree that the nmodule name isn't very clear. If this is meant to be > the start of a generic consistency checker, lets call it that. Otherwise, it > should be marked as being specific to btrees, because presumably we might > eventually want similar tools for GIN, etc. (FWIW I'd vote for a general > consistency checker).
It's a generic consistency checker -- that's the intent. Robert wanted to go that way about a year ago, and I think it makes sense (this module started out life as btreecheck). As I said, I don't really care what the name ends up being. amcheck seems fine to me, but I'll go with any reasonable suggestion that reflects the scope of the tool as a generic consistency checker for AMs, including heapam. I don't know that I understand the concern about the naming of bt_index_parent_check(). I called it something that accurately reflects what it does. That particular SQL-callable function is more orientated towards helping hackers than towards helping users detect routine problems, as the docs say, but it could help users working with hackers, and it might even help users acting on their own. I don't want to make it sound scary, because it isn't. I don't know what problems will be detected by this tool most often, and I don't think it would be wise to try to predict how that will work out. And so, concealing the internal workings of each function/check by giving them all totally non-technical names seems like a bad idea. It's not that hard to understand that a B-Tree has multiple levels, and checking the levels against each other requires more restrictive/stronger heavyweight locks. I've only added 2 SQL-callable functions, each of which has one argument, and the first of which has a generic name and very light locking requirements, like a SELECT. It's not that hard to get the general idea, as an ordinary user. > I know the vacuum race condition would be very rare, but I don't think it > can be ignored. Last thing you want out of a consistency checker is false > negatives/positives. That's what I said. And the docs also say that there should never be a false positive. That's definitely an important design goal here, because it enables routine testing. > I do think it would be reasonable to just wholesale > block against concurrent vacuums, but I don't think there's any reasonable > way to do that. Actually, that's exactly what bt_index_parent_check() does already. VACUUM requires a SHARE UPDATE EXCLUSIVE lock on the heap relation, which cannot be concurrently acquired due to bt_index_parent_check()'s acquisition of a SHARE lock. The locking for bt_index_parent_check() is almost the same as REINDEX, except that that acquires an ACCESS EXCLUSIVE lock on the index relation; bt_index_parent_check() only requires an EXCLUSIVE lock on the index relation. Not sure about the cost delay thing. Delays are disabled by default for manually issued VACUUM, so have doubts that that's useful. If you want the tool to limp on when it finds an error, that can be done by changing the constant for the CORRUPTION macro in amcheck.c. But having that be dynamically configurable is not really compatible with the goal of having amcheck be run routinely. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers