From: | Peter Geoghegan <pg(at)heroku(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
Subject: | Re: Refactoring speculative insertion with unique indexes a little |
Date: | 2016-03-16 23:43:06 |
Message-ID: | CAM3SWZTo6KRG0o7QmgXrtd_rc0PqU_KhSm27Rm=QmPMLd6QOug@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Mar 16, 2016 at 11:25 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Sure, and if everybody does that, then there will be 40 patches that
> get updated in the last 2 days if the CommitFest, and that will be
> impossible. Come on. You're demanding a degree of preferential
> treatment which is unsupportable.
It's unexpected that an entirely maintenance-orientated patch like
this would be received this way. I'm not demanding anything, or
applying any real pressure. Let's just get on with it.
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.
I do not pursue something like this without good reason. I'm
optimistic that the patch will be accepted if it is carefully
considered.
--
Peter Geoghegan
Attachment | Content-Type | Size |
---|---|---|
0001-Refactor-speculative-insertion-into-unique-indexes.patch | text/x-patch | 17.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | David Steele | 2016-03-16 23:44:24 | Re: proposal: make NOTIFY list de-duplication optional |
Previous Message | David Steele | 2016-03-16 23:39:22 | Re: Speedup twophase transactions |