The update ignore may still report dup-key error when retry

[lcwangchao]

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

This unit test fails

func TestUpdateRowRetryAndThenDupKey(t *testing.T) {
	store := testkit.CreateMockStore(t)
	tk := testkit.NewSteppedTestKit(t, store)
	tk.MustExec("use test")
	tk.MustExec("create table t(id int primary key, u int unique)")
	tk.MustExec("insert into t values(1, 1)")

	// session 1 update u=2 for id=1 and halt before executor first runs.
	tk.SetBreakPoints(sessiontxn.BreakPointBeforeExecutorFirstRun)
	tk.SteppedMustExec("update ignore t set u = 2 where id = 1").
		ExpectStopOnBreakPoint(sessiontxn.BreakPointBeforeExecutorFirstRun)

	// session 2  insert a new row (2, 2) to make the unique key conflict.
	tk2 := testkit.NewTestKit(t, store)
	tk2.MustExec("use test")
	tk2.MustExec("insert into t values(2, 2)")

	// Continue the execution of session1, it should meet an optimistic conflict and retry.
	// The second execution is still failed because of the unique key conflict.
	// But the `update ignore` statement should not give any error.
	tk.Continue().ExpectStopOnBreakPoint(sessiontxn.BreakPointBeforeExecutorFirstRun)
	tk.Continue().ExpectIdle()
	// Should only a dup-key warning and the row 1 is not updated.
	tk.MustQuery("show warnings").Check(testkit.Rows("Warning 1062 Duplicate entry '2' for key 't.u'"))
	tk.MustQuery("select * from t order by id").Check(testkit.Rows("1 1", "2 2"))
}

2. What did you expect to see? (Required)

The test should pass.

3. What did you see instead (Required)

The test fails with error:

        	Error:      	Received unexpected error:
        	            	[kv:1062]Duplicate entry '2' for key 't.u'

4. What is your TiDB version? (Required)

[lcwangchao]

I'll explain how it happens. The table t are created and inited with one row:

TiDB root@127.0.0.1:test> show create table t;
+-------+-------------------------------------------------------------+
| Table | Create Table                                                |
+-------+-------------------------------------------------------------+
| t     | CREATE TABLE `t` (                                          |
|       |   `id` int(11) NOT NULL,                                    |
|       |   `u` int(11) DEFAULT NULL,                                 |
|       |   PRIMARY KEY (`id`) /*T![clustered_index] CLUSTERED */,    |
|       |   UNIQUE KEY `u` (`u`)                                      |
|       | ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |
+-------+-------------------------------------------------------------+
1 row in set
Time: 0.015s
TiDB root@127.0.0.1:test> select * from t;
+----+---+
| id | u |
+----+---+
| 1  | 1 |
+----+---+
1 row in set
Time: 0.012s

Then session 1 trys to update this with insert ignore outside a transaction:

update t set u=2 where id=1;

Because it is not in an explicit transaction, TiDB will try to use optimistic txn mode to update this row. It will succeed when updating row 1 in memory because there is no conflict.

Then another session, session2 inserts a new row (2, 2) to the table t before session1 commits. This will cause session to retry the whole SQL because an optimistic conflict is detected in the commit phase. The second try will use the pessimistic mode:

tidb/pkg/session/session.go

Lines 3891 to 3893 in 08147e7

	if s.sessionVars.RetryInfo.Retrying {
	txnMode = ast.Pessimistic
	}

It will cause UpdateRecord to enter a branch:

tidb/pkg/table/tables/tables.go

Lines 579 to 589 in 08147e7

	if !sessVars.InTxn() {
	savePresumeKeyNotExist := sessVars.PresumeKeyNotExists
	if !sessVars.ConstraintCheckInPlace && sessVars.TxnCtx.IsPessimistic {
	sessVars.PresumeKeyNotExists = true
	}
	err = t.rebuildIndices(sctx, txn, h, touched, oldData, newData, table.WithCtx(ctx))
	sessVars.PresumeKeyNotExists = savePresumeKeyNotExist
	if err != nil {
	return err
	}
	} else {

That sets PresumeKeyNotExists to true which means the key conflict check will be postponed to commit phase. However update ignore requires this value to be false because it needs to handle the dup-key error in UpdateExec to discard invalid updates. So the dup-key errors happens in commit phase and no code will handle it.

[tiancaiamao]

So I understand a bit about the background now.
In the past, we try to adapt an optimization: don't check the duplicate entry immediately, instead use PresumeKeyNotExists and let 2PC to check it until the txn commit.

However, the update ingore case is not fit for this optimization: it should never get a duplicate entry error. And the previous code does not handle this case well.