On Mon, Nov 13, 2017 at 8:48 PM, Peter Geoghegan <p...@bowt.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
