sessionctx/binloginfo: fix uncomment pre_split_regions ddl-querys in binlog #11762

crazycs520 · 2019-08-17T01:47:44Z

What problem does this PR solve?

When tidb write binlog query, TiDB should comment some special syntax for compatibility. eg:
SHARD_ROW_ID_BITS, PRE_SPLIT_REGIONS.

But before this PR, TiDB won't comment PRE_SPLIT_REGIONS, This PR use to fix this problem.

Before :

create table t1 (id int ) shard_row_id_bits=2 pre_split_regions=2;

// the write binlog query is, and mysql execute below sql will got error.
create table t1 (id int ) /*!90000 shard_row_id_bits=2 */ pre_split_regions=2 ;

This PR

create table t1 (id int ) shard_row_id_bits=2 pre_split_regions=2;
// the write binlog query is:
create table t1 (id int ) /*!90000 shard_row_id_bits=2 pre_split_regions=2 */;

What is changed and how it works?

Check List

Tests

Unit test

Code changes

Has exported function/method change

Side effects

Related changes

Need to cherry-pick to the release branch

Release note

Fix the issue that the TiDB special syntax pre_split_regions was uncommented in create table statement when write binlog query. This may be cause downstream database error.

…binlog

sre-bot · 2019-08-17T01:47:51Z

Thanks for your PR.
This PR will be closed by bot because you already had 2 opened PRs, close or merge them before submitting a new one.
#11702 , #11721

crazycs520 · 2019-08-17T01:48:49Z

/run-all-tests

codecov · 2019-08-17T01:53:30Z

Codecov Report

Merging #11762 into master will decrease coverage by 0.029%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #11762        +/-   ##
================================================
- Coverage   81.4796%   81.4506%   -0.0291%     
================================================
  Files           434        434                
  Lines         93627      93475       -152     
================================================
- Hits          76287      76136       -151     
- Misses        11880      11888         +8     
+ Partials       5460       5451         -9

sessionctx/binloginfo/binloginfo.go

crazycs520 · 2019-08-17T03:14:54Z

/run-all-tests

sessionctx/binloginfo/binloginfo.go

bb7133 · 2019-08-17T04:06:39Z

sessionctx/binloginfo/binloginfo.go

 	if strings.Contains(ddlQuery, specialPrefix) {
 		return ddlQuery
 	}
