session: fix global variable collation_server does not take effect in new session

[xiongjiwei]

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

after setting the global collation_server , the new session's collation_server variable should use it.

global variable collation_server does not take effect because the loadCommonGlobalVariablesIfNeeded function overwrite the variables, and we had set the collation_server when setting the character_set_server, see #21034

Check List

Tests

• Unit test

Release note

• fix global variable collation_server does not take effect in new session

[xiongjiwei]

/run-all-tests

[xiongjiwei]

/cc @pingcap/sql-infra-reviewers

[xiongjiwei]

/run-all-tests

[xhebox]

I am not sure if this is the correct fix. I mean that global variables are promised to be refreshed every 3 mins. They will not take effect immediately.

[xiongjiwei]

I am not sure if this is the correct fix. I mean that global variables are promised to be refreshed every 3 mins. They will not take effect immediately.

I have no knowledge about global variables are promised to be refreshed every 3 mins. Are there any materials?

[xhebox]

Here, I have one issue, #22808. Andy fault, there is a typo, it is 3s, or 2s, not mins.

[morgo]

/LGTM

Here, I have one issue, #22808. Andy fault, there is a typo, it is 3s, or 2s, not mins.

On a single-server it is usually much quicker, this is just the documented time to receive updates asyncronously through etcd on other servers.

We have several examples where there can be a delay (another one is FLUSH PRIVILEGES), and usually fix it by syncronously also updating it locally. So technically the initiating server will apply the instruction twice.

I think the intention is that set global should be instant on the initiating server too, but I know there is a race condition if the new session starts too quickly. I am hoping that we can fix it in future by using a simple map to keep the global cache (similar to what privileges does) and not rely on internal SQL.

Update: My comment here was incorrect. It did nto use etcd to update at the time this was written, but it does now.

[xhebox]

LGTM

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has been approved by:

• morgo
• xhebox

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.

[xhebox]

I think the intention is that set global should be instant on the initiating server too

Agreed.

I am hoping that we can fix it in future by using a simple map to keep the global cache (similar to what privileges does) and not rely on internal SQL.

Well, I don't think it is a good idea to merge a PR that is not completely working, though... Anyway, let us merge it as it is proposed. But it should be fine to revert the change back when immediate global cache is implemented, or whatever.

[morgo]

Well, I don't think it is a good idea to merge a PR that is not completely working, though...

If we do a more complete fix, I think it will be a lot of work, and not suitable to cherry pick to 5.0.

Anyway, let us merge it as it is proposed. But it should be fine to revert the change back when immediate global cache is implemented, or whatever.

Agreed. Having the unit test here also helps for this future work.

[morgo]

/merge

[ti-chi-bot]

This pull request has been accepted and is ready to merge.

Commit hash: 19ceee8

[morgo]

/run-unit-test

[xiongjiwei]

/merge

[xiongjiwei]

/run-cherry-pick

[ti-srebot]

cherry pick to release-5.0 in PR #24156