*: add tidb_enable_noop_functions to default disable get_lock()/release_lock() #10987

cfzjywxk · 2019-06-28T15:03:14Z

What problem does this PR solve?

This adds the variable tidb_enable_noop_functions to control usage of NOOP funcs in tidb such as get_lock, release_lock (always return 1), since the current behavior could be dangerous for apps relying on these functions working correctly.

The default for tidb_enable_noop_functions is false, thus users must manually set session/global to true to have unsafe behavior.

What is changed and how it works?

add global/session variable "tidb_enable_noop_functions" to control usage of noop functions,
default these usage will report "function xxx has only noop implementation in tidb now, use tidb_enable_noop_functions to enable these functions"". while set true, these functions can be
used like before

Check List

Tests

Unit test
Integration test

Code changes

Has newly added system variable

Side effects

Breaking backward compatibility
applications using former version of tidbs have to set "tidb_enable_noop_functions" to true to meet their needs

Related changes

Need to update the documentation
Need to be included in the release note

cfzjywxk · 2019-06-28T15:05:03Z

/run-all-tests

expression/builtin.go

CLAassistant · 2019-06-28T15:18:33Z

All committers have signed the CLA.

zz-jason · 2019-06-28T15:27:53Z

please follow the contributing guide to refactor the PR title: https://github.com/pingcap/community/blob/master/commit-message-pr-style.md

codecov · 2019-06-28T15:50:44Z

Codecov Report

Merging #10987 into master will decrease coverage by 0.514%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #10987        +/-   ##
================================================
- Coverage   81.6366%   81.1225%   -0.5141%     
================================================
  Files           420        420                
  Lines         91726      89472      -2254     
================================================
- Hits          74882      72582      -2300     
- Misses        11563      11636        +73     
+ Partials       5281       5254        -27

cfzjywxk · 2019-06-28T16:12:47Z

@zz-jason PTAL

morgo · 2019-06-28T17:42:42Z

@cfzjywxk I think a better behavior instead of "function not found" would be to suggest the user can change the setting to have syntax-only support. With the current behavior I don't believe many users will discover that such a setting exists (see #10065 for an example error).

I also prefer a name like tidb_skip_errors_for_unsupported_function (maybe this is a bit long, @kolbe do you have any other ideas?)

cc @pingcap/usability-team

kolbe · 2019-06-28T20:32:33Z

I don't like tidb_use_fake_funcs very much. Having abbreviations in variable names is confusing, and I think using "fake" doesn't convey the right idea. But I also think tidb_skip_errors_for_unsupported_function is much too long. Since these are noop functions, I suggest we call the option tidb_noop_functions or tidb_enable_noop_functions or something along those lines.

morgo · 2019-06-28T20:34:04Z

tidb_enable_noop_functions

+1 for this name

cfzjywxk · 2019-06-30T07:22:33Z

@morgo PTAL

morgo · 2019-06-30T15:03:53Z

LGTM

executor/set_test.go

lysu

LGTM

lysu · 2019-07-01T12:02:13Z

/run-all-tests

executor/set_test.go

cfzjywxk · 2019-07-01T13:37:52Z

/run-all-tests

…release_lock functions

cfzjywxk · 2019-07-05T02:59:40Z

/run-all-tests

cfzjywxk · 2019-07-05T03:12:23Z

/run-all-tests

lysu · 2019-07-05T03:56:46Z

/run-all-tests

zz-jason · 2019-07-08T05:08:47Z

store/tikv/client_fail_test.go

