session, sessionctx: Make sysvar tidb_read_staleness TypeInt #32794

TszKitLo40 · 2022-03-03T07:21:10Z

What problem does this PR solve?

Issue Number: close #32763

Problem Summary:

What is changed and how it works?

Make sysvar tidb_read_staleness TypeInt

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2022-03-03T07:21:11Z

[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 submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

TszKitLo40 · 2022-03-03T07:21:37Z

/cc @morgo

TszKitLo40 · 2022-03-03T09:48:11Z

session/session_test.go

@@ -5828,7 +5828,8 @@ func (s *testSessionSuite) TestTiDBReadStaleness(c *C) {
 	c.Assert(err, NotNil)
 	err = tk.ExecToErr("set @@tidb_read_staleness='foo'")
 	c.Assert(err, NotNil)
-	tk.MustExec("set @@tidb_read_staleness=''")
+	err = tk.ExecToErr("set @@tidb_read_staleness='100'")


This case will fail in CI, I think it should be checked in sysvar.go.

If this only supports negative values, we should test one of those too.

@morgo But why set @@tidb_read_staleness='100' can succeed now? Because the MaxValue is 0, shouldn't the validation will automatically be performed?

Yes it should. We don't have any other examples where the range is been -val and 0 though, maybe there is a bug?

My point was that we should test that -100 is tested, as an example of an expected valid value.

The minValue and maxValue is checked in checkInt64SystemVar, but it won't throw an error, only the warning is appended and the value will be set to minValue or maxValue if needed.

Ahh yes! The MySQL behavior is to autoconvert to the closest permitted value + raise a warning. This is the case since #25686 It is only wrong typed values that will error.

If you modify the test to read back the value, it should be zero?

I think yes. Do yo think we need to check it in the test?

Any modification is needed for this PR? @morgo

djshow832

Is it backward compatible? When a user sets the value in an older version, and then upgrades to this version, what will happen?

TszKitLo40 · 2022-03-03T13:37:22Z

set @@tidb_read_staleness=''

I think the affected statement is set @@tidb_read_staleness='', because the type of tidb_read_staleness should be int.

sessionctx/variable/sysvar.go

morgo · 2022-03-03T14:19:25Z

session/session_test.go

@@ -5828,7 +5828,8 @@ func (s *testSessionSuite) TestTiDBReadStaleness(c *C) {
 	c.Assert(err, NotNil)
 	err = tk.ExecToErr("set @@tidb_read_staleness='foo'")
 	c.Assert(err, NotNil)
-	tk.MustExec("set @@tidb_read_staleness=''")
+	err = tk.ExecToErr("set @@tidb_read_staleness='100'")


If this only supports negative values, we should test one of those too.

morgo · 2022-03-03T14:27:27Z

Is it backward compatible? When a user sets the value in an older version, and then upgrades to this version, what will happen?

The variable is only session scoped, so we never persist the meta data. But I provided a comment on how to still accept empty if applications do this.

You are right though, that this is still a concern for variables that include global scope. I have a PR @ #31583 - looking for your review :-)

morgo · 2022-03-04T17:00:03Z

/run-unit-test

sre-bot · 2022-03-04T17:22:16Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/97be7b807080e8c02a660fe077e4bdb161755c15

TszKitLo40 · 2022-03-21T01:39:57Z

@morgo Can anyone help me review and merge this PR?

TszKitLo40 · 2022-04-06T02:44:31Z

/cc @xhebox

xhebox · 2022-04-06T04:38:50Z

/hold
@TszKitLo40 It seems check_dev fails?

xhebox · 2022-04-06T06:34:09Z

/unhold
/merge

ti-chi-bot · 2022-04-06T06:34:12Z

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

Commit hash: abaa9a2

TszKitLo40 · 2022-04-06T06:38:15Z

I think it has been fixed. @xhebox

session, sessionctx: Make sysvar tidb_read_staleness TypeInt

ea35b45

TszKitLo40 requested a review from a team as a code owner March 3, 2022 07:21

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 3, 2022

ti-chi-bot requested a review from morgo March 3, 2022 07:21

fix test

202b9b8

ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 3, 2022

fix test

428bc4c

TszKitLo40 commented Mar 3, 2022

View reviewed changes

djshow832 reviewed Mar 3, 2022

View reviewed changes

morgo reviewed Mar 3, 2022

View reviewed changes

TszKitLo40 added 2 commits March 4, 2022 14:58

Address review comments

cde4356

Address review comments

11e1d4e

ti-chi-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 4, 2022

TszKitLo40 added 2 commits March 4, 2022 15:38

Add test

39c9f69

Merge branch 'master' into issue32763

211859a

Merge branch 'master' into issue32763

37df210

morgo self-requested a review March 9, 2022 03:48

morgo approved these changes Mar 9, 2022

View reviewed changes

ti-chi-bot added status/LGT1 Indicates that a PR has LGTM 1. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 9, 2022

Merge branch 'master' into issue32763

5ede582

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2022

nitpick

1c4d3d3

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2022

Merge branch 'master' into issue32763

4c0395e

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 17, 2022

gofmt

a88b4d4

morgo requested a review from djshow832 March 21, 2022 02:25

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2022

Merge branch 'master' into issue32763

1b3dfef

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2022

ti-chi-bot requested a review from xhebox April 6, 2022 02:44

xhebox approved these changes Apr 6, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 6, 2022

Merge branch 'master' into issue32763

3db7c2f

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2022

fix format

abaa9a2

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 6, 2022

Merge branch 'master' into issue32763

c27aae4

Merge branch 'master' into issue32763

97be7b8

ti-chi-bot merged commit b37b3b1 into pingcap:master Apr 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

session, sessionctx: Make sysvar tidb_read_staleness TypeInt #32794

session, sessionctx: Make sysvar tidb_read_staleness TypeInt #32794

TszKitLo40 commented Mar 3, 2022 •

edited

Loading

ti-chi-bot commented Mar 3, 2022 •

edited

Loading

TszKitLo40 commented Mar 3, 2022

TszKitLo40 Mar 3, 2022

morgo Mar 3, 2022

TszKitLo40 Mar 4, 2022

morgo Mar 4, 2022

TszKitLo40 Mar 4, 2022 •

edited

Loading

morgo Mar 4, 2022

TszKitLo40 Mar 5, 2022 •

edited

Loading

TszKitLo40 Mar 9, 2022

djshow832 left a comment

TszKitLo40 commented Mar 3, 2022

morgo Mar 3, 2022

morgo commented Mar 3, 2022

morgo commented Mar 4, 2022

sre-bot commented Mar 4, 2022 •

edited

Loading

TszKitLo40 commented Mar 21, 2022

TszKitLo40 commented Apr 6, 2022

xhebox commented Apr 6, 2022

xhebox commented Apr 6, 2022

ti-chi-bot commented Apr 6, 2022

TszKitLo40 commented Apr 6, 2022

session, sessionctx: Make sysvar tidb_read_staleness TypeInt #32794

session, sessionctx: Make sysvar tidb_read_staleness TypeInt #32794

Conversation

TszKitLo40 commented Mar 3, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 3, 2022 • edited Loading

TszKitLo40 commented Mar 3, 2022

TszKitLo40 Mar 3, 2022

Choose a reason for hiding this comment

morgo Mar 3, 2022

Choose a reason for hiding this comment

TszKitLo40 Mar 4, 2022

Choose a reason for hiding this comment

morgo Mar 4, 2022

Choose a reason for hiding this comment

TszKitLo40 Mar 4, 2022 • edited Loading

Choose a reason for hiding this comment

morgo Mar 4, 2022

Choose a reason for hiding this comment

TszKitLo40 Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

TszKitLo40 Mar 9, 2022

Choose a reason for hiding this comment

djshow832 left a comment

Choose a reason for hiding this comment

TszKitLo40 commented Mar 3, 2022

morgo Mar 3, 2022

Choose a reason for hiding this comment

morgo commented Mar 3, 2022

morgo commented Mar 4, 2022

sre-bot commented Mar 4, 2022 • edited Loading

TszKitLo40 commented Mar 21, 2022

TszKitLo40 commented Apr 6, 2022

xhebox commented Apr 6, 2022

xhebox commented Apr 6, 2022

ti-chi-bot commented Apr 6, 2022

TszKitLo40 commented Apr 6, 2022

TszKitLo40 commented Mar 3, 2022 •

edited

Loading

ti-chi-bot commented Mar 3, 2022 •

edited

Loading

TszKitLo40 Mar 4, 2022 •

edited

Loading

TszKitLo40 Mar 5, 2022 •

edited

Loading

sre-bot commented Mar 4, 2022 •

edited

Loading