Re: Refactoring speculative insertion with unique indexes a little
От | Heikki Linnakangas |
---|---|
Тема | Re: Refactoring speculative insertion with unique indexes a little |
Дата | |
Msg-id | bc7d9674-9852-3e35-5122-cff48b363eb0@iki.fi обсуждение исходный текст |
Ответ на | Re: Refactoring speculative insertion with unique indexes a little (Peter Geoghegan <pg@heroku.com>) |
Ответы |
Re: Refactoring speculative insertion with unique indexes a little
(Peter Geoghegan <pg@heroku.com>)
|
Список | pgsql-hackers |
On 03/17/2016 01:43 AM, Peter Geoghegan wrote: > I attach a revision, that makes all the changes that Heikki suggested, > except one. As already noted several times, following this suggestion > would have added a bug. Alvaro preferred my original approach here in > any case. I refer to my original approach of making the new > UNIQUE_CHECK_SPECULATIVE case minimally different from the existing > UNIQUE_CHECK_PARTIAL case currently used for deferred unique > constraints and speculative insertion, as opposed to making the new > UNIQUE_CHECK_SPECULATIVE "like CHECK_UNIQUE_YES, but return FALSE > instead of throwing an error on conflict". That was broken because > CHECK_UNIQUE_YES waits for the outcome of an xact, which > UNIQUE_CHECK_PARTIAL never does, and so UNIQUE_CHECK_SPECULATIVE must > never do. > > Any and all waits happen in the first phase of speculative insertion, > and never the seconds. I could give a complicated explanation for why, > involving a deadlock scenario, but a simple explanation will do: it > has always worked that way, and was tested to work that way. > > Feedback from Heikki led to these changes for this revision: > > * The use of arguments within ExecInsert() was simplified. > > * More concise AM documentation. > > The docs essentially describe two new concepts: > > - What unique index insertion needs to know about speculative > insertion in general. This doesn't just apply to speculative inserters > themselves, of course. > > - What speculative insertion is. Why it exists (why we don't just wait > on xact). In other words, what "unprincipled deadlocks" are, and how > they are avoided (from a relatively high level). > > I feel like I have a responsibility to make sure that this mechanism > is well documented, especially given that not that many people were > involved in its design. It's possible that no more than the 3 original > authors of UPSERT fully understand speculative insertion -- it's easy > to miss some of the subtleties. Thanks for working on this, and sorry for disappearing and dropping this on the floor earlier. This doesn't apply anymore, thanks to 9c810a2e. Shouldn't be hard to fix. I was in support of this earlier in general, even though I had some questions on the details. But now that I look at the patch again, I'm not so sure anymore. I don't think this actually makes things clearer. It adds new cases that the code needs to deal with. Index AM writers now have to care about the difference between a UNIQUE_CHECK_PARTIAL and UNIQUE_CHECK_SPECULATIVE. You can have the exact same implementation for both, but at the very least the index AM author needs to read the paragraph you added to the docs to understand the difference. That adds some cognitive load. Likewise, in ExecInsertIndexTuples(), this makes the deferred-index case work slightly differently from speculative insertion. It's not a big difference, but it again forces you to keep one more scenario in mind, when reading the code. So overall, I think we should just drop this. Maybe a comment somewhere would be in order, to point out that ExecInsertIndexTuples() and index_insert might perform some unnecessary work, by inserting index tuples for a doomed heap tuple, if a speculative insertion fails. But that's all. - Heikki
В списке pgsql-hackers по дате отправления: