Skip to content

*: consitent get infoschema (#24230)#24528

Closed
ti-srebot wants to merge 1 commit intopingcap:release-4.0from
ti-srebot:release-4.0-5e9e0e6e37be
Closed

*: consitent get infoschema (#24230)#24528
ti-srebot wants to merge 1 commit intopingcap:release-4.0from
ti-srebot:release-4.0-5e9e0e6e37be

Conversation

@ti-srebot
Copy link
Contributor

@ti-srebot ti-srebot commented May 10, 2021

cherry-pick #24230 to release-4.0
You can switch your code base to this Pull Request by using git-extras:

# In tidb repo:
git pr https://github.com/pingcap/tidb/pull/24528

After apply modifications, you can push your change to this PR via:

git push git@github.com:ti-srebot/tidb.git pr/24528:release-4.0-5e9e0e6e37be

What problem does this PR solve?

The main issue: #24233

Problem Summary: In some cases, TxnCtx.InfoSchema is directly used while there may be SnapshotInfoSchema. There was a similar fix #15258, only for distsql. There are more left cases:

executor/coprocessor.go: buildDAGExecutor
executor/infoschema_reader.go: setDataForTiKVRegionStatus, setDataForTiDBHotRegions
expression/builtin_info.go: builtinNextValSig.evalInt, builtinLastValSig.evalInt, builtinSetValSig.evalInt
planner/core/expression_rewriter.go: rewriteAstExpr
planner/core/integration_test.go: TestPartitionPruningForEQ

While I want to replace all TxnCtx.InfoSchema into infoschema.GetInfoSchema(sessionCtx), I found that there will be a circular dependency in expression -> ... -> infoschema -> expression.

So I moved GetInfoSchema function into sessionctx/variable, as a method of SessionVars. Then replaced all calls into sessionVars.GetInfoSchema().(infoschema.InfoSchema). For expression package, it is sessionVars.GetInfoSchema().(util.SequenceSchema). That is why this PR is large.

The call is a bit longer with the extra GetSessionVars() and type cast .(infoschema.InfoSchema), but it works and removed one of four sessionctx imports in the infoschema package. Hopefully infoschema will not depend on sessionctx in the future...

Check List

Tests

  • Unit test
  • Integration test

Release note

  • Fix tidb_snapshot did not take effect in some cases

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-chi-bot
Copy link
Member

[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.

Details

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot ti-srebot added component/expression sig/execution SIG execution sig/sql-infra SIG: SQL Infra size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/4.0-cherry-pick labels May 10, 2021
@ti-chi-bot ti-chi-bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 10, 2021
@ti-srebot ti-srebot added this to the v4.0.13 milestone May 10, 2021
@ti-chi-bot ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 10, 2021
@ti-srebot
Copy link
Contributor Author

@xhebox you're already a collaborator in bot's repo.

@xhebox xhebox closed this May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/expression sig/execution SIG execution sig/sql-infra SIG: SQL Infra size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. type/4.0-cherry-pick

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants