Re: INSERT ... ON CONFLICT IGNORE (and UPDATE) 3.0 - Mailing list pgsql-hackers

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

* 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
AFAICSis because something like   INSERT INTO foo SELECT DISTINCT ON CONFLICT IGNORE;   is allowed by the grammar. The
needfor 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
anonoptional distinct clause.   I've queued up a commit cleaning this up in my repo, feel free to   merge and polish. *
UpdateInsertStmtis 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'lltry 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
insertionnow. 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
pervasiveterm that doesn't seem absurd.
 

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

Greetings,

Andres Freund