Re: [HACKERS] ginInsertCleanup called from vacuum could still miss tuples to be deleted

On Mon, Nov 13, 2017 at 6:56 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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