Re: INSERT ... ON CONFLICT {UPDATE | IGNORE}

On Thu, Sep 25, 2014 at 1:48 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> A. UPDATE/INSERT privilege infrastructure.
> Add tests to it, make it separately committable, so we can get that done.
> Submit to Oct CF; get that done early.

Makes sense. As long as we assume that we want a unified syntax like
this - that is, that we need something vaguely insert-update or
update-insertish - then we need this. Unfortunately, we cannot add
regression tests for this without almost the full patch set.

> B. Agree command semantics by producing these things
> * Explanatory documentation (Ch6.4 Data Manipulation - Upsert)
> * SQL Reference Documentation (INSERT)
> * Test cases for feature
> * Test cases for concurrency
> * Test cases for pgbench

Okay. I do have stress-tests, that are separately maintained, in case
you missed that:

https://github.com/petergeoghegan/upsert

> All of the above, as a separate committable patch. I hate the fact
> that you have written no user facing documentation for this feature.
> How can anyone tell whether the tests you've written are correct or
> even consistent to a particular definition of correctness?

I'd hoped that the commit messages, and my discussion of the feature
were adequate.

> Submit as patch for review only to Oct 15 CF
> We then agree what is required for further work
> At this stage, poll the Django and Rails communities for acceptance
> and early warning of these features. Listen.

I know an original founder of the Django project quite well - Jacob
Kaplan-Moss (a co-worker - the guy that keynoted pgOpen in its second
year). He is very interested in this effort.

> 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.

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.

Sorry, I forgot to not talk about locking.

> But we're still discussing SQL semantics. So first things first, then
> loop back around, hoping our design has not been concurrently
> deleted...

I hope the discussion can avoid "unprincipled deadlocks"....

-- 
Peter Geoghegan