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

On 2015-04-19 21:37:51 -0700, Peter Geoghegan wrote:
> Attached patch, V3.4, implements what I believe you and Heikki have in
> mind here.

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.

* The logical stuff looks much saner.

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

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

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

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

* 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.
The reason the conflict exists AFAICS is because something like
INSERT INTO foo SELECT DISTINCT ON CONFLICT IGNORE;
is allowed by the grammar. The need for the nonassoc could be
avoided by requiring DISTINCT to be followed by a column. We
currently *do* enforce that, just not in the parser (c.f.
transformDistinctClause). That requires one more production in
simple_select, and a nonoptional distinct clause.
I've queued up a commit cleaning this up in my repo, feel free to
merge and polish.
* UpdateInsertStmt is a horrible name. OnConflictUpdateStmt maybe?
* '(' index_params where_clause ')' is imo rather strange. The where
clause is inside the parens? That's quite different from the
original index clause.
* SPEC_IGNORE, /* INSERT of "ON CONFLICT IGNORE" */ looks like
a wrongly copied comment.
* 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.
* I don't think we use the term 'ordinary table' in error messages so
far.
* 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 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.
* 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.

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

Greetings,

Andres Freund