From 122493c8d197fa7ccd58de984b6bb0b2374c0b7d Mon Sep 17 00:00:00 2001 From: Aleksander Alekseev Date: Wed, 25 Jan 2023 15:59:35 +0300 Subject: [PATCH v2] 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: Andres Freund, TODO FIXME Discussion: https://postgr.es/m/CAJ7c6TPQJNFETz9H_qPpA3x7ybz2D1QMDtBku_iK33gT3UR34Q%40mail.gmail.com --- 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 | 45 ++++++++++++++----- src/test/regress/sql/constraints.sql | 4 +- src/test/regress/sql/insert_conflict.sql | 37 +++++++++++++-- 11 files changed, 133 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 table_name [ AS deterministic 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. 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..3641dc571a 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 = 'success'; 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 | success +(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..f22a54ce0e 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,9 +714,40 @@ select * from selfconflict; 1 | 1 2 | 1 3 | 1 -(3 rows) + 4 | -1 + 5 | -2 + 6 | -3 +(6 rows) drop table selfconflict; +-- check if self-conflicting INSERTs work with triggers +create table t (a int unique, b int); +create or replace function t_insert() returns trigger as $$ +begin + raise notice 't_insert triggered: new = %, old = %', new, old; + return null; +end +$$ language 'plpgsql'; +create or replace function t_update() returns trigger as $$ +begin + raise notice 't_update triggered: new = %, old = %', new, old; + return null; +end +$$ language 'plpgsql'; +create trigger t_insert_trigger +after insert on t +for each row execute procedure t_insert(); +create trigger t_insert_update +after update on t +for each row execute procedure t_update(); +insert into t values (1,1), (1,2) on conflict (a) do update set b = 0; +NOTICE: t_insert triggered: new = (1,1), old = +NOTICE: t_update triggered: new = (1,0), old = (1,1) +insert into t values (2,1), (2,2), (3,1) on conflict (a) do update set b = 0; +NOTICE: t_insert triggered: new = (2,1), old = +NOTICE: t_update triggered: new = (2,0), old = (2,1) +NOTICE: t_insert triggered: new = (3,1), old = +drop table t; -- check ON CONFLICT handling with partitioned tables create table parted_conflict_test (a int unique, b char) partition by list (a); create table parted_conflict_test_1 partition of parted_conflict_test (b unique) for values in (1, 2); diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql index 5ffcd4ffc7..c900d42a4a 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 = 'success'; SELECT * FROM UNIQUE_TBL; diff --git a/src/test/regress/sql/insert_conflict.sql b/src/test/regress/sql/insert_conflict.sql index 23d5778b82..96054122a0 100644 --- a/src/test/regress/sql/insert_conflict.sql +++ b/src/test/regress/sql/insert_conflict.sql @@ -434,21 +434,52 @@ 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; drop table selfconflict; +-- check if self-conflicting INSERTs work with triggers + +create table t (a int unique, b int); + +create or replace function t_insert() returns trigger as $$ +begin + raise notice 't_insert triggered: new = %, old = %', new, old; + return null; +end +$$ language 'plpgsql'; + +create or replace function t_update() returns trigger as $$ +begin + raise notice 't_update triggered: new = %, old = %', new, old; + return null; +end +$$ language 'plpgsql'; + +create trigger t_insert_trigger +after insert on t +for each row execute procedure t_insert(); + +create trigger t_insert_update +after update on t +for each row execute procedure t_update(); + +insert into t values (1,1), (1,2) on conflict (a) do update set b = 0; +insert into t values (2,1), (2,2), (3,1) on conflict (a) do update set b = 0; + +drop table t; + -- check ON CONFLICT handling with partitioned tables create table parted_conflict_test (a int unique, b char) partition by list (a); create table parted_conflict_test_1 partition of parted_conflict_test (b unique) for values in (1, 2); -- 2.39.1