*: infoschema compatibility with prepare

[xhebox]

Signed-off-by: xhe [email protected]

What problem does this PR solve?

Problem Summary: #24613 has one workaround at planner/core/common_plans.go:L268. This PR removed it.

How it works

As the new design in #24613, you should never write: ret := &PreprocessorReturn{InfoSchema: is}, the introduction of this new struct is to return something from planner.Preprocess() without touching its API definition, instead of passing down something. But without that, TestPlanCacheSchemaChange will fail.

The failure of test is due to the incorrect infoschema. And you could blame the incorrectness to concurrent DDL leads to stale infoschema in parallel transactions.

It is so hard to solve that we only workaround the problem for some of the cases. Like prepare/execute statements does not choose the correct infoschema for updateRead(even if there are no parallel transactions), so it picked up the wrong cached plan. #22381 is a workaround for this problem.

I came up with the solution of returning the latest infoschema for pessimistic transaction, and it is a direct solution for concurrent DDL and stale infoschema problem. But it seems a little risky for @jackysp .

So here is another solution, instead of solving the rock hard concurrent DDL and stale infoschema problem, the solution of #22381 is refined and concurrent DDL for tk1 and tk3 in the test is manually removed. The test should be still effective, tk2 will responsible for the wanted SchemaChange cases: there is still concurrent DDL for tk1 and tk2.

Release note

• No release note

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

[xhebox]

Fixing tests
/hold

[jackysp]

I think we have an internal discussion for it, and it depends on the MDL feature in TiDB.

[xhebox]

@jackysp I'll pick another approach to fix instead.

[xhebox]

/unhold

[nolouch]

PTAL @lysu @cfzjywxk

[cfzjywxk]

I think it's a bit risky currently to change the used schema version in transaction execution. Now the possible inconsistency issue bettwen the current read and snapshot schema meta is not resolved completely or it's worked around by a temprora index in https://github.com/pingcap/tidb/pull/22152/files. There are some implicit logic in tidb logic assuming the information schema is always consistent during the executions.

[xhebox]

No, it is not a change in this PR. It is a change introduced by #22381. I only extract the logic outside IsCachedExecOK and optimize, which is essentially a cleanup. I did nothing to the current logic of execution.

The real change is the removal of tk3 and InfoSchema: is in ret := &PreprocessorReturn{InfoSchema: is}. The removal of InfoSchema: is will lead to the test error again, and the removal of tk3 fixed the test error by avoid concurrent DDL.

[cfzjywxk]

The inforschema change for the cached plan short path is ok as it's only for autocommit point get read, the extraction may affect the common path in whch change inforschema version is risky.

[xhebox]

OK, it is not a must for me to change that anyway. It is reverted.

[winoros]

Current changes are not the same as what the pr body describes?
I have no idea why the the new one can take effect:joy:

[xhebox]

Current changes are not the same as what the pr body describes?
I have no idea why the the new one can take effect😂

Same, except that the refine part is removed. In short, I want to remove PreprocessorReturn{Infoschema: is}, which will lead to concurrent DDL error in tests, and the corresponding tests are patched.

The cause for concurrent DDL error: if you specify infoschema in PreprocessorReturn, it is enforced to the latest infoschema, which will sync changes amomg three different transactions.

But it is actually not correct in another view of point. Our expectation is that infoschema is unchanged because txn startTS is never changed, and DDL is not executed in this session. Only some cases are special, e.g. forUpdate needs the latest infoschema.

My previous PR unify most getters of infoschema to return the infoschema of the transaction, if it is in transactions. This infoschema of transction does not sync among others, and that breaks some tests assuming synchronisation of infoschema amomg multiple transactions.

[ti-chi-bot]

@xhebox: 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.

[xhebox]

Closed in favor of #26759