From 97cec6232bdf8ccaf298d894f71a7f2760e2edd5 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Wed, 25 Jan 2023 15:59:35 +0300
Subject: [PATCH v1] Make ON CONFLICT DO NOTHING and ON CONFLICT DO UPDATE
 consistent

Prior to this commit we allowed self-conflicting inserts for DO NOTHING:

  CREATE TABLE t (a INT UNIQUE, b INT);
  INSERT INTO t VALUES (1,1), (1,2) ON CONFLICT DO NOTHING;
  -- succeeds, inserting the first row and ignoring the second

... but not for DO UPDATE:

  INSERT INTO t VALUES (1,1), (1,2) ON CONFLICT (a) DO UPDATE SET b = 0;
  ERROR:  ON CONFLICT DO UPDATE command cannot affect row a second time

Now DO UPDATE is handled similarly to DO NOTHING.

Author: Aleksander Alekseev
Reviewed-by: TODO FIXME
Discussion: TODO FIXME
---
 doc/src/sgml/ref/insert.sgml                  |  4 +-
 src/backend/access/heap/heapam.c              | 20 ++++++++-
 src/backend/access/heap/heapam_handler.c      |  4 +-
 src/backend/access/table/tableam.c            |  3 +-
 src/backend/executor/nodeModifyTable.c        | 43 ++++++++-----------
 src/include/access/heapam.h                   |  3 +-
 src/include/access/tableam.h                  |  7 +--
 src/test/regress/expected/constraints.out     | 28 ++++++------
 src/test/regress/expected/insert_conflict.out | 17 +++-----
 src/test/regress/sql/constraints.sql          |  4 +-
 src/test/regress/sql/insert_conflict.sql      |  6 +--
 11 files changed, 74 insertions(+), 65 deletions(-)

diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
index 7cea70329e..368e6f5bd6 100644
--- a/doc/src/sgml/ref/insert.sgml
+++ b/doc/src/sgml/ref/insert.sgml
@@ -531,9 +531,7 @@ INSERT INTO <replaceable class="parameter">table_name</replaceable> [ AS <replac
     clause is a <quote>deterministic</quote> statement.  This means
     that the command will not be allowed to affect any single existing
     row more than once; a cardinality violation error will be raised
-    when this situation arises.  Rows proposed for insertion should
-    not duplicate each other in terms of attributes constrained by an
-    arbiter index or constraint.
+    when this situation arises.
    </para>
 
    <para>
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index e6024a980b..37a3be7f09 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3129,7 +3129,7 @@ simple_heap_delete(Relation relation, ItemPointer tid)
 TM_Result
 heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
 			CommandId cid, Snapshot crosscheck, bool wait,
-			TM_FailureData *tmfd, LockTupleMode *lockmode)
+			TM_FailureData *tmfd, LockTupleMode *lockmode, bool force_visibility)
 {
 	TM_Result	result;
 	TransactionId xid = GetCurrentTransactionId();
@@ -3299,6 +3299,21 @@ l2:
 	locker_remains = false;
 	result = HeapTupleSatisfiesUpdate(&oldtup, cid, buffer);
 
+	if (force_visibility && (result == TM_Invisible))
+	{
+		/*
+		 * Special case of ON CONFLICT .. DO UPDATE with conflicting tuples
+		 * created within the same command. Normally a command doesn't see the
+		 * tuples created by this command e.g. to avoid Halloween problem.
+		 * However in this particular case we want to treat the tuples as if
+		 * they were inserted by the previosly executed command.
+		 *
+		 * This is done in order to make the behavior consistent with ON
+		 * CONFLICT .. DO NOTHING.
+		 */
+		result = TM_Ok;
+	}
+
 	/* see below about the "no wait" case */
 	Assert(result != TM_BeingModified || wait);
 
@@ -3457,6 +3472,7 @@ l2:
 			LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
 			heap_acquire_tuplock(relation, &(oldtup.t_self), *lockmode,
 								 LockWaitBlock, &have_tuple_lock);
+
 			XactLockTableWait(xwait, relation, &oldtup.t_self,
 							  XLTW_Update);
 			checked_lockers = true;
@@ -4174,7 +4190,7 @@ simple_heap_update(Relation relation, ItemPointer otid, HeapTuple tup)
 	result = heap_update(relation, otid, tup,
 						 GetCurrentCommandId(true), InvalidSnapshot,
 						 true /* wait for commit */ ,
-						 &tmfd, &lockmode);
+						 &tmfd, &lockmode, false);
 	switch (result)
 	{
 		case TM_SelfModified:
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index c4b1916d36..001addd780 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -314,7 +314,7 @@ static TM_Result
 heapam_tuple_update(Relation relation, ItemPointer otid, TupleTableSlot *slot,
 					CommandId cid, Snapshot snapshot, Snapshot crosscheck,
 					bool wait, TM_FailureData *tmfd,
-					LockTupleMode *lockmode, bool *update_indexes)
+					LockTupleMode *lockmode, bool *update_indexes, bool force_visibility)
 {
 	bool		shouldFree = true;
 	HeapTuple	tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);
@@ -325,7 +325,7 @@ heapam_tuple_update(Relation relation, ItemPointer otid, TupleTableSlot *slot,
 	tuple->t_tableOid = slot->tts_tableOid;
 
 	result = heap_update(relation, otid, tuple, cid, crosscheck, wait,
-						 tmfd, lockmode);
+						 tmfd, lockmode, force_visibility);
 	ItemPointerCopy(&tuple->t_self, &slot->tts_tid);
 
 	/*
diff --git a/src/backend/access/table/tableam.c b/src/backend/access/table/tableam.c
index ef0d34fcee..065c8e9e74 100644
--- a/src/backend/access/table/tableam.c
+++ b/src/backend/access/table/tableam.c
@@ -355,7 +355,8 @@ simple_table_tuple_update(Relation rel, ItemPointer otid,
 								GetCurrentCommandId(true),
 								snapshot, InvalidSnapshot,
 								true /* wait for commit */ ,
-								&tmfd, &lockmode, update_indexes);
+								&tmfd, &lockmode, update_indexes,
+								false /* don't force visibility */ );
 
 	switch (result)
 	{
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index f419c47065..2baf1ea7f5 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1955,7 +1955,7 @@ ExecUpdatePrepareSlot(ResultRelInfo *resultRelInfo,
 static TM_Result
 ExecUpdateAct(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
 			  ItemPointer tupleid, HeapTuple oldtuple, TupleTableSlot *slot,
-			  bool canSetTag, UpdateContext *updateCxt)
+			  bool canSetTag, UpdateContext *updateCxt, bool force_visibility)
 {
 	EState	   *estate = context->estate;
 	Relation	resultRelationDesc = resultRelInfo->ri_RelationDesc;
@@ -2089,7 +2089,7 @@ lreplace:
 								estate->es_crosscheck_snapshot,
 								true /* wait for commit */ ,
 								&context->tmfd, &updateCxt->lockmode,
-								&updateCxt->updateIndexes);
+								&updateCxt->updateIndexes, force_visibility);
 	if (result == TM_Ok)
 		updateCxt->updated = true;
 
