ddl, executor: add warning log for lock/unlock tables #29301

Defined2014 · 2021-11-01T08:18:16Z

What problem does this PR solve?

Issue Number: close #28967

Problem Summary: Add warning log for lock tables and unlock tables when enable-table-lock flag not enabled.

What is changed and how it works?

Check List

Tests

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

Release note

None

ti-chi-bot · 2021-11-01T08:18:17Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

morgo
tangenta

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.

Defined2014 · 2021-11-01T08:18:40Z

/cc @morgo
PTAL

morgo · 2021-11-01T18:06:46Z

executor/ddl.go

 func (e *DDLExec) executeUnlockTables(_ *ast.UnlockTablesStmt) error {
 	if !config.TableLockEnabled() {
+		e.ctx.GetSessionVars().StmtCtx.AppendWarning(errors.Errorf("Unlock tables works only when enable-table-lock is set in config file"))
 		return nil
 	}


A couple of suggestions:

We make the mode dependent on https://docs.pingcap.com/tidb/dev/system-variables#tidb_enable_noop_functions-new-in-v40 -- but it only ever returns warnings at most (since it's important DDL can restore). This provides a mechanism for users to kill the warning if that is problematic for their script.

We add a new error number / error code. This helps us track the errors in information schema so we know which unsupported syntax the user is encountering.

Let me know what you think about either. These are just suggestions.

Sounds good for me.

Maybe we can use mysql.ErrNotSupportedYet with different error message between ErrFunctionsNoopImpl.

Sounds good to me.

Done，PTAL @morgo

morgo · 2021-11-02T15:07:56Z

executor/ddl.go

+		if noopFuncsMode == variable.OffInt {
+			return err
+		} else if noopFuncsMode == variable.WarnInt {
+			e.ctx.GetSessionVars().StmtCtx.AppendWarning(err)
+		}
+		return nil


Sorry, for this syntax I think we can only WARN at most. I know this is different from most other noop functions, but this syntax is used by mysqldump. If we generate an error, mysqldump files won't restore by default :(

The behavior will be very strange. If we only have WARN, I think use TableLockEnabled() to control it is better than noopFuncsMode . What's your opinion? @morgo

I agree it is strange, but the change is too severe if mysqldump can not be restored.

For the docs: https://docs.pingcap.com/tidb/dev/system-variables#tidb_enable_noop_functions-new-in-v40

We can add a sentence: Setting tidb_enable_noop_functions to OFF does not prevent LOCK TABLE t1 WRITE from executing. A warning is returned instead. This behavior is required for compatibility with mysqldump.

Updated, PTAL @morgo

morgo · 2021-11-04T03:22:50Z

executor/errors.go

@@ -64,6 +64,7 @@ var (
 	ErrNotSupportedWithSem           = dbterror.ClassOptimizer.NewStd(mysql.ErrNotSupportedWithSem)
 	ErrPluginIsNotLoaded             = dbterror.ClassExecutor.NewStd(mysql.ErrPluginIsNotLoaded)
 	ErrSetPasswordAuthPlugin         = dbterror.ClassExecutor.NewStd(mysql.ErrSetPasswordAuthPlugin)
+	ErrFuncNotEnabled                = dbterror.ClassExecutor.NewStdErr(mysql.ErrNotSupportedYet, parser_mysql.Message("%-.32s works only when %-.32s is set in config file", nil))


The warning is not clear that the behavior is a noop. This distinction is important in understanding the risks. May I suggest instead:

LOCK TABLE is not supported. To enable this experimental feature, set 'enable-table-lock' in the configuration file.

Otherwise LGTM. I think it is too confusing to say that it is also affected by tidb_enable_noop_funcs = ON, so we can handle that in docs.

Yes, it's too confusing. I think we should return warning whether noop_funcs enabled or not. Maybe controlled by enable-table-lock is enough. @morgo

Modified. @morgo

@AilinKid PTAL

Defined2014 · 2021-11-04T04:00:32Z

/cc @AilinKid

morgo

LGTM, thanks for making edits!

tangenta · 2021-11-04T06:09:08Z

/merge

ti-chi-bot · 2021-11-04T06:09:11Z

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

Commit hash: 18dd8d0

winoros · 2021-11-04T06:29:41Z

/run-cherry-picker

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2021-11-04T06:30:17Z

cherry pick to release-5.3 in PR #29437

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2021-11-04T06:31:20Z

cherry pick to release-5.1 in PR #29439

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2021-11-04T06:32:24Z

cherry pick to release-5.2 in PR #29440

close #28967

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 Nov 1, 2021

ti-chi-bot requested a review from morgo November 1, 2021 08:18

morgo reviewed Nov 1, 2021

View reviewed changes

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 2, 2021

morgo reviewed Nov 2, 2021

View reviewed changes

Defined2014 force-pushed the 28967 branch from e4450ea to 4650381 Compare November 3, 2021 07:50

ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 3, 2021

Defined2014 force-pushed the 28967 branch from d9a74da to 6571941 Compare November 4, 2021 02:30

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 4, 2021

morgo reviewed Nov 4, 2021

View reviewed changes

ti-chi-bot requested a review from AilinKid November 4, 2021 04:00

ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 4, 2021

morgo self-requested a review November 4, 2021 04:19

morgo approved these changes Nov 4, 2021

View reviewed changes

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

Defined2014 added 5 commits November 4, 2021 13:52

ddl, executor: add warning log for lock/unlock tables

c10a7a4

follow comments

e63be9f

only return warning log

a6d64fb

add error in errors.toml

01a179f

follow comments

fe442d0

Defined2014 added 2 commits November 4, 2021 13:52

follow comments

ea7692c

fix test

18dd8d0

Defined2014 force-pushed the 28967 branch from e5c1f40 to 18dd8d0 Compare November 4, 2021 05:53

tangenta approved these changes Nov 4, 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 Nov 4, 2021

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

Merge branch 'master' into 28967

478575b

AilinKid approved these changes Nov 4, 2021

View reviewed changes

ti-chi-bot merged commit dcbfda9 into pingcap:master Nov 4, 2021

Defined2014 deleted the 28967 branch November 4, 2021 06:23

winoros added needs-cherry-pick-release-5.1 needs-cherry-pick-release-5.2 needs-cherry-pick-release-5.3 Type: Need cherry pick to release-5.3 labels Nov 4, 2021

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Nov 4, 2021

cherry pick pingcap#29301 to release-5.3

38e588d

Signed-off-by: ti-srebot <[email protected]>

ti-srebot mentioned this pull request Nov 4, 2021

ddl, executor: add warning log for lock/unlock tables (#29301) #29437

Merged

4 tasks

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Nov 4, 2021

cherry pick pingcap#29301 to release-5.1

7cbd249

Signed-off-by: ti-srebot <[email protected]>

ti-srebot mentioned this pull request Nov 4, 2021

ddl, executor: add warning log for lock/unlock tables (#29301) #29439

Merged

4 tasks

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Nov 4, 2021

cherry pick pingcap#29301 to release-5.2

3a3b744

Signed-off-by: ti-srebot <[email protected]>

ti-srebot mentioned this pull request Nov 4, 2021

ddl, executor: add warning log for lock/unlock tables (#29301) #29440

Merged

4 tasks

ti-chi-bot pushed a commit that referenced this pull request Nov 9, 2021

ddl, executor: add warning log for lock/unlock tables (#29301) (#29437)

2332dcc

aytrack mentioned this pull request Nov 12, 2021

CI lock table cases failed #29717

Closed

ti-chi-bot pushed a commit that referenced this pull request Apr 12, 2022

ddl, executor: add warning log for lock/unlock tables (#29301) (#29440)

31b7654

close #28967

ti-chi-bot pushed a commit that referenced this pull request May 31, 2022

ddl, executor: add warning log for lock/unlock tables (#29301) (#29439)

ec172fe

close #28967

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl, executor: add warning log for lock/unlock tables #29301

ddl, executor: add warning log for lock/unlock tables #29301

Defined2014 commented Nov 1, 2021

ti-chi-bot commented Nov 1, 2021 •

edited

Loading

Defined2014 commented Nov 1, 2021

morgo Nov 1, 2021

Defined2014 Nov 2, 2021

morgo Nov 2, 2021

Defined2014 Nov 2, 2021

morgo Nov 2, 2021

Defined2014 Nov 3, 2021 •

edited

Loading

morgo Nov 3, 2021

Defined2014 Nov 4, 2021

morgo Nov 4, 2021 •

edited

Loading

Defined2014 Nov 4, 2021 •

edited

Loading

Defined2014 Nov 4, 2021 •

edited

Loading

Defined2014 commented Nov 4, 2021

morgo left a comment

tangenta commented Nov 4, 2021

ti-chi-bot commented Nov 4, 2021

winoros commented Nov 4, 2021

ti-srebot commented Nov 4, 2021

ti-srebot commented Nov 4, 2021

ti-srebot commented Nov 4, 2021

ddl, executor: add warning log for lock/unlock tables #29301

ddl, executor: add warning log for lock/unlock tables #29301

Conversation

Defined2014 commented Nov 1, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Nov 1, 2021 • edited Loading

Defined2014 commented Nov 1, 2021

morgo Nov 1, 2021

Choose a reason for hiding this comment

Defined2014 Nov 2, 2021

Choose a reason for hiding this comment

morgo Nov 2, 2021

Choose a reason for hiding this comment

Defined2014 Nov 2, 2021

Choose a reason for hiding this comment

morgo Nov 2, 2021

Choose a reason for hiding this comment

Defined2014 Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

morgo Nov 3, 2021

Choose a reason for hiding this comment

Defined2014 Nov 4, 2021

Choose a reason for hiding this comment

morgo Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

Defined2014 Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

Defined2014 Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

Defined2014 commented Nov 4, 2021

morgo left a comment

Choose a reason for hiding this comment

tangenta commented Nov 4, 2021

ti-chi-bot commented Nov 4, 2021

winoros commented Nov 4, 2021

ti-srebot commented Nov 4, 2021

ti-srebot commented Nov 4, 2021

ti-srebot commented Nov 4, 2021

ti-chi-bot commented Nov 1, 2021 •

edited

Loading

Defined2014 Nov 3, 2021 •

edited

Loading

morgo Nov 4, 2021 •

edited

Loading

Defined2014 Nov 4, 2021 •

edited

Loading

Defined2014 Nov 4, 2021 •

edited

Loading