Don't throw serialization errors for self-conflicts in INSERT ON CONFLICT.

tglsfdc · tglsfdc · commit c4016fcb1fd2 · 2016-10-23T18:36:13.000-04:00
A transaction that conflicts against itself, for example
	INSERT INTO t(pk) VALUES (1),(1) ON CONFLICT DO NOTHING;
should behave the same regardless of isolation level.  It certainly
shouldn't throw a serialization error, as retrying will not help.
We got this wrong due to the ON CONFLICT logic not considering the case,
as reported by Jason Dusek.

Core of this patch is by Peter Geoghegan (based on an earlier patch by
Thomas Munro), though I didn't take his proposed code refactoring for fear
that it might have unexpected side-effects.  Test cases by Thomas Munro
and myself.

Report: &lt;CAO3NbwOycQjt2Oqy2VW-eLTq2M5uGMyHnGm=RNga4mjqcYD7gQ@mail.gmail.com&gt;
Related-Discussion: &lt;57EE93C8.8080504@postgrespro.ru&gt;
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
@@ -195,9 +195,18 @@ ExecCheckHeapTupleVisible(EState *estate,
 		return;
 
 	if (!HeapTupleSatisfiesVisibility(tuple, estate->es_snapshot, buffer))
-		ereport(ERROR,
-				(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
+	{
+		/*
+		 * We should not raise a serialization failure if the conflict is
+		 * against a tuple inserted by our own transaction, even if it's not
+		 * visible to our snapshot.  (This would happen, for example, if
+		 * conflicting keys are proposed for insertion in a single command.)
+		 */
+		if (!TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetXmin(tuple->t_data)))
+			ereport(ERROR,
+					(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
 			 errmsg("could not serialize access due to concurrent update")));
+	}
 }
 
 /*
diff --git a/src/test/isolation/expected/insert-conflict-do-nothing-2.out b/src/test/isolation/expected/insert-conflict-do-nothing-2.out
@@ -0,0 +1,105 @@
+Parsed test spec with 2 sessions
+
+starting permutation: beginrr1 beginrr2 donothing1 c1 donothing2 c2 show
+step beginrr1: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step beginrr2: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step donothing1: INSERT INTO ints(key, val) VALUES(1, 'donothing1') ON CONFLICT DO NOTHING;
+step c1: COMMIT;
+step donothing2: INSERT INTO ints(key, val) VALUES(1, 'donothing2'), (1, 'donothing3') ON CONFLICT DO NOTHING;
+step c2: COMMIT;
+step show: SELECT * FROM ints;
+key            val            
+
+1              donothing1     
+
+starting permutation: beginrr1 beginrr2 donothing2 c2 donothing1 c1 show
+step beginrr1: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step beginrr2: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step donothing2: INSERT INTO ints(key, val) VALUES(1, 'donothing2'), (1, 'donothing3') ON CONFLICT DO NOTHING;
+step c2: COMMIT;
+step donothing1: INSERT INTO ints(key, val) VALUES(1, 'donothing1') ON CONFLICT DO NOTHING;
+step c1: COMMIT;
+step show: SELECT * FROM ints;
+key            val            
+
+1              donothing2     
+
+starting permutation: beginrr1 beginrr2 donothing1 donothing2 c1 c2 show
+step beginrr1: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step beginrr2: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step donothing1: INSERT INTO ints(key, val) VALUES(1, 'donothing1') ON CONFLICT DO NOTHING;
+step donothing2: INSERT INTO ints(key, val) VALUES(1, 'donothing2'), (1, 'donothing3') ON CONFLICT DO NOTHING; <waiting ...>
+step c1: COMMIT;
+step donothing2: <... completed>
+error in steps c1 donothing2: ERROR:  could not serialize access due to concurrent update
+step c2: COMMIT;
+step show: SELECT * FROM ints;
+key            val            
+
+1              donothing1     
+
+starting permutation: beginrr1 beginrr2 donothing2 donothing1 c2 c1 show
+step beginrr1: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step beginrr2: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step donothing2: INSERT INTO ints(key, val) VALUES(1, 'donothing2'), (1, 'donothing3') ON CONFLICT DO NOTHING;
+step donothing1: INSERT INTO ints(key, val) VALUES(1, 'donothing1') ON CONFLICT DO NOTHING; <waiting ...>
+step c2: COMMIT;
+step donothing1: <... completed>
+error in steps c2 donothing1: ERROR:  could not serialize access due to concurrent update
+step c1: COMMIT;
+step show: SELECT * FROM ints;
+key            val            
+
+1              donothing2     
+
+starting permutation: begins1 begins2 donothing1 c1 donothing2 c2 show
+step begins1: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step begins2: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step donothing1: INSERT INTO ints(key, val) VALUES(1, 'donothing1') ON CONFLICT DO NOTHING;
+step c1: COMMIT;
+step donothing2: INSERT INTO ints(key, val) VALUES(1, 'donothing2'), (1, 'donothing3') ON CONFLICT DO NOTHING;
+step c2: COMMIT;
+step show: SELECT * FROM ints;
+key            val            
+
+1              donothing1     
+
+starting permutation: begins1 begins2 donothing2 c2 donothing1 c1 show
+step begins1: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step begins2: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step donothing2: INSERT INTO ints(key, val) VALUES(1, 'donothing2'), (1, 'donothing3') ON CONFLICT DO NOTHING;
+step c2: COMMIT;
+step donothing1: INSERT INTO ints(key, val) VALUES(1, 'donothing1') ON CONFLICT DO NOTHING;
+step c1: COMMIT;
+step show: SELECT * FROM ints;
+key            val            
+
+1              donothing2     
+
+starting permutation: begins1 begins2 donothing1 donothing2 c1 c2 show
+step begins1: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step begins2: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step donothing1: INSERT INTO ints(key, val) VALUES(1, 'donothing1') ON CONFLICT DO NOTHING;
+step donothing2: INSERT INTO ints(key, val) VALUES(1, 'donothing2'), (1, 'donothing3') ON CONFLICT DO NOTHING; <waiting ...>
+step c1: COMMIT;
+step donothing2: <... completed>
+error in steps c1 donothing2: ERROR:  could not serialize access due to concurrent update
+step c2: COMMIT;
+step show: SELECT * FROM ints;
+key            val            
+
+1              donothing1     
+
+starting permutation: begins1 begins2 donothing2 donothing1 c2 c1 show
+step begins1: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step begins2: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step donothing2: INSERT INTO ints(key, val) VALUES(1, 'donothing2'), (1, 'donothing3') ON CONFLICT DO NOTHING;
+step donothing1: INSERT INTO ints(key, val) VALUES(1, 'donothing1') ON CONFLICT DO NOTHING; <waiting ...>
+step c2: COMMIT;
+step donothing1: <... completed>
+error in steps c2 donothing1: ERROR:  could not serialize access due to concurrent update
+step c1: COMMIT;
+step show: SELECT * FROM ints;
+key            val            
+
+1              donothing2     
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
@@ -25,6 +25,7 @@ test: eval-plan-qual
 test: lock-update-delete
 test: lock-update-traversal
 test: insert-conflict-do-nothing
+test: insert-conflict-do-nothing-2
 test: insert-conflict-do-update
 test: insert-conflict-do-update-2
 test: insert-conflict-do-update-3
diff --git a/src/test/isolation/specs/insert-conflict-do-nothing-2.spec b/src/test/isolation/specs/insert-conflict-do-nothing-2.spec
@@ -0,0 +1,34 @@
+# INSERT...ON CONFLICT DO NOTHING test with multiple rows
+# in higher isolation levels
+
+setup
+{
+  CREATE TABLE ints (key int primary key, val text);
+}
+
+teardown
+{
+  DROP TABLE ints;
+}
+
+session "s1"
+step "beginrr1" { BEGIN ISOLATION LEVEL REPEATABLE READ; }
+step "begins1" { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step "donothing1" { INSERT INTO ints(key, val) VALUES(1, 'donothing1') ON CONFLICT DO NOTHING; }
+step "c1" { COMMIT; }
+step "show" { SELECT * FROM ints; }
+
+session "s2"
+step "beginrr2" { BEGIN ISOLATION LEVEL REPEATABLE READ; }
+step "begins2" { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step "donothing2" { INSERT INTO ints(key, val) VALUES(1, 'donothing2'), (1, 'donothing3') ON CONFLICT DO NOTHING; }
+step "c2" { COMMIT; }
+
+permutation "beginrr1" "beginrr2" "donothing1" "c1" "donothing2" "c2" "show"
+permutation "beginrr1" "beginrr2" "donothing2" "c2" "donothing1" "c1" "show"
+permutation "beginrr1" "beginrr2" "donothing1" "donothing2" "c1" "c2" "show"
+permutation "beginrr1" "beginrr2" "donothing2" "donothing1" "c2" "c1" "show"
+permutation "begins1" "begins2" "donothing1" "c1" "donothing2" "c2" "show"
+permutation "begins1" "begins2" "donothing2" "c2" "donothing1" "c1" "show"
+permutation "begins1" "begins2" "donothing1" "donothing2" "c1" "c2" "show"
+permutation "begins1" "begins2" "donothing2" "donothing1" "c2" "c1" "show"
diff --git a/src/test/regress/expected/insert_conflict.out b/src/test/regress/expected/insert_conflict.out
@@ -727,3 +727,38 @@ select * from twoconstraints;
 (1 row)
 
 drop table twoconstraints;
+-- check handling of self-conflicts at various isolation levels
+create table selfconflict (f1 int primary key, f2 int);
+begin transaction isolation level read committed;
+insert into selfconflict values (1,1), (1,2) on conflict do nothing;
+commit;
+begin transaction isolation level repeatable read;
+insert into selfconflict values (2,1), (2,2) on conflict do nothing;
+commit;
+begin transaction isolation level serializable;
+insert into selfconflict values (3,1), (3,2) on conflict do nothing;
+commit;
+begin transaction isolation level read committed;
+insert into selfconflict values (4,1), (4,2) on conflict(f1) do update set f2 = 0;
+ERROR:  ON CONFLICT DO UPDATE command cannot affect row a second time
+HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.
+commit;
+begin transaction isolation level repeatable read;
+insert into selfconflict values (5,1), (5,2) on conflict(f1) do update set f2 = 0;
+ERROR:  ON CONFLICT DO UPDATE command cannot affect row a second time
+HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.
+commit;
+begin transaction isolation level serializable;
+insert into selfconflict values (6,1), (6,2) on conflict(f1) do update set f2 = 0;
+ERROR:  ON CONFLICT DO UPDATE command cannot affect row a second time
+HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.
+commit;
+select * from selfconflict;
+ f1 | f2 
+----+----
+  1 |  1
+  2 |  1
+  3 |  1
+(3 rows)
+
+drop table selfconflict;
diff --git a/src/test/regress/sql/insert_conflict.sql b/src/test/regress/sql/insert_conflict.sql
@@ -421,3 +421,35 @@ insert into twoconstraints values(2, '((0,0),(1,2))')
   on conflict on constraint twoconstraints_f2_excl do nothing;  -- do nothing
 select * from twoconstraints;
 drop table twoconstraints;
+
+-- check handling of self-conflicts at various isolation levels
+
+create table selfconflict (f1 int primary key, f2 int);
+
+begin transaction isolation level read committed;
+insert into selfconflict values (1,1), (1,2) on conflict do nothing;
+commit;
+
+begin transaction isolation level repeatable read;
+insert into selfconflict values (2,1), (2,2) on conflict do nothing;
+commit;
+
+begin transaction isolation level serializable;
+insert into selfconflict values (3,1), (3,2) on conflict do nothing;
+commit;
+
+begin transaction isolation level read committed;
+insert into selfconflict values (4,1), (4,2) on conflict(f1) do update set f2 = 0;
+commit;
+
+begin transaction isolation level repeatable read;
+insert into selfconflict values (5,1), (5,2) on conflict(f1) do update set f2 = 0;
+commit;
+
+begin transaction isolation level serializable;
+insert into selfconflict values (6,1), (6,2) on conflict(f1) do update set f2 = 0;
+commit;
+
+select * from selfconflict;
+
+drop table selfconflict;
