On Mon, Nov 13, 2017 at 6:56 PM, Masahiko Sawada <sawada.m...@gmail.com> wrote: > /* > * We would like to prevent concurrent cleanup process. For that we will > * lock metapage in exclusive mode using LockPage() call. Nobody other > * will use that lock for metapage, so we keep possibility of concurrent > * insertion into pending list > */ > > So I conjecture that this has been introduced for not the reason why > we need to detect deadlock but the reason why we need to a different > lock from the lock used by insertion into pending list.
I understood that much, but I think that we need to detect problems and recover from them (something like _bt_page_recyclable()), rather than preventing them with pessimistic locking -- or, at least, there is no reason I know to think that the HW lock is sufficient, and I am tempted to go that way to fix this. Commit e9568083, which added the feature that led to commit e2c79e14, may itself be the basic problem here. Another complication is that 218f5158 changed things here again. It's possible that the report about 10 [1] (not the undetected deadlock report) involved that commit, but either way we have big problems here. GIN doesn't have something like nbtree's RecentGlobalXmin/_bt_page_recyclable() interlock. I don't know that much about GIN (I would like to learn more), but I will try to break this down, for myself, and for others. We know: * There are some cases in which we simply don't ever delete pages (in the entry tree), so that's obviously why there is no need for RecentGlobalXmin interlock there. Easy. * According to the GIN README, the pending list cleanup by VACUUM has a super-exclusive lock on the root, to block out concurrent inserters (that hold a pin on the root throughout). That's why it was/is okay that VACUUM recycled pending list pages without a RecentGlobalXmin interlock. Not so easy, but still not hard. * Commit e9568083 and follow-ups let inserters free space from deleted pages. Why should inserters not need a super-exclusive lock, just like VACUUM? The README still says that they need one! This is the first time we ever called RecordFreeIndexPage() from anywhere that is not strictly VACUUM code. That feels wrong to me. The obvious solution is to not recycle in inserter's calls to ginInsertCleanup(), but I have no idea if that will work, in part because I don't understand GIN very well. > Yeah, I agree. I'll manage to get the time to look carefully at the > GIN code related to fast insertion and cleanup pending list. Cool. I'll try to spare some more time for this, too. [1] https://www.postgresql.org/message-id/A5C88C48-4573-453B-BF52-3B34A97D7A0A%40xmission.com -- Peter Geoghegan