Re: INSERT ... ON CONFLICT IGNORE (and UPDATE) 3.0

On Sat, Mar 28, 2015 at 6:36 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Just had a longer chat with Peter about this patch.

It was a very useful chat. Thanks for making yourself available to do it.

> * Not a fan of the heap flags usage, the reusage seems sketch to me
> * Explain should show the arbiter index in text as well
> * AddUniqueSpeculative is a bad name, it should refer IndexInfo
> * Work on the ExecInsert() comments
> * Let's remove the planner choosing the "cheapest" arbiter index; it
> should do all.
> * s/infer_unique_index/infer_arbiter_index/

OK.

> * Not supporting inheritance properly makes me uncomfortable. I don't
> think users will think that's a acceptable/reasonable restriction.

I'll look into making the inference specification deduce a child relation index.

> * Let's not use t_ctid directly, but add a wrapper

We talked about a union. This seems quite doable.

> * The code uses LockTupleExclusive to lock rows. That means the fkey key
> locking doesn't work; That's annoying. This means that using upsert
> will potentially cause deadlocks in other sessions :(. I think you'll
> have to determine what lock to acquire by fetching the tuple, doing
> the key comparison, and acquire the appropriate lock. That should be
> fine.

Looking into the implementation of this. As we discussed, the
difficulty about avoiding a lock escalation within ExecUpdate() is
that we must fetch the row, run EvalPlanQual() to check if the new row
version generated by updating will require a LockTupleExclusive or
LockTupleNoKeyExclusive, and then lock the row using the right
lockmode, and only then call ExecUpdate(). Right now, UPSERT benefits
from fetching and locking the row together, so going this way imposes
a little additional complexity. It's probably worth it, though.

> * I think we should decouple the insertion and wal logging more. I think
> the promise tuple insertion should be different from the final
> insertion of the actual tuple. For one it seems cleaner to me, for
> another it will avoid the uglyness around logical decoding. I think
> also that the separation will make it more realistic to use something
> like this for a COPY variant that doesn't raise unique violations and
> such.

Your COPY argument swung this for me. I'm looking into the implementation.

> * We discussed the infererence and that it should just reuse (code,
> grammar, docs) the column specification from create index.

Agreed.

--
Peter Geoghegan