executor, session, sessionctx/variable: add validate function for sysval sql_auto_is_null #28247

unconsolable · 2021-09-22T04:30:34Z

What problem does this PR solve?

Issue Number: close #28230

What is changed and how it works?

What's Changed:

Add validate function checkSQLAutoIsNull
return error ErrValueNotSupportedWhen when set tidb_enable_noop_functions = 0 while checkSQLAutoIsNull == 1
Set tidb_enable_noop_functions to 1 in test cases which changes checkSQLAutoIsNull to 1
Add validate function test cases in executor/set_test.go

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

set sql_auto_is_null to 1 will error as follows when tidb_enable_noop_functions is 0
ERROR 1235 (42000): function sql_auto_is_null has only noop implementation in tidb now, use tidb_enable_noop_functions to enable these functions

Signed-off-by: unconsolable <chenzhipeng2012@gmail.com>

ti-chi-bot · 2021-09-22T04:30:35Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

djshow832
morgo

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.

Signed-off-by: unconsolable <chenzhipeng2012@gmail.com>

unconsolable · 2021-09-22T05:03:15Z

/cc @morgo
PTAL

morgo · 2021-09-22T16:46:00Z

executor/set_test.go

@@ -1408,6 +1408,34 @@ func (s *testSuite5) TestSetClusterConfigJSONData(c *C) {
 	}
 }

+func (s *testSuite5) TestIssue28230(c *C) {


I know there are other exceptions, but it's recommended to name the issue something descriptive and add a comment for the issue. i.e. TestSQLAutoIsNull()

morgo · 2021-09-22T16:49:48Z

executor/set_test.go

+	tk := testkit.NewTestKit(c, s.store)
+
+	err := tk.ExecToErr("set sql_auto_is_null = 1;")
+	c.Assert(terror.ErrorEqual(err, variable.ErrFunctionsNoopImpl), IsTrue, Commentf("err %v", err))
+	// change tidb_enable_noop_functions to 1, it will success
+	tk.MustExec("set tidb_enable_noop_functions = 1;")
+	tk.MustExec("set sql_auto_is_null = 1;")
+	tk.MustQuery("select @@tidb_enable_noop_functions;").Check(testkit.Rows("1"))
+	// restore tidb_enable_noop_functions to 0 failed, as sql_auto_is_null is 1
+	err = tk.ExecToErr("set tidb_enable_noop_functions = 0;")
+	c.Assert(terror.ErrorEqual(err, variable.ErrValueNotSupportedWhen), IsTrue, Commentf("err %v", err))
+	// after set sql_auto_is_null to 0, restore success
+	tk.MustExec("set sql_auto_is_null = 0;")
+	tk.MustExec("set tidb_enable_noop_functions = 0;")
+
+	// Same to global scope
+	err = tk.ExecToErr("set @@global.sql_auto_is_null = 1;")
+	c.Assert(terror.ErrorEqual(err, variable.ErrFunctionsNoopImpl), IsTrue, Commentf("err %v", err))
+	tk.MustExec("set @@global.tidb_enable_noop_functions = 1;")
+	tk.MustExec("set @@global.sql_auto_is_null = 1;")
+	tk.MustQuery("select @@global.tidb_enable_noop_functions;").Check(testkit.Rows("1"))
+	err = tk.ExecToErr("set @@global.tidb_enable_noop_functions = 0;")
+	c.Assert(terror.ErrorEqual(err, variable.ErrValueNotSupportedWhen), IsTrue, Commentf("err %v", err))
+	tk.MustExec("set @@global.sql_auto_is_null = 0;")
+	tk.MustExec("set @@global.tidb_enable_noop_functions = 0;")
+}
+


I think you can move this test to sessionctx/variable/sysvar_test.go and test it through the sysvar API (unit test). That will save you from having to make errValueNotSupportedWhen public, and increase the unit test coverage of sessionctx/variable.

morgo · 2021-09-22T16:52:52Z

sessionctx/variable/varsutil.go

+// checkSQLAutoIsNull requires TiDBEnableNoopFuncs=1 for the same scope otherwise an error will be returned.
+// See also https://github.com/pingcap/tidb/issues/28230
+func checkSQLAutoIsNull(vars *SessionVars, normalizedValue string, originalValue string, scope ScopeFlag) (string, error) {
+	feature := "sql_auto_is_null"
+	if TiDBOptOn(normalizedValue) {
+		if scope == ScopeSession {
+			if vars.EnableNoopFuncs {
+				return normalizedValue, nil
+			}
+			return Off, ErrFunctionsNoopImpl.GenWithStackByArgs(feature)
+		}
+		val, err := vars.GlobalVarsAccessor.GetGlobalSysVar(TiDBEnableNoopFuncs)
+		if err != nil {
+			return originalValue, errUnknownSystemVariable.GenWithStackByArgs(TiDBEnableNoopFuncs)
+		}
+		if !TiDBOptOn(val) {
+			return Off, ErrFunctionsNoopImpl.GenWithStackByArgs(feature)
+		}
+	}
+	return normalizedValue, nil
+}
+


