strange behavior when begin transaction when 'tidb_snapshot' is set

[lcwangchao]

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

s1, and s2 are two tidb connections.

s1	s2
create table test.ttt (id int primary key, a int);
insert into test.ttt values(1, 1);
	begin;
update test.ttt set a=2 where id=1;
	do sleep(1);set @@tidb_snapshot=TIMESTAMP(NOW());
	select a from test.ttt where id=1;
	select a from test.ttt where id=1 for update;
	select a from test.ttt where id=1;

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

mysql> select a from test.ttt where id=1;
+------+
| a    |
+------+
|    1 |
+------+
1 row in set (0.00 sec)

mysql> select a from test.ttt where id=1 for update;
+------+
| a    |
+------+
|    2 |
+------+
1 row in set (0.00 sec)

mysql> select a from test.ttt where id=1;
+------+
| a    |
+------+
|    1 |
+------+
1 row in set (0.00 sec)

3. What did you see instead (Required)

mysql> select a from test.ttt where id=1;
+------+
| a    |
+------+
|    1 |
+------+
1 row in set (0.00 sec)

mysql> select a from test.ttt where id=1 for update;
+------+
| a    |
+------+
|    2 |
+------+
1 row in set (0.00 sec)

mysql> select a from test.ttt where id=1;
+------+
| a    |
+------+
|    2 |
+------+
1 row in set (0.00 sec)

4. What is your TiDB version? (Required)

master

[lcwangchao]

@cfzjywxk PTAL

[lcwangchao]

It seems when building a executor, getSnapshotTs's return uses sys var 'SnapshotTS' once it isset ignoring the txn's startTs.

tidb/executor/builder.go

Lines 1397 to 1404 in fbbada3

	snapshotTS := b.ctx.GetSessionVars().SnapshotTS
	if snapshotTS == 0 {
	txn, err := b.ctx.Txn(true)
	if err != nil {
	return 0, err
	}
	snapshotTS = txn.StartTS()
	}

And when opening PointGetExecutor, it will step to line 135 if for update read ts is bigger than txn read ts, thats why it only happends after a select ... for update clause.

tidb/executor/point_get.go

Lines 132 to 136 in fbbada3

	if e.txn.Valid() && txnCtx.StartTS == txnCtx.GetForUpdateTS() {
	e.snapshot = e.txn.GetSnapshot()
	} else {
	e.snapshot = e.ctx.GetStore().GetSnapshot(kv.Version{Ver: snapshotTS})
	}

[cfzjywxk]

It seems when building a executor, getSnapshotTs's return uses sys var 'SnapshotTS' once it isset ignoring the txn's startTs.

tidb/executor/builder.go

Lines 1397 to 1404 in fbbada3

	snapshotTS := b.ctx.GetSessionVars().SnapshotTS
	if snapshotTS == 0 {
	txn, err := b.ctx.Txn(true)
	if err != nil {
	return 0, err
	}
	snapshotTS = txn.StartTS()
	}

And when opening PointGetExecutor, it will step to line 135 if for update read ts is bigger than txn read ts, thats why it only happends after a select ... for update clause.

tidb/executor/point_get.go

Lines 132 to 136 in fbbada3

	if e.txn.Valid() && txnCtx.StartTS == txnCtx.GetForUpdateTS() {
	e.snapshot = e.txn.GetSnapshot()
	} else {
	e.snapshot = e.ctx.GetStore().GetSnapshot(kv.Version{Ver: snapshotTS})
	}

@lcwangchao
Thanks for the feedback, the behaviour is strange and we'll disscuss about the tidb_snapshot variable when it's used with the pessimistic transaction.

[cfzjywxk]

/cc
@jackysp @MyonKeminta
We may need to discuss about this behaviour setting snapshot ts during transaction execution. For pessimistic transactions, if the snapshot is set and other features like read-committed is used, I'm afraid there could be unexpected behaviours or possbile risks.

[jackysp]

I think we should narrow down the scope of tidb snapshot.

[MyonKeminta]

I agree to limit the usage of this variable. IMHO it should never be used with transactions. But limiting its usage can potentially cause compatibility issues between new and old versions of TiDB.