-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*: refine mysql.tidb_mdl_view #48728
Conversation
Signed-off-by: wjhuang2016 <[email protected]>
pkg/sessionctx/variable/session.go
Outdated
@@ -2084,6 +2084,7 @@ func NewSessionVars(hctx HookContext) *SessionVars { | |||
if EnableRowLevelChecksum.Load() { | |||
vars.EnableRowLevelChecksum = true | |||
} | |||
vars.systems[MaxAllowedPacket] = strconv.Itoa(int(DefMaxAllowedPacket)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that we can use concat in UT
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #48728 +/- ##
=================================================
- Coverage 79.3015% 67.5877% -11.7139%
=================================================
Files 2447 2568 +121
Lines 673721 846719 +172998
=================================================
+ Hits 534271 572278 +38007
- Misses 118049 250789 +132740
- Partials 21401 23652 +2251
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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. |
Signed-off-by: wjhuang2016 <[email protected]>
Signed-off-by: wjhuang2016 <[email protected]>
Signed-off-by: wjhuang2016 <[email protected]>
Signed-off-by: wjhuang2016 <[email protected]>
Signed-off-by: wjhuang2016 <[email protected]>
Signed-off-by: wjhuang2016 <[email protected]>
Signed-off-by: wjhuang2016 <[email protected]>
Signed-off-by: wjhuang2016 <[email protected]>
/retest |
/test tiprow_fast_test |
@hawkingrei: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
Signed-off-by: wjhuang2016 <[email protected]>
/retest |
tk.MustExec(c.createTable) | ||
for _, cr := range c.createTable { | ||
if strings.Contains(c.name, "err") { | ||
_, _ = tk.Exec(cr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check it does return err?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all the SQLs return an error, I don't think it's necessary to check the error since it is just a preparation.
// But we need to init max_allowed_packet to use concat function during bootstrap or upgrade. | ||
err := vars.SetSystemVar(variable.MaxAllowedPacket, strconv.FormatUint(variable.DefMaxAllowedPacket, 10)) | ||
if err != nil { | ||
logutil.BgLogger().Error("set system variable max_allowed_packet error", zap.Error(err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we return err if it's required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not always required, only when we need to create or replace this view. Do you think we still need to return an error?
WHERE (ddl_jobs.state != 'synced' and ddl_jobs.state != 'cancelled') | ||
AND Find_in_set(ddl_jobs.table_id, cluster_tidb_trx.related_table_ids) | ||
AND cluster_tidb_trx.session_id = cluster_processlist.id | ||
FROM mysql.tidb_ddl_job, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TiDB versions less than 6.2 do not have this table. Does this have compatibility issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After testing, the old owner doesn't check if the table exists or not. So there it no compatibility issues for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Maybe we can create an issue.
/hold |
/unhold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for now but version rollback needs to be considered for the cases like this. @Benjamin2037
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, tangenta, yudongusa, zimulala The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
What problem does this PR solve?
Issue Number: close #50149, close #46933, close #47743
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.