Re: INSERT ... ON CONFLICT {UPDATE | IGNORE} 2.0 - Mailing list pgsql-hackers

On Fri, Feb 6, 2015 at 1:51 PM, Bruce Momjian <bruce@momjian.us> wrote:
> Other than the locking part, the biggest part of this patch is adjusting
> things so that an INSERT can change into an UPDATE.

Thanks for taking a look at it. That's somewhat cleaned up in the
attached patchseries - V2.2. This has been rebased to repair the minor
bit-rot pointed out by Thom.

Highlights:

* Better parser representation.

The auxiliary UPDATE never uses its own RangeVar (in fact,
UpdateStmt.relation is never set). This eliminates any possibility of
repeated name lookup problems, addressing Andres' concern. But it also
results in better code. The auxiliary UPDATE is not modified by the
parent INSERT at all - rather, the UPDATE knows to fetch its target
Relation from the parsestate parent INSERT. There is no need to
artificially cut off the parent within the auxiliary UPDATE to make
sure the independent RTE is not visible (during parse analysis, prior
to merging the two, as happened in earlier revisions) - that was a
kludge that I'm glad to be rid of. There is no merging of distinct
INSERT and UPDATE Relations/RTEs because there is only ever one
Relation/RTE to begin with. Previously, the merging merged RTE
selectedCols and updatedCols into the parent INSERT (for column-level
privileges, for example). I'm also a lot less cute about determining
whether an UPDATE is an auxiliary UPDATE from within the parser, which
was also a concern raised by Andres.

About 90% of the special case code previously in transformInsertStmt()
is now in setTargetTable(). This is a significant improvement all
around, since the code is now more consistent with existing parse
analysis code - setTargetTable() is naturally where the auxilary
UPDATE figures out details on its target, and builds an EXCLUDED RTE
and adds it to the namespace as a special case (just like for regular
UPDATE targets, which similarly get added to the namespace +
joinlist).

All of this implies a slight behavioral change (which is documented):
The TARGET.* alias is now visible everywhere. So you see it within
every node of EXPLAIN output, and if you want to qualify a RETURNING
column, the TARGET.* alias must be used (not the original table name).
I think that this is an improvement too, although it is arguably a
slight behavioral change to INSERTs in general (can't think why anyone
would particularly want to qualify with an alias in INSERT's
RETURNING, though). Note that the EXCLUDED.* pseudo-alias is still
only visible within the UPDATE's targetlist and WHERE clause. I think
it would be a bad idea to make the EXCLUDED.* tuples visible from
RETURNING [1].

* Cleaner ExecInsert() control flow. Andres rightly complained that
the existing control flow was convoluted. I believe he will find this
revision a lot clearer, although I have not gone so far as creating
something like an ExecUpsert().

* Better documentation. The executor README has been overhauled to
describe the flow of things from a higher level. The procarray changes
are better documented by comments, too.

* Special work previously within BuildIndexInfo() that is needed for
unique indexes for the UPSERT case only is now done only in the UPSERT
case. There is now no added overhead in BuildIndexInfo() for existing
cases.

* Worked on feedback on various points of style raised by Andres (e.g.
an errhint() was removed).

* Better explanation of the re-use of XLOG_HEAP* flag bits. I believe
that it's fine to reuse the "(1<<7)" bit, given that each distinct use
of the bit can only appear in distinct record types (that is, the bit
is used by xl_heap_multi_insert, and now xl_heap_delete). Those two
uses should be mutually exclusive. It's not as if we've had to be
economical with the use of heap flag XLog record bits before now, so
the best approach here isn't obvious. For now, I see no problem with
this reuse.

* SnapshotSelf (that is, HeapTupleSatisfiesSelf()) has additions
analogous to previous additions to the HeapTupleSatisfiesVacuum() and
HeapTupleSatisfiesMVCC() visibility routines. I still don't think that
the changes to tqual.c are completely satisfactory, but as long as
they're directly necessary (which they evidently are - Jeff's
stress-testing tool shows that) then I should at least make the
appropriate changes everywhere. We should definitely focus on why
they're necessary, and consider if we can do better.

* There was some squashing of commits, since Andres felt that they
weren't all useful as separate commits. I've still split out the RTE
permissions commit, as well as the RLS commit (plus the documentation
and test commits, FWIW). I hope that this will make it easier to
review parts of the patch, without being generally excessive.

When documentation and tests are left out, the entire patch series is left at:

 68 files changed, 2958 insertions(+), 297 deletions(-)

which is not too big.

Thanks

[1] http://www.postgresql.org/message-id/CAM3SWZTcpy9rroLM3TkfuU4HDLrEtuGzxLptGn2vLhVAFwQCVA@mail.gmail.com
--
Peter Geoghegan