*: avoid pass session ID by context & fix no schemaChecker error log

[tiancaiamao]

What problem does this PR solve?

Issue Number: close #22783

Problem Summary:

Pass important argument by context.WithValue is a bad idea.

The callee need the data in the context.Context, but the caller maybe passing a context.Bacground()
#22783 is caused by such kind of refactoring.

In the beginning, we just use the session ID for logging.
Now I review the code and find we're using sessionID == 0 to check whether it's an internal transaction or a user's transaction.

What is changed and how it works?

Introduce a txn.SetOption(kv.SessionID, id) to replace the old way of passing session ID by context.Context.

What's Changed:

Use txn.SetOption(kv.SessionID, id) instead of txn.Commit(tikvutil.SetSessionID(ctx, s.GetSessionVars().ConnectionID)) to pass the session ID.

How it Works:

Related changes

• PR to update pingcap/docs/pingcap/docs-cn:
• Need to cherry-pick to the release branch

Check List

Tests

• No code

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

[cfzjywxk]

Will this make the following logic executed unexpected for the inner session statements whose session id is 0?

[tiancaiamao]

For internal queries, val is always nil; for external queries sessionID > 0 always hold,
so val == nil || sessionID > 0 is basically always true? @MyonKeminta

[tiancaiamao]

Now I've change it to:
sessionID > 0 => onCommit
sessionID == 0 => nothing

The original author means to call onCommit for external queries (although val == nil may be confusing in the old code).
@cfzjywxk

[lysu]

maybe it's hard to distinguish external from internal queries form sessionID?

InRestrictedSQL also can call doCommit

tidb/session/session.go

Line 521 in c3f9b08

s.txn.SetOption(kv.SessionID, s.GetSessionVars().ConnectionID)

[tiancaiamao]

If we take a session from the session pool to execute the SQL, sessionID is not set.
However, if we direct use the session to execute a internal SQL, that might be a problem.

[lysu]

it seems some other test case

tidb/store/tikv/region_request.go

Line 413 in 18cbfaa

if v := bo.GetCtx().Value(util.SessionID); v != nil {

need this ..

@MyonKeminta PTAL

[tiancaiamao]

I have updated the test, you can see the _test.go file changes in this PR

[lysu]

it seems the previous

tidb/store/tikv/region_request.go

Line 465 in 18cbfaa

inject = false

will be used in some test case that out of tidb repo
https://github.com/pingcap/automated-tests/blob/cc44a56fd59cfdb71eaf37210e0861ba7acdc925/ticases/transaction/asynccommit/linearizability.go#L445

I'm not sure whether it works 😞

[cfzjywxk]

Seems we may change the session id related logic too or abstract a isInnerExecution accordingly.

[MyonKeminta]

Yes that's a problem. Some failpoints that's used by tests outside the repo checks the session id and works only when session id != 0. These kind of usages also occurs in 2pc.go, prewrite.go, etc.

[ti-chi-bot]

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

[tiancaiamao]

Close for now, it's not a very easy fix.