Because this func is only used in one place, you might consider inlining it to sessionctx/variable/noop.go (up to you: it's also considerably longer than most other validation functions because it has to handle both session/global scope rules.

Signed-off-by: unconsolable <chenzhipeng2012@gmail.com>

morgo · 2021-09-23T04:29:28Z

LGTM. I've added the compatibility-breaker tag. It's not a commonly used feature, but we can't cherry pick this, and it needs to be clear in docs.

djshow832 · 2021-09-23T11:44:54Z

/merge

ti-chi-bot · 2021-09-23T11:44:56Z

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

Commit hash: b0e125d

ti-chi-bot · 2021-09-23T11:45:06Z

@unconsolable: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

unconsolable added 3 commits September 22, 2021 11:05

sessionctx: add validation func for checkSQLAutoIsNull

1e63336

Signed-off-by: unconsolable <chenzhipeng2012@gmail.com>

*: add test case

8653c97

Signed-off-by: unconsolable <chenzhipeng2012@gmail.com>

sessionctx: add comment

28dec33

Signed-off-by: unconsolable <chenzhipeng2012@gmail.com>

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 22, 2021

sessionctx/variable: fix lint

c2f69a4

Signed-off-by: unconsolable <chenzhipeng2012@gmail.com>

ti-chi-bot requested a review from morgo September 22, 2021 05:03

morgo reviewed Sep 22, 2021

View reviewed changes

*: move test, inline validation func, unexport error

0cec6fe

Signed-off-by: unconsolable <chenzhipeng2012@gmail.com>

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

Merge branch 'master' into issue-28230

57c942c

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

sessionctx/variable: add validate in test

b0e125d

Signed-off-by: unconsolable <chenzhipeng2012@gmail.com>

morgo self-requested a review September 23, 2021 04:27

morgo approved these changes Sep 23, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 23, 2021

morgo requested a review from a team September 23, 2021 04:27

morgo added the compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. label Sep 23, 2021

djshow832 approved these changes Sep 23, 2021

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 Sep 23, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 23, 2021

Merge branch 'master' into issue-28230

502b277

ti-chi-bot merged commit ca7ba5a into pingcap:master Sep 23, 2021

This was referenced Sep 23, 2021

system variables: update tidb_enable_noop_functions behavior pingcap/docs#6525

Merged

system variables: update tidb_enable_noop_functions behavior pingcap/docs-cn#7162

Merged

unconsolable deleted the issue-28230 branch September 23, 2021 13:09

morgo mentioned this pull request Dec 16, 2021

Able to use Update without where clause in sql_safe_updates mode. #30636

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor, session, sessionctx/variable: add validate function for sysval sql_auto_is_null #28247

executor, session, sessionctx/variable: add validate function for sysval sql_auto_is_null #28247

unconsolable commented Sep 22, 2021 •

edited by morgo

Loading

ti-chi-bot commented Sep 22, 2021 •

edited

Loading

unconsolable commented Sep 22, 2021

morgo Sep 22, 2021

morgo Sep 22, 2021

morgo Sep 22, 2021

morgo commented Sep 23, 2021

djshow832 commented Sep 23, 2021

ti-chi-bot commented Sep 23, 2021

ti-chi-bot commented Sep 23, 2021

executor, session, sessionctx/variable: add validate function for sysval sql_auto_is_null #28247

executor, session, sessionctx/variable: add validate function for sysval sql_auto_is_null #28247

Conversation

unconsolable commented Sep 22, 2021 • edited by morgo Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 22, 2021 • edited Loading

unconsolable commented Sep 22, 2021

morgo Sep 22, 2021

Choose a reason for hiding this comment

morgo Sep 22, 2021

Choose a reason for hiding this comment

morgo Sep 22, 2021

Choose a reason for hiding this comment

morgo commented Sep 23, 2021

djshow832 commented Sep 23, 2021

ti-chi-bot commented Sep 23, 2021

ti-chi-bot commented Sep 23, 2021

unconsolable commented Sep 22, 2021 •

edited by morgo

Loading

ti-chi-bot commented Sep 22, 2021 •

edited

Loading