@@ -50,6 +50,7 @@ func (s *testClientSuite) TestPanicInRecvLoop(c *C) {
 	time.Sleep(time.Second)
 	c.Assert(failpoint.Disable("github.com/pingcap/tidb/store/tikv/panicInFailPendingRequests"), IsNil)
 	c.Assert(failpoint.Disable("github.com/pingcap/tidb/store/tikv/gotErrorInRecvLoop"), IsNil)
+	time.Sleep(time.Second)


why adding a sleep?

@zz-jason new case "client_fail_test" not stable may fail running "make test", like:

FAIL: client_fail_test.go:34: testClientSuite.TestPanicInRecvLoop

client_fail_test.go:59:
c.Assert(err, IsNil)
... value *errors.fundamental = injected error in batchRecvLoop ("injected error in batchRecvLoop")

TiDB 3.0 may also have this problem. /cc @hicqu

cfzjywxk · 2019-07-08T05:15:46Z

/run-all-tests

zz-jason

LGTM

cfzjywxk requested review from lysu, jackysp, XuHuaiyu and crazycs520 June 28, 2019 15:03

cfzjywxk added component/session status/WIP labels Jun 28, 2019

zz-jason reviewed Jun 28, 2019

View reviewed changes

expression/builtin.go Outdated Show resolved Hide resolved

cfzjywxk force-pushed the p4100 branch from 10113b7 to e0a52b1 Compare June 28, 2019 15:43

cfzjywxk changed the title ~~issue#4100 add new variable to default disable usage of get_lock and …~~ session, sysvar: issue#4100 add new variable to default disable usage of get_lock and … Jun 28, 2019

morgo changed the title ~~session, sysvar: issue#4100 add new variable to default disable usage of get_lock and …~~ session, sysvar: add new variable to default disable usage of get_lock and … Jun 28, 2019

cfzjywxk force-pushed the p4100 branch from e0a52b1 to 5e4aa21 Compare June 30, 2019 07:17

jackysp reviewed Jul 1, 2019

View reviewed changes

executor/set_test.go Outdated Show resolved Hide resolved

cfzjywxk force-pushed the p4100 branch from 5e4aa21 to 07394e1 Compare July 1, 2019 07:40

lysu reviewed Jul 1, 2019

View reviewed changes

lysu added status/LGT1 Indicates that a PR has LGTM 1. and removed status/WIP labels Jul 1, 2019

zz-jason reviewed Jul 1, 2019

View reviewed changes

executor/set_test.go Show resolved Hide resolved

zz-jason changed the title ~~session, sysvar: add new variable to default disable usage of get_lock and …~~ *: add tidb_enable_noop_functions to default disable get_lock()/release_lock() Jul 1, 2019

issue#4100 add new variable to default disable usage of get_lock and …

38fa026

…release_lock functions

cfzjywxk force-pushed the p4100 branch from fe9c454 to 38fa026 Compare July 1, 2019 14:00

Merge branch 'master' into p4100

dc2f890

Merge branch 'master' into p4100

dde6efe

cfzjywxk added 4 commits July 8, 2019 11:04

Merge branch 'master' into p4100

8e21627

merge confilct format

e1585db

merge conficts format

1e4809f

make other tests stable

e4f20e5

zz-jason reviewed Jul 8, 2019

View reviewed changes

zz-jason approved these changes Jul 8, 2019

View reviewed changes

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 8, 2019

cfzjywxk merged commit a737d26 into pingcap:master Jul 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: add tidb_enable_noop_functions to default disable get_lock()/release_lock() #10987

*: add tidb_enable_noop_functions to default disable get_lock()/release_lock() #10987

cfzjywxk commented Jun 28, 2019 •

edited

Loading

cfzjywxk commented Jun 28, 2019

CLAassistant commented Jun 28, 2019 •

edited

Loading

zz-jason commented Jun 28, 2019

codecov bot commented Jun 28, 2019 •

edited

Loading

cfzjywxk commented Jun 28, 2019

morgo commented Jun 28, 2019

kolbe commented Jun 28, 2019

morgo commented Jun 28, 2019

cfzjywxk commented Jun 30, 2019

morgo commented Jun 30, 2019

lysu left a comment

lysu commented Jul 1, 2019

cfzjywxk commented Jul 1, 2019

cfzjywxk commented Jul 5, 2019

cfzjywxk commented Jul 5, 2019

lysu commented Jul 5, 2019

zz-jason Jul 8, 2019

cfzjywxk Jul 8, 2019

zz-jason Jul 8, 2019

cfzjywxk commented Jul 8, 2019

zz-jason left a comment

*: add tidb_enable_noop_functions to default disable get_lock()/release_lock() #10987

*: add tidb_enable_noop_functions to default disable get_lock()/release_lock() #10987

Conversation

cfzjywxk commented Jun 28, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

cfzjywxk commented Jun 28, 2019

CLAassistant commented Jun 28, 2019 • edited Loading

zz-jason commented Jun 28, 2019

codecov bot commented Jun 28, 2019 • edited Loading

Codecov Report

cfzjywxk commented Jun 28, 2019

morgo commented Jun 28, 2019

kolbe commented Jun 28, 2019

morgo commented Jun 28, 2019

cfzjywxk commented Jun 30, 2019

morgo commented Jun 30, 2019

lysu left a comment

Choose a reason for hiding this comment

lysu commented Jul 1, 2019

cfzjywxk commented Jul 1, 2019

cfzjywxk commented Jul 5, 2019

cfzjywxk commented Jul 5, 2019

lysu commented Jul 5, 2019

zz-jason Jul 8, 2019

Choose a reason for hiding this comment

cfzjywxk Jul 8, 2019

Choose a reason for hiding this comment

zz-jason Jul 8, 2019

Choose a reason for hiding this comment

cfzjywxk commented Jul 8, 2019

zz-jason left a comment

Choose a reason for hiding this comment

cfzjywxk commented Jun 28, 2019 •

edited

Loading

CLAassistant commented Jun 28, 2019 •

edited

Loading

codecov bot commented Jun 28, 2019 •

edited

Loading