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

On Mon, Nov 13, 2017 at 8:48 PM, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> One thing that really bothers me about commit e2c79e14 is that
> LockPage() is called, not LockBuffer(). GIN had no LockPage() calls
> before that commit, and is now the only code in the entire system that
> calls LockPage()/ConditionalLockPage() (the hash am no longer uses
> page heavyweight locks following recent work there).

I would like to get rid of that LockPage() call, for sure, because
it's problematic in terms of allowing writes in parallel mode.
However, I think the reason here is the same as why the hash AM used
to use them. If you use a buffer lock, you really can't -- or
shoudn't, at least -- hold it across a whole series of operations,
because anyone waiting for that lock is waiting *uninterruptibly*.
The hash AM wanted to iterate through all of the pages in a bucket
chain and do something to each one while preventing concurrent scans;
the need here is similar. Aside from the uninterruptible-wait
problem, such coding patterns are extremely prone to deadlock. If
there's any chance that a process waiting for the buffer lock you hold
might be holding a buffer lock you try to acquire, you have got a
problem.

I don't view the use of LockPage() here as wrong or scary. I find
that it's impeding some of my own development goals, but that's not to
say it wasn't a good choice for the problem that they were trying to
solve. Had they solved it some other way, parallel writes might be
easier, but you can't win 'em all.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company