-	loc := shardPat.FindStringIndex(strings.ToUpper(ddlQuery))
-	if loc == nil {
+	ddlQuery = addSpecialCommentByRegexp(ddlQuery, shardPat)


Rather than adding special comments, then combining them by extracting in regexp pattern, how about firstly storing all them in some struct, say, a slice and then generating the final comment at once?

The latter approach has 2 advantages:

It has better performance with less regexp matching.

If we have more special comments in the future, they will be easy to handle

AilinKid · 2019-08-17T09:19:02Z

sessionctx/binloginfo/binloginfo.go

+	upperQuery := strings.ToUpper(ddlQuery)
+	var specialComments []string
+	minIdx := math.MaxInt64
+	for _, reg := range regs {


For SQL : create table t6 (id int ) shard_row_id_bits=2 shard_row_id_bits=2 pre_split_regions=2;
If it comes with the same Regexps in SQL, will the comments duplicate here?
Should do the check and test.

Good catch. Done.

AilinKid · 2019-08-17T09:21:57Z

/Rest LGTM
By the way, @bb7133 's comments make sense.

crazycs520 · 2019-08-17T10:23:54Z

/rebuild

crazycs520 · 2019-08-17T10:36:37Z

/run-all-tests

crazycs520 · 2019-08-17T11:13:35Z

/run-all-tests

bb7133 · 2019-08-18T09:10:04Z

sessionctx/binloginfo/binloginfo.go

-var shardPat = regexp.MustCompile(`SHARD_ROW_ID_BITS\s*=\s*\d+`)
+var shardPat = regexp.MustCompile(`SHARD_ROW_ID_BITS\s*=\s*\d+\s*`)
+var preSplitPat = regexp.MustCompile(`PRE_SPLIT_REGIONS\s*=\s*\d+\s*`)
+var redundantCommentPat = regexp.MustCompile(` \*\/\s*\/\*!90000`)


Now this variable is redudant

done. thanks

bb7133 · 2019-08-19T01:47:33Z

hi @crazycs520 , CI failed with the following message:

FAIL: index_change_test.go:54: testIndexChangeSuite.TestIndexChange

index_change_test.go:126:
    c.Check(errors.ErrorStack(checkErr), Equals, "")
... obtained string = "" +
...     "job's row count 0 != 3\n" +
...     "github.com/pingcap/tidb/ddl.(*testIndexChangeSuite).TestIndexChange.func1\n" +
...     "\t/go/src/github.com/pingcap/tidb/ddl/index_change_test.go:112\n" +
...     "github.com/pingcap/tidb/ddl.(*TestDDLCallback).OnJobUpdated\n" +
...     "\t/go/src/github.com/pingcap/tidb/ddl/callback_test.go:72\n" +
...     "github.com/pingcap/tidb/ddl.(*worker).handleDDLJobQueue\n" +
...     "\t/go/src/github.com/pingcap/tidb/ddl/ddl_worker.go:443\n" +
...     "github.com/pingcap/tidb/ddl.(*worker).start\n" +
...     "\t/go/src/github.com/pingcap/tidb/ddl/ddl_worker.go:139\n" +
...     "github.com/pingcap/tidb/ddl.(*ddl).start.func1\n" +
...     "\t/go/src/github.com/pingcap/tidb/ddl/ddl.go:440\n" +
...     "github.com/pingcap/tidb/util.WithRecovery\n" +
...     "\t/go/src/github.com/pingcap/tidb/util/misc.go:81\n" +
...     "runtime.goexit\n" +
...     "\t/usr/local/go/src/runtime/asm_amd64.s:1337"
... expected string = ""

index_change_test.go:171:
    c.Check(errors.ErrorStack(checkErr), Equals, "")
... obtained string = "" +
...     "job's row count 0 != 3\n" +
...     "github.com/pingcap/tidb/ddl.(*testIndexChangeSuite).TestIndexChange.func1\n" +
...     "\t/go/src/github.com/pingcap/tidb/ddl/index_change_test.go:112\n" +
...     "github.com/pingcap/tidb/ddl.(*TestDDLCallback).OnJobUpdated\n" +
...     "\t/go/src/github.com/pingcap/tidb/ddl/callback_test.go:72\n" +
...     "github.com/pingcap/tidb/ddl.(*worker).handleDDLJobQueue\n" +
...     "\t/go/src/github.com/pingcap/tidb/ddl/ddl_worker.go:443\n" +
...     "github.com/pingcap/tidb/ddl.(*worker).start\n" +
...     "\t/go/src/github.com/pingcap/tidb/ddl/ddl_worker.go:139\n" +
...     "github.com/pingcap/tidb/ddl.(*ddl).start.func1\n" +
...     "\t/go/src/github.com/pingcap/tidb/ddl/ddl.go:440\n" +
...     "github.com/pingcap/tidb/util.WithRecovery\n" +
...     "\t/go/src/github.com/pingcap/tidb/util/misc.go:81\n" +
...     "runtime.goexit\n" +
...     "\t/usr/local/go/src/runtime/asm_amd64.s:1337"
... expected string = ""

PTAL, thanks

crazycs520 · 2019-08-19T05:20:05Z

/run-all-tests

bb7133

LGTM

winkyao · 2019-08-19T08:37:28Z

sessionctx/binloginfo/binloginfo.go

+	return addSpecialCommentByRegexps(ddlQuery, shardPat, preSplitPat)
+}
+
+func addSpecialCommentByRegexps(ddlQuery string, regs ...*regexp.Regexp) string {


Please add some comments for this function.

tangenta

The code is write-only..
LGTM

AilinKid

LGTM

crazycs520 · 2019-08-19T14:46:19Z

/run-all-tests

…binlog (pingcap#11762)

…binlog (#11762) (#12121)

…binlog (#11762) (#12120)

sessionctx/binloginfo: fix uncomment pre_split_regions ddl querys in …

1718d9b

…binlog

sre-bot added the closed/no-pr-quota label Aug 17, 2019

crazycs520 added the component/DDL-need-LGT3 label Aug 17, 2019

sre-bot closed this Aug 17, 2019

crazycs520 added the component/binlog label Aug 17, 2019

crazycs520 reopened this Aug 17, 2019

bb7133 reviewed Aug 17, 2019

View reviewed changes

sessionctx/binloginfo/binloginfo.go Outdated Show resolved Hide resolved

sessionctx/binloginfo/binloginfo.go Outdated Show resolved Hide resolved

sessionctx/binloginfo/binloginfo.go Outdated Show resolved Hide resolved

address comment

a0ca779

bb7133 reviewed Aug 17, 2019

View reviewed changes

sessionctx/binloginfo/binloginfo.go Outdated Show resolved Hide resolved

bb7133 reviewed Aug 17, 2019

View reviewed changes

crazycs520 added 3 commits August 17, 2019 16:59

combine comment

41da1a4

use slice

eca81e5

remove redundant code

bf7746a

AilinKid reviewed Aug 17, 2019

View reviewed changes

crazycs520 added 2 commits August 17, 2019 17:24

fix test

dcc7809

address comment

4548b56

bb7133 reviewed Aug 18, 2019

View reviewed changes

crazycs520 and others added 4 commits August 19, 2019 10:38

Merge branch 'master' into fix-pre-split-bin-log

a4edad8

address comment to remove redundant code

eecf981

Merge branch 'master' into fix-pre-split-bin-log

3493380

Merge branch 'master' into fix-pre-split-bin-log

7fdee22

bb7133 reviewed Aug 19, 2019

View reviewed changes

Merge branch 'master' into fix-pre-split-bin-log

dca1909

winkyao reviewed Aug 19, 2019

View reviewed changes

tangenta reviewed Aug 19, 2019

View reviewed changes

AilinKid reviewed Aug 19, 2019

View reviewed changes

Merge branch 'master' into fix-pre-split-bin-log

54fabd1

crazycs520 added the status/LGT3 The PR has already had 3 LGTM. label Aug 19, 2019

add comment

fbb4e64

winkyao approved these changes Aug 19, 2019

View reviewed changes

winkyao merged commit a1411ba into pingcap:master Aug 19, 2019

crazycs520 added a commit to crazycs520/tidb that referenced this pull request Sep 10, 2019

sessionctx/binloginfo: fix uncomment pre_split_regions ddl-querys in …

da5eafe

…binlog (pingcap#11762)

crazycs520 mentioned this pull request Sep 10, 2019

sessionctx/binloginfo: fix uncomment pre_split_regions ddl-querys in binlog (#11762) #12120

Merged

crazycs520 added a commit to crazycs520/tidb that referenced this pull request Sep 10, 2019

sessionctx/binloginfo: fix uncomment pre_split_regions ddl-querys in …

4d898ae

…binlog (pingcap#11762)

crazycs520 mentioned this pull request Sep 10, 2019

sessionctx/binloginfo: fix uncomment pre_split_regions ddl-querys in binlog (#11762) #12121

Merged

ngaut pushed a commit that referenced this pull request Sep 10, 2019

sessionctx/binloginfo: fix uncomment pre_split_regions ddl-querys in …

52874bf

…binlog (#11762) (#12121)

ngaut pushed a commit that referenced this pull request Sep 10, 2019

sessionctx/binloginfo: fix uncomment pre_split_regions ddl-querys in …

cdf4566

…binlog (#11762) (#12120)

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sessionctx/binloginfo: fix uncomment pre_split_regions ddl-querys in binlog #11762

sessionctx/binloginfo: fix uncomment pre_split_regions ddl-querys in binlog #11762

crazycs520 commented Aug 17, 2019 •

edited

Loading

sre-bot commented Aug 17, 2019

crazycs520 commented Aug 17, 2019

codecov bot commented Aug 17, 2019 •

edited

Loading

crazycs520 commented Aug 17, 2019

bb7133 Aug 17, 2019

AilinKid Aug 17, 2019

crazycs520 Aug 17, 2019

AilinKid commented Aug 17, 2019

crazycs520 commented Aug 17, 2019

crazycs520 commented Aug 17, 2019

crazycs520 commented Aug 17, 2019

bb7133 Aug 18, 2019

crazycs520 Aug 19, 2019

bb7133 commented Aug 19, 2019

crazycs520 commented Aug 19, 2019

bb7133 left a comment

winkyao Aug 19, 2019

tangenta left a comment

AilinKid left a comment

crazycs520 commented Aug 19, 2019

sessionctx/binloginfo: fix uncomment pre_split_regions ddl-querys in binlog #11762

sessionctx/binloginfo: fix uncomment pre_split_regions ddl-querys in binlog #11762

Conversation

crazycs520 commented Aug 17, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

sre-bot commented Aug 17, 2019

crazycs520 commented Aug 17, 2019

codecov bot commented Aug 17, 2019 • edited Loading

Codecov Report

crazycs520 commented Aug 17, 2019

bb7133 Aug 17, 2019

Choose a reason for hiding this comment

AilinKid Aug 17, 2019

Choose a reason for hiding this comment

crazycs520 Aug 17, 2019

Choose a reason for hiding this comment

AilinKid commented Aug 17, 2019

crazycs520 commented Aug 17, 2019

crazycs520 commented Aug 17, 2019

crazycs520 commented Aug 17, 2019

bb7133 Aug 18, 2019

Choose a reason for hiding this comment

crazycs520 Aug 19, 2019

Choose a reason for hiding this comment

bb7133 commented Aug 19, 2019

crazycs520 commented Aug 19, 2019

bb7133 left a comment

Choose a reason for hiding this comment

winkyao Aug 19, 2019

Choose a reason for hiding this comment

tangenta left a comment

Choose a reason for hiding this comment

AilinKid left a comment

Choose a reason for hiding this comment

crazycs520 commented Aug 19, 2019

crazycs520 commented Aug 17, 2019 •

edited

Loading

codecov bot commented Aug 17, 2019 •

edited

Loading