public inbox for [email protected]
help / color / mirror / Atom feedFrom: Aleksander Alekseev <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Subject: [PATCH] Make ON CONFLICT DO NOTHING and ON CONFLICT DO UPDATE consistent
Date: Wed, 25 Jan 2023 18:45:12 +0300
Message-ID: <CAJ7c6TPQJNFETz9H_qPpA3x7ybz2D1QMDtBku_iK33gT3UR34Q@mail.gmail.com> (raw)
Hi hackers,
Currently we allow self-conflicting inserts for ON CONFLICT 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 ON CONFLICT .. 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
HINT: Ensure that no rows proposed for insertion within the same
command have duplicate constrained values.
```
Tom pointed out in 2016 that this is actually a bug [1] and I agree.
The proposed patch fixes this.
[1]: https://www.postgresql.org/message-id/22438.1477265185%40sss.pgh.pa.us
--
Best regards,
Aleksander Alekseev
Attachments:
[application/octet-stream] v1-0001-Make-ON-CONFLICT-DO-NOTHING-and-ON-CONFLICT-DO-UP.patch (17.2K, ../CAJ7c6TPQJNFETz9H_qPpA3x7ybz2D1QMDtBku_iK33gT3UR34Q@mail.gmail.com/2-v1-0001-Make-ON-CONFLICT-DO-NOTHING-and-ON-CONFLICT-DO-UP.patch)
download | inline diff:
From 97cec6232bdf8ccaf298d894f71a7f2760e2edd5 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <[email protected]>
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
view thread (2+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: [PATCH] Make ON CONFLICT DO NOTHING and ON CONFLICT DO UPDATE consistent
In-Reply-To: <CAJ7c6TPQJNFETz9H_qPpA3x7ybz2D1QMDtBku_iK33gT3UR34Q@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox