planner: do not push down lock to pointGet/bacthPointGet when selection exists

[you06]

What problem does this PR solve?

Issue Number: close #21688

Problem Summary:

What is changed and how it works?

Proposal: xxx

What's Changed:

When using selection plan with pointGet/batchPointGet, #15257 will push down lock to pointGet/batchPointGet and remove the selectLock plan. This may wrongly lock indexes, this PR handle the invalid situation and do not push down in such cases.

How it Works:

Do not push down locks when selection plan exists.

Related changes

• Need to cherry-pick to the release-4.0

Check List

Tests

• Unit test

Release note

• Fix a bug that point get with extra where conditions may wrongly lock keys.

[blacktear23]

@you06 pointGet and batchPointGet will lock not exists index keys, this is because in MySQL if you select for update finally get an empty result it will use GapLock to prevent other transaction insert or update such field with the same value. But MySQL can let other transactions set a read lock over that gap lock.
So lock the not exists keys is reasonable for keep repeatable read isolation in TiDB. But in TiDB locks means write lock so that will cause other transaction will block at executing select statements.
@cfzjywxk PTAL.

[you06]

@you06 pointGet and batchPointGet will lock not exists index keys, this is because in MySQL if you select for update finally get an empty result it will use GapLock to prevent other transaction insert or update such field with the same value. But MySQL can let other transactions set a read lock over that gap lock.
So lock the not exists keys is reasonable for keep repeatable read isolation in TiDB. But in TiDB locks means write lock so that will cause other transaction will block at executing select statements.
@cfzjywxk PTAL.

Lock non-exist key in point-get is reasonable, the problems comes when we use a point get for faster plan but don't actually want it locks key, eg.

create table t(c1 int, c2 int, unique index u0(c1));
insert into t values(1, 1);
begin pessimistic;
explain select * from t where c1 = 2 and c2 = 2 for update;
+---------------------+---------+------+-----------------------+----------------------+
| id                  | estRows | task | access object         | operator info        |
+---------------------+---------+------+-----------------------+----------------------+
| Projection_5        | 0.00    | root |                       | test.t.c1, test.t.c2 |
| └─Selection_8       | 0.00    | root |                       | eq(test.t.c2, 2)     |
|   └─Point_Get_7     | 1.00    | root | table:t, index:u0(c1) | lock                 |
+---------------------+---------+------+-----------------------+----------------------+
explain select * from t where c2 = 2 for update;
+-----------------------------+---------+-----------+---------------+--------------------------------+
| id                          | estRows | task      | access object | operator info                  |
+-----------------------------+---------+-----------+---------------+--------------------------------+
| Projection_5                | 0.00    | root      |               | test.t.c1, test.t.c2           |
| └─SelectLock_6              | 0.00    | root      |               | for update 0                   |
|   └─TableReader_9           | 0.00    | root      |               | data:Selection_8               |
|     └─Selection_8           | 0.00    | cop[tikv] |               | eq(test.t.c2, 2)               |
|       └─TableFullScan_7     | 1.00    | cop[tikv] | table:t       | keep order:false, stats:pseudo |
+-----------------------------+---------+-----------+---------------+--------------------------------+

Compare with c1 = 2 and c2 = 2, c2 = 2 obviously has a wider condition, but it locks less keys, this does not make sense to me. IMO, non-exist keys should be only locked when the query condition coincides a unique index, fully covered and no extra.

Further more, a test case(TestIssue20692) relies on non-exist index lock on c1 = 2 and c2 = 2, that's why this PR is blocked. I do think how we solve #20692 is not good enough.

[blacktear23]

@you06 in MySQL select * from t where c2 = 2 for update; will lock table t. And this is how MySQL GapLock works. In TiDB we do not have the GapLock, so select * from t where c2 = 2 for update; will lock nothing. But select * from t where c1 = 2 and c2 = 2 for update; will lock c1 = 2. But if you execute it in MySQL, select * from t where c1 = 2 and c2 = 2 for update; MySQL will apply a GapLock on c1 = 2 and the range is related to what we have in table.

[blacktear23]

If we want to solve this problem perfectly we should introduce something like GapLock in TiDB. And that will be a big project.

[you06]

If we want to solve this problem perfectly we should introduce something like GapLock in TiDB. And that will be a big project.

Yeah, GapLock is not in roadmap due to a great performance impact.

[cfzjywxk]

@blacktear23
We will review the expected lock behaviours in detail in the near future, as we have different choices for these behaviours if we want to be compatible with MySQL or Oracle.
@you06 will make a document about this, I think we need more discussions.

[ti-chi-bot]

@you06: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.