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

On Mon, Feb 16, 2015 at 6:32 PM, Peter Geoghegan <pg@heroku.com> wrote:
> The best I think we can hope for is having a dedicated "if
> (TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
> InvalidTransactionId))" macro HeapTupleHeaderSuperDeleted() to do the
> work each time, which does not need to be checked so often. A second
> attached patch (compact_tqual_works.patch - which is non-broken,
> AFAICT) shows how this is possible, while also moving the check
> further down within each tqual.c routine (which seems more in keeping
> with the fact that super deletion is a relatively obscure concept).

I attach the patch series of V2.3. Highlights:

* Overhaul of speculative insertion related changes within tqual.c.
Refactored for readability as outlined in my earlier comments quoted
above. Assertions added, serving to show exactly where super deleted
tuples are and are not expected.

* Formally forbid INSERT ... ON CONFLICT into system catalogs. If
nothing else, this obviates the need for historic snapshots to care
about super deleted tuples.

* Minor setrefs.c tweaks. Minor ExecInitModifyTable() tweaks, too.

* Fix for minor bitrot against master branch.

* Further comments on the speculativeInsertionToken per-backend variable.

* Livelock insurance for exclusion constraints.

Importantly, Heikki wanted us to break out the patch to fix the
current problem of theoretical deadlock risks [1] ahead of committing
ON CONFLICT UPDATE/IGNORE. Heikki acknowledged that there were still
theoretical livelock risks in his reworked minimal patch. After
careful consideration, I have not broken out the changes to do this
incrementally along the lines that Heikki suggested.

Heikki seemed to think that the deadlock problems were not really
worth fixing independently of ON CONFLICT UPDATE support, but rather
represented a useful way of committing code incrementally. Do I have
that right? Certainly, anyone would agree that unprincipled deadlocks
(for regular inserters with exclusion constraints) are better than
livelocks. Heikki did not address the livelock risks with his minimal
reworked patch, which I've done here for ON CONFLICT.

The way I chose to break the livelock (what I call "livelock
insurance") does indeed involve comparing XIDs, which Heikki thought
most promising. But it also involves having the oldest XID wait on
another session's speculative token in the second phase, which
ordinarily does not occur in the second
phase/check_exclusion_or_unique_constraint() call. The idea is that
one session is guaranteed to be the waiter that has a second iteration
within its second-phase check_exclusion_or_unique_constraint() call,
where (following the super deletion of conflict TIDs by the other
conflicting session or sessions) reliably finds that it can proceed
(those other sessions are denied the opportunity to reach their second
phase by our second phase waiter's still-not-super-deleted tuple).

However, it's difficult to see how to map this on to general exclusion
constraint insertion + enforcement. In Heikki's recent sketch of this
[1], there is no pre-check, since that is considered an "UPSERT thing"
deferred until a later patch, and therefore my scheme here cannot work
(recall that for plain inserts with exclusion constraints, we insert
first and check last). I have a hard time justifying adding the
pre-check for plain exclusion constraint inserters given the total
lack of complaints from the field regarding unprincipled deadlocks,
and given the fact that it would probably make the code more
complicated than it needs to be. It is critical that there be a
pre-check to prevent livelock, though, because otherwise the
restarting sessions can go straight to their "second" phase
(check_exclusion_or_unique_constraint() call), without ever realizing
that they should not do so. Therefore, as I said, I have not broken
out the code in line with Heikki's suggestion.

It's possible that I have it wrong here - I was wrong to dismiss
Heikki's contention that the livelock hazards were fixable without too
much pain - but I don't think so.

It seems like the livelock insurance is pretty close to or actually
free, and doesn't imply that a "second phase wait for token" needs to
happen too often. With heavy contention on a small number of possible
tuples (100), and 8 clients deleting and inserting, I can see it
happening only once every couple of hundred milliseconds on my laptop.
It's not hard to imagine why the code didn't obviously appear to be
broken before now, as the window for an actual livelock must have been
small. Also, deadlocks bring about more deadlocks (since the deadlock
detector kicks in), whereas livelocks do not bring about more
livelocks.

I haven't been able to reproduce earlier apparent bugs with exclusion
constraints [2] recently. I can only speculate that they were fixed.
Does anyone with a big server care to run the procedure outlined for
exclusion constraints in the jjanes_upsert tool [3]? It would be nice
to have additional confidence that the exclusion constraint stuff is
robust.

[1] http://www.postgresql.org/message-id/54DFC6F8.5050108@vmware.com
[2] http://www.postgresql.org/message-id/CAM3SWZTkHOwyA5A9ib=uVf0Vs326yoCBdpp_NYkDjM2_-ScxFA@mail.gmail.com
[3] https://github.com/petergeoghegan/jjanes_upsert
--
Peter Geoghegan