Re: [PATCHES] GIN improvements

> * shiftList() holds an exclusive lock on metapage throughout its run,
> which means that it's impossible for two of them to run concurrently.
> So why bother with "concurrent deletion" detection?
Because metapage is locked immediately before shiftList call, while metapage is
unlocked another process could produce locking metapage and execution of
shiftList. So, when shiftList starts it should check of already deleted page. If
shiftList sees already deleted page then it doesn't do anything and reports to
the caller.

> * shiftList does LockBufferForCleanup, which means that it can be blocked
> for an indefinitely long time by a concurrent scan, and since it's holding
> exclusive lock on metapage no new scans or insertions can start meanwhile.
> This is not only horrid from a performance standpoint but it very probably
> can result in deadlocks --- which will be deadlocks on LWLocks and thus
> not even detected by the system.
Ops, I see possible scenario: UPDATE tbl SET gin_indexed_field = ... where
gin_indexed_field .... with concurrent shiftList. Will fix. Thank you.

Nevertheless, shiftList should be fast in typical scenario: it doesn't do
complicated work but just marks as deleted pages which already was readed before.

> * GIN index scans release lock and pin on one pending-list page before
> acquiring pin and lock on the next, which means there's a race condition:
> shiftList could visit and delete the next page before we get to it,
> because there's a window where we're holding no buffer lock at all.
Agree, will fix.

> * It seems also possible that once a list page has been marked
> GIN_DELETED, it could be re-used for some other purpose before a
> scan-in-flight reaches it -- reused either as a regular index page or as a
Impossible - because deletion is running from the head of list and scan too. But
deletion locks metapage and locks pages for cleanup. So, scan may start only
from not yet deleted page and will go through the list before deletion process.

> * There is a bigger race condition, which is that after a scan has
> returned a tuple from a pending page, vacuum could move the index entry
> into the main index structure, and then that same scan could return that
> same index entry a second time. This is a no-no, and I don't see any easy
> fix.
Hmm, isn't it allowed for indexes? At least GiST has this behaviour from its
birth date.

--
Teodor Sigaev E-mail: teodor(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/