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

On Tue, Apr 21, 2015 at 7:57 AM, Andres Freund <andres@anarazel.de> wrote:
> I'm not 100% sure Heikki and I am on exactly the same page here :P
>
> I'm looking at git diff $(git merge-base upstream/master HEAD).. where
> HEAD is e1a5822d164db0.

Merged your stuff into my Github branch. Heikki is pushing changes
there directly now.

> * The logical stuff looks much saner.

Cool.

> * Please add tests for the logical decoding stuff. Probably both a plain
>   regression and and isolationtester test in
>   contrib/test_decoding. Including one that does spooling to disk.

Working on it...hope to push that to Github soon.

> * I don't like REORDER_BUFFER_CHANGE_INTERNAL_INSERT/DELETE as names. Why not
>   _SPECINSERT and _SPECDELETE or such?

Changed that on Github.

> * Iff we're going to have the XLOG_HEAP_AFFIRM record, I'd rather have
>   that guide the logical decoding code. Seems slightly cleaner.

I thought that you didn't think that would always work out? That in
some possible scenario it could break?

> * Still not a fan of the name 'arbiter' for the OC indexes.

What do you prefer? Seems to describe what we're talking about
reasonably well to me.

> * Gram.y needs a bit more discussion:
>   * Can anybody see a problem with changing the precedence of DISTINCT &
>     ON to nonassoc? Right now I don't see a problem given both are
>     reserved keywords already.

Why did you push code that solved this in a roundabout way, but
without actually reverting my original nonassoc changes (which would
now not result in shift/reduce conflicts?). What should we do about
that? Seems your unsure (otherwise you'd have reverted my thing). I
don't like that you seem to have regressed diagnostic output [1].
Surely it's simpler to use the nonassoc approach? I think this works
by giving the relevant keywords an explicit priority lower than '(',
so that a rule with ON CONFLICT '(' will shift rather than reducing a
conflicting rule (CONFLICT is an unreserved keyword). I wrote the code
so long ago that I can't really remember why I thought it was the
right thing, though.

>   * UpdateInsertStmt is a horrible name. OnConflictUpdateStmt maybe?

Done on Github.

>   * '(' index_params where_clause ')' is imo rather strange. The where
>     clause is inside the parens? That's quite different from the
>     original index clause.

I don't know. Maybe I was lazy about fixing shift/reduce conflicts.   :-)

I'll look at this some more.

> * SPEC_IGNORE,  /* INSERT of "ON CONFLICT IGNORE" */ looks like
>   a wrongly copied comment.

Not sure what you mean here. Please clarify.

> * The indentation in RoerderBufferCommit is clearly getting out of hand,
>   I've queued up a commit cleaning this up in my repo, feel free to merge.

Done on Github.

> * I don't think we use the term 'ordinary table' in error messages so
>   far.

Fixed on Github.

> * I still think it's unacceptable to redefine
>   XLOG_HEAP_LAST_MULTI_INSERT as XLOG_HEAP_SPECULATIVE_TUPLE like you
>   did. I'll try to find something better.

I did what you suggested in your follow-up e-mail (changes are on Github [2]).

> * I wonder if we now couldn't avoid changing heap_delete's API - we can
>   always super delete if we find a speculative insertion now. It'd be
>   nice not to break out of core callers if not necessary.

Maybe, but if there is a useful way to break out only a small subset
of heap_delete(), I'm not seeing it. Most of the callers that need a
new NULL argument are heap_insert() callers, actually. There are now 3
heap_delete() callers, up from 2.

> * breinbaas on IRC just mentioned that it'd be nice to have upsert as a
>   link in the insert. Given that that's the pervasive term that doesn't
>   seem absurd.

Not sure what you mean. Where would the link appear? It is kind of
hard to categorize that text so that we're strictly either talking
about INSERT or UPSERT. Might be possible, though.

> I think this is getting closer to a commit. Let's get this done.

Great!

The blockers to committing the IGNORE patch I see are:

* We need to tweak some of the logical decoding stuff a bit more, I
feel. Firm up on the details of whether we treat a confirmation record
as a logical decoding change that is involved in the new dance during
transaction reassembly.

* We need to sort out those issues with the grammar, since that only
really applies to the inference specification. Maybe the WHERE clause
that the inference specification accepts can be broken out. No ON
CONFLICT UPDATE specific issues left there, AFAICT though.

That really seems totally doable in just a couple of days.

The blockers for committing the ON CONFLICT UPDATE patch are larger, but doable:

* We need to figure out the tuple lock strength details. I think this
is doable, but it is the greatest challenge to committing ON CONFLICT
UPDATE at this point. Andres feels that we should require no greater
lock strength than an equivalent UPDATE. I suggest we get to this
after committing the IGNORE variant. We probably need to discuss this
some more.

* At the very least, I need to rebase my RLS patch onto what Stephen
just pushed (Dean's work). It would be nice to get Stephen and/or Dean
to review those aspects, as subject matter experts.

* It would also be nice to get someone like Tom to take a quick look
at what I'm doing in the optimizer, and in particular the rewriter.
Maybe he'd feel that the normalization that I'm doing (the
ExcludedExpr stuff) would be better suited to the optimizer. This
doesn't have to be a blocker to commit - it's just a suggestion.

Do you agree with my assessment? Did I miss a blocker? I'd like to
hear what Heikki has to say here too, now that he is pushing code to
Github. What needs more work before we can commit 1) ON CONFLICT
IGNORE, and 2) ON CONFLICT UPDATE?

Thanks

[1] https://github.com/petergeoghegan/postgres/commit/75d96c23676fd91568e9ec638470250c8b5e5f1b
[2] https://github.com/petergeoghegan/postgres/commit/0cfde636bf1ffca438418fa0c02043805e99f30f
-- 
Peter Geoghegan