@@ -2240,7 +2240,7 @@ ExecCrossPartitionUpdateForeignKey(ModifyTableContext *context,
 static TupleTableSlot *
 ExecUpdate(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
 		   ItemPointer tupleid, HeapTuple oldtuple, TupleTableSlot *slot,
-		   bool canSetTag)
+		   bool canSetTag, bool force_visibility)
 {
 	EState	   *estate = context->estate;
 	Relation	resultRelationDesc = resultRelInfo->ri_RelationDesc;
@@ -2298,7 +2298,7 @@ ExecUpdate(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
 
 redo_act:
 		result = ExecUpdateAct(context, resultRelInfo, tupleid, oldtuple, slot,
-							   canSetTag, &updateCxt);
+							   canSetTag, &updateCxt, force_visibility);
 
 		/*
 		 * If ExecUpdateAct reports that a cross-partition update was done,
@@ -2496,6 +2496,9 @@ ExecOnConflictUpdate(ModifyTableContext *context,
 	TransactionId xmin;
 	bool		isnull;
 
+	/* Is this a case of conflicting tuples created within the same command? */
+	bool		conflict_within_command = false;
+
 	/* Determine lock mode to use */
 	lockmode = ExecUpdateLockMode(context->estate, resultRelInfo);
 
@@ -2522,17 +2525,6 @@ ExecOnConflictUpdate(ModifyTableContext *context,
 			 * This can occur when a just inserted tuple is updated again in
 			 * the same command. E.g. because multiple rows with the same
 			 * conflicting key values are inserted.
-			 *
-			 * This is somewhat similar to the ExecUpdate() TM_SelfModified
-			 * case.  We do not want to proceed because it would lead to the
-			 * same row being updated a second time in some unspecified order,
-			 * and in contrast to plain UPDATEs there's no historical behavior
-			 * to break.
-			 *
-			 * It is the user's responsibility to prevent this situation from
-			 * occurring.  These problems are why the SQL standard similarly
-			 * specifies that for SQL MERGE, an exception must be raised in
-			 * the event of an attempt to update the same row twice.
 			 */
 			xminDatum = slot_getsysattr(existing,
 										MinTransactionIdAttributeNumber,
@@ -2541,12 +2533,14 @@ ExecOnConflictUpdate(ModifyTableContext *context,
 			xmin = DatumGetTransactionId(xminDatum);
 
 			if (TransactionIdIsCurrentTransactionId(xmin))
-				ereport(ERROR,
-						(errcode(ERRCODE_CARDINALITY_VIOLATION),
-				/* translator: %s is a SQL command name */
-						 errmsg("%s command cannot affect row a second time",
-								"ON CONFLICT DO UPDATE"),
-						 errhint("Ensure that no rows proposed for insertion within the same command have duplicate constrained values.")));
+			{
+				/*
+				 * Detect the case of ON CONFLICT .. DO UPDATE ..  with
+				 * conflicting tuples created within the same command.
+				 */
+				conflict_within_command = true;
+				break;
+			}
 
 			/* This shouldn't happen */
 			elog(ERROR, "attempted to lock invisible tuple");
@@ -2674,7 +2668,7 @@ ExecOnConflictUpdate(ModifyTableContext *context,
 	*returning = ExecUpdate(context, resultRelInfo,
 							conflictTid, NULL,
 							resultRelInfo->ri_onConflict->oc_ProjSlot,
-							canSetTag);
+							canSetTag, conflict_within_command);
 
 	/*
 	 * Clear out existing tuple, as there might not be another conflict among
@@ -2881,7 +2875,8 @@ lmerge_matched:
 				}
 				ExecUpdatePrepareSlot(resultRelInfo, newslot, context->estate);
 				result = ExecUpdateAct(context, resultRelInfo, tupleid, NULL,
-									   newslot, mtstate->canSetTag, &updateCxt);
+									   newslot, mtstate->canSetTag, &updateCxt,
+									   false);
 				if (result == TM_Ok && updateCxt.updated)
 				{
 					ExecUpdateEpilogue(context, &updateCxt, resultRelInfo,
@@ -3847,7 +3842,7 @@ ExecModifyTable(PlanState *pstate)
 
 				/* Now apply the update. */
 				slot = ExecUpdate(&context, resultRelInfo, tupleid, oldtuple,
-								  slot, node->canSetTag);
+								  slot, node->canSetTag, false);
 				break;
 
 			case CMD_DELETE:
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 417108f1e0..023aeff1a6 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -249,7 +249,8 @@ extern void heap_abort_speculative(Relation relation, ItemPointer tid);
 extern TM_Result heap_update(Relation relation, ItemPointer otid,
 							 HeapTuple newtup,
 							 CommandId cid, Snapshot crosscheck, bool wait,
-							 struct TM_FailureData *tmfd, LockTupleMode *lockmode);
+							 struct TM_FailureData *tmfd, LockTupleMode *lockmode,
+							 bool force_visibility);
 extern TM_Result heap_lock_tuple(Relation relation, HeapTuple tuple,
 								 CommandId cid, LockTupleMode mode, LockWaitPolicy wait_policy,
 								 bool follow_updates,
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 3fb184717f..996e99a5f9 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -526,7 +526,8 @@ typedef struct TableAmRoutine
 								 bool wait,
 								 TM_FailureData *tmfd,
 								 LockTupleMode *lockmode,
-								 bool *update_indexes);
+								 bool *update_indexes,
+								 bool force_visibility);
 
 	/* see table_tuple_lock() for reference about parameters */
 	TM_Result	(*tuple_lock) (Relation rel,
@@ -1506,12 +1507,12 @@ static inline TM_Result
 table_tuple_update(Relation rel, ItemPointer otid, TupleTableSlot *slot,
 				   CommandId cid, Snapshot snapshot, Snapshot crosscheck,
 				   bool wait, TM_FailureData *tmfd, LockTupleMode *lockmode,
-				   bool *update_indexes)
+				   bool *update_indexes, bool force_visibility)
 {
 	return rel->rd_tableam->tuple_update(rel, otid, slot,
 										 cid, snapshot, crosscheck,
 										 wait, tmfd,
-										 lockmode, update_indexes);
+										 lockmode, update_indexes, force_visibility);
 }
 
 /*
diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index e6f6602d95..226b3a133e 100644
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -429,21 +429,21 @@ INSERT INTO UNIQUE_TBL (t) VALUES ('six');
 INSERT INTO UNIQUE_TBL (t) VALUES ('seven');
 INSERT INTO UNIQUE_TBL VALUES (5, 'five-upsert-insert') ON CONFLICT (i) DO UPDATE SET t = 'five-upsert-update';
 INSERT INTO UNIQUE_TBL VALUES (6, 'six-upsert-insert') ON CONFLICT (i) DO UPDATE SET t = 'six-upsert-update';
--- should fail
-INSERT INTO UNIQUE_TBL VALUES (1, 'a'), (2, 'b'), (2, 'b') ON CONFLICT (i) DO UPDATE SET t = 'fails';
-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.
+-- should not fail
+INSERT INTO UNIQUE_TBL VALUES (11, 'a'), (22, 'b'), (22, 'b') ON CONFLICT (i) DO UPDATE SET t = 'fails';
 SELECT * FROM UNIQUE_TBL;
- i |         t          
----+--------------------
- 1 | one
- 2 | two
- 4 | four
-   | six
-   | seven
- 5 | five-upsert-update
- 6 | six-upsert-insert
-(7 rows)
+ i  |         t          
+----+--------------------
+  1 | one
+  2 | two
+  4 | four
+    | six
+    | seven
+  5 | five-upsert-update
+  6 | six-upsert-insert
+ 11 | a
+ 22 | fails
+(9 rows)
 
 DROP TABLE UNIQUE_TBL;
 CREATE TABLE UNIQUE_TBL (i int UNIQUE NULLS NOT DISTINCT, t text);
diff --git a/src/test/regress/expected/insert_conflict.out b/src/test/regress/expected/insert_conflict.out
index 9e9e3bd00c..773981a322 100644
--- a/src/test/regress/expected/insert_conflict.out
+++ b/src/test/regress/expected/insert_conflict.out
@@ -700,19 +700,13 @@ 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.
+insert into selfconflict values (4,1), (4,2) on conflict(f1) do update set f2 = -1;
 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.
+insert into selfconflict values (5,1), (5,2) on conflict(f1) do update set f2 = -2;
 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.
+insert into selfconflict values (6,1), (6,2) on conflict(f1) do update set f2 = -3;
 commit;
 select * from selfconflict;
  f1 | f2 
@@ -720,7 +714,10 @@ select * from selfconflict;
   1 |  1
   2 |  1
   3 |  1
-(3 rows)
+  4 | -1
+  5 | -2
+  6 | -3
+(6 rows)
 
 drop table selfconflict;
 -- check ON CONFLICT handling with partitioned tables
diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql
index 5ffcd4ffc7..f5bc7b096a 100644
--- a/src/test/regress/sql/constraints.sql
+++ b/src/test/regress/sql/constraints.sql
@@ -299,8 +299,8 @@ INSERT INTO UNIQUE_TBL (t) VALUES ('seven');
 
 INSERT INTO UNIQUE_TBL VALUES (5, 'five-upsert-insert') ON CONFLICT (i) DO UPDATE SET t = 'five-upsert-update';
 INSERT INTO UNIQUE_TBL VALUES (6, 'six-upsert-insert') ON CONFLICT (i) DO UPDATE SET t = 'six-upsert-update';
--- should fail
-INSERT INTO UNIQUE_TBL VALUES (1, 'a'), (2, 'b'), (2, 'b') ON CONFLICT (i) DO UPDATE SET t = 'fails';
+-- should not fail
+INSERT INTO UNIQUE_TBL VALUES (11, 'a'), (22, 'b'), (22, 'b') ON CONFLICT (i) DO UPDATE SET t = 'fails';
 
 SELECT * FROM UNIQUE_TBL;
 
diff --git a/src/test/regress/sql/insert_conflict.sql b/src/test/regress/sql/insert_conflict.sql
index 23d5778b82..42c5f6c778 100644
--- a/src/test/regress/sql/insert_conflict.sql
+++ b/src/test/regress/sql/insert_conflict.sql
@@ -434,15 +434,15 @@ 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;
+insert into selfconflict values (4,1), (4,2) on conflict(f1) do update set f2 = -1;
 commit;
 
 begin transaction isolation level repeatable read;
-insert into selfconflict values (5,1), (5,2) on conflict(f1) do update set f2 = 0;
+insert into selfconflict values (5,1), (5,2) on conflict(f1) do update set f2 = -2;
 commit;
 
 begin transaction isolation level serializable;
-insert into selfconflict values (6,1), (6,2) on conflict(f1) do update set f2 = 0;
+insert into selfconflict values (6,1), (6,2) on conflict(f1) do update set f2 = -3;
 commit;
 
 select * from selfconflict;
-- 
2.39.1

