On 09/26/2014 12:13 AM, Peter Geoghegan wrote:
On Thu, Sep 25, 2014 at 1:48 PM, Simon Riggs <si...@2ndquadrant.com> wrote:
C. Internal weirdness
Submit C based upon earlier agreed B, submit to Dec 15 CF, major patch
deadline, so we can fine tune for last CF.
Then Heikki rewrites half your patch in a better way, you thank him
and then we commit. All done.
I don't have a problem with Heikki or anyone else rewriting the value
locking part of the patch, provided it meets my requirements for such
a mechanism. Since Heikki already agreed that that standard should be
imposed, he'd hardly take issue with it now.
However, the fact is that once you actually make something like
promise tuples meet that standard, at the very least it becomes a lot
messier than you'd think. Heikki's final prototype "super deleted"
tuples by setting their xmin to InvalidTransactionId. We weren't sure
that that doesn't break some random other heapam code. Consider this,
for example:
https://github.com/postgres/postgres/blob/REL9_4_STABLE/src/backend/executor/execMain.c#L1961
So that looks safe in the face of setting xmin to InvalidTransactionId
in the way the later prototype patch did if you think about it for a
while, but there are other places where that is less clear. In short,
it becomes something that we have to worry about for ever, because
"xmin cannot change without the tuple in the slot changing" is clearly
an invariant for certain purposes. It might accidentally fail to fail
right now, but I'm not comfortable with it.
Just to be clear: I wrote the initial patch to demonstrate what I had in
mind, because I was not able to explain it well enough otherwise. You
pointed out issues with it, which I then fixed. You then pointed out
more issues, which I then fixed again.
My patch version was a proof of concept, to demonstrate that it can be
done. What I'd like you to do now, as the patch author, is to take the
promise tuple approach and clean it up. If the xmin stuff is ugly,
figure out some other way to do it.
Now, I might be convinced that that's actually the way to go. I have
an open mind. But that will take discussion. I like that page
hwlocking is something that many systems do (even including Oracle, I
believe). Making big changes to nbtree is always something that
deserves to be met with skepticism, but it is nice to have an
implementation that lives in the head of AM.
I don't know what you mean by "in the head of AM", but IMO it would be
far better if we can implement this outside the index AMs. Then it will
work with any index AM.
BTW, in the discussions, you pointed out that exclusion constraints
currently behave differently from a unique index, when two backends
insert a tuple at the same time. With a unique index, one of them will
fail, but one is always guaranteed to succeed. With an exclusion
constraint, they can both fail if you're unlucky. I think the promise
tuples would allow us to fix that, too, while we're at it. In fact, you
might consider tackling that first, and build the new INSERT ON CONFLICT
syntax on top of that. Basically, an INSERT to a table with an exclusion
constraint would be the same as "INSERT ON CONFLICT throw an error".
That would be a useful way to split this patch into two parts.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
