Optimistic: support start task with inconsistent upstream table schema #3903

GMHDBJD · 2021-12-16T05:35:09Z

What problem does this PR solve?

Issue Number: close #3629
close #3786
close #3708

What is changed and how it works?

worker flush table info when entering sync stage
master get table info when receiving ddl info
remove info/operation/locks/drop columns when stop a optimistic task

For DM-master:

fetch table info for every table when creating a lock rather than use joined as init table
when master restart, no need to rebuild locks base on init schema, just handle info by order
when the task stops, remove all metadata for optimism, when restarting the task, we can fetch table info from downstream instead.

For DM-worker:

get and flush table info when entering sync stage

Check List

Tests

Unit test
Integration test

Release note

support start task with inconsistent upstream table schemas in optimistic shard mode.

ti-chi-bot · 2021-12-16T05:35:10Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Ehco1996
lance6716

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.

GMHDBJD · 2021-12-16T05:35:33Z

/hold
/run-all-tests

GMHDBJD · 2021-12-16T05:40:58Z

/run-dm-integration-test
/verify

GMHDBJD · 2021-12-16T05:42:00Z

/run-all-tests

…ntSchema

GMHDBJD · 2021-12-17T02:31:46Z

/run-all-tests

…ntSchema

GMHDBJD · 2021-12-19T13:51:44Z

/run-dm-integration-test

GMHDBJD · 2021-12-19T13:59:19Z

/run-dm-integration-test

GMHDBJD · 2021-12-19T15:09:33Z

/run-dm-integration-test

GMHDBJD · 2021-12-19T15:16:35Z

/run-dm-integration-test

…ntSchema

GMHDBJD · 2021-12-20T03:15:18Z

/run-dm-integration-test

…ntSchema

GMHDBJD · 2021-12-20T05:56:26Z

/run-all-tests

lance6716 · 2022-01-26T03:28:40Z

/hold

waiting @lichunzhu review

Co-authored-by: lance6716 <lance6716@gmail.com>

lichunzhu

Will review tests later. Have two questions now:

Should we delete obsolete init-schema for dm-clusters upgraded from lower versions?
Should we flush table info for newly added tables?

lichunzhu · 2022-01-26T09:12:34Z

dm/dm/master/shardddl/optimist.go

+			// TODO: handle drop table
+			continue
+		}
+		if !o.tk.SourceTableExist(info.Task, info.Source, info.UpSchema, info.UpTable, info.DownSchema, info.DownTable) {


Is this too arbitrary? If we add some new tables while the scheduler didn't get at optimism.GetAllSourceTables, will it cause a problem here?

The logic is same as origin logic

tiflow/dm/pkg/shardddl/optimism/keeper.go

Lines 62 to 69 in f917d36

// filter info which doesn't have SourceTable

// SourceTable will be changed after user update block-allow-list

// But old infos still remain in etcd.

// TODO: add a mechanism to remove all outdated infos in etcd.

if !lock.TableExist(info.Source, info.UpSchema, info.UpTable) {

delete(ifm[task][source][schema], table)

continue

}

For create table while leader is transferred, there may be some problem. I will create a pr for create table with more tests.

lichunzhu · 2022-01-26T12:05:36Z

dm/syncer/checkpoint.go

-	sql2, arg := cp.genUpdateSQL(sourceSchema, sourceTable, location, nil, tiBytes, false)
-	sqls = append(sqls, sql2)
-	args = append(args, arg)
+	batch := 100


make 100 as a constant?

GMHDBJD · 2022-01-26T12:33:17Z

Should we delete obsolete init-schema for dm-clusters upgraded from lower versions?

I leave a TODO in

tiflow/dm/dm/common/common.go

Line 81 in c9a821c

// TODO: prune in etcd when upgrade

I think we should prune them after upgrading to a major version(e.g. 6.0)

Should we flush table info for newly added tables

Yes. We still have some problem with newly create/drop table(e.g. #3823), I will create a new pr to solve them.

GMHDBJD · 2022-01-26T12:45:34Z

/run-all-tests

GMHDBJD · 2022-01-26T13:08:08Z

/run-all-tests

dm/pkg/shardddl/optimism/lock.go

GMHDBJD · 2022-02-07T03:58:37Z

/run-all-tests

lichunzhu

Rest LGTM

lichunzhu · 2022-02-07T09:37:15Z

dm/tests/shardddl_optimistic/run.sh

+function random_restart() {
+	mod=$(($RANDOM % 4))
+	if [[ "$mod" == "0" ]]; then
+		restart_master
+	elif [[ "$mod" == "1" ]]; then
+		restart_worker1
+	elif [[ "$mod" == "2" ]]; then
+		restart_worker2
+	else
+		restart_task $cur/conf/double-source-optimistic.yaml
+	fi
+}
+


Actually, I think it's better to test all four cases for integration tests. It will be easier for us to trace which PR introduces the bug.

I want to test different combinations of them. e.g.(restart master and then restart worker), but too many combinations, so use randome. I add some log for trace in 41cf696

lichunzhu · 2022-02-07T09:39:34Z

dm/tests/shardddl_optimistic/run.sh

+	run_sql_source1 "alter table ${shardddl1}.${tb2} add column b varchar(10);"
+	run_sql_source2 "alter table ${shardddl1}.${tb1} add column b varchar(10);"
+	run_sql_source2 "alter table ${shardddl1}.${tb2} add column b varchar(10);"
+
+	run_sql_source1 "insert into ${shardddl1}.${tb1} values(5,'aaa');"
+	run_sql_source1 "insert into ${shardddl1}.${tb2} values(6,'bbb');"
+	run_sql_source2 "insert into ${shardddl1}.${tb1} values(7,'ccc');"
+	run_sql_source2 "insert into ${shardddl1}.${tb2} values(8,'ddd');"
+
+	run_sql_source1 "alter table ${shardddl1}.${tb1} add column c text;"
+	run_sql_source1 "insert into ${shardddl1}.${tb1} values(9,'eee','eee');"
+	run_sql_source1 "alter table ${shardddl1}.${tb2} drop column b;"
+	run_sql_source1 "insert into ${shardddl1}.${tb2} values(10);"
+	run_sql_source2 "alter table ${shardddl1}.${tb1} add column c text;"
+	run_sql_source2 "insert into ${shardddl1}.${tb1} values(11,'fff','fff');"
+	run_sql_source2 "alter table ${shardddl1}.${tb2} drop column b;"
+	run_sql_source2 "insert into ${shardddl1}.${tb2} values(12);"
+
+	run_sql_tidb_with_retry "select count(1) from ${shardddl}.${tb}" "count(1): 12"
+
+	run_dm_ctl $WORK_DIR "127.0.0.1:$MASTER_PORT" \
+		"stop-task test -s mysql-replica-02" \
+		"\"result\": true" 2
+
+	run_sql_source1 "alter table ${shardddl1}.${tb1} drop column b;"
+	run_sql_source1 "insert into ${shardddl1}.${tb1} values(13,'ggg');"
+	run_sql_source1 "alter table ${shardddl1}.${tb2} add column c text;"
+	run_sql_source1 "insert into ${shardddl1}.${tb2} values(14,'hhh');"
+
+	run_sql_tidb_with_retry "select count(1) from ${shardddl}.${tb}" "count(1): 14"
+	run_sql_tidb_with_retry "select count(1) from INFORMATION_SCHEMA.COLUMNS where TABLE_SCHEMA='${shardddl}' AND TABLE_NAME='${tb}';" \
+		"count(1): 2"
+
+	run_dm_ctl $WORK_DIR "127.0.0.1:$MASTER_PORT" \
+		"start-task $cur/conf/double-source-optimistic.yaml -s mysql-replica-02" \
+		"\"result\": true" 2
+
+	run_sql_source1 "insert into ${shardddl1}.${tb1} values(15,'iii');"
+	run_sql_source1 "insert into ${shardddl1}.${tb1} values(16,'jjj');"
+	run_sql_source2 "alter table ${shardddl1}.${tb1} drop column b;"
+	run_sql_source2 "insert into ${shardddl1}.${tb1} values(17,'kkk');"
+	run_sql_source2 "alter table ${shardddl1}.${tb2} add column c text;"
+	run_sql_source2 "insert into ${shardddl1}.${tb2} values(18,'lll');"
+
+	check_sync_diff $WORK_DIR $cur/conf/diff_config.toml
+}
+
+function DM_STOP_TASK_FOR_A_SOURCE_CASE() {
+	run_sql_source1 "insert into ${shardddl1}.${tb1} values(1);"
+	run_sql_source1 "insert into ${shardddl1}.${tb2} values(2);"
+	run_sql_source2 "insert into ${shardddl1}.${tb1} values(3);"
+	run_sql_source2 "insert into ${shardddl1}.${tb2} values(4);"
+
+	run_sql_source1 "alter table ${shardddl1}.${tb1} add column b varchar(10);"
+	run_sql_source1 "alter table ${shardddl1}.${tb2} add column b varchar(10);"
+	run_sql_source2 "alter table ${shardddl1}.${tb1} add column b varchar(10);"
+	run_sql_source2 "alter table ${shardddl1}.${tb2} add column b varchar(10);"
+
+	run_sql_source1 "insert into ${shardddl1}.${tb1} values(5,'aaa');"
+	run_sql_source1 "insert into ${shardddl1}.${tb2} values(6,'bbb');"
+	run_sql_source2 "insert into ${shardddl1}.${tb1} values(7,'ccc');"
+	run_sql_source2 "insert into ${shardddl1}.${tb2} values(8,'ddd');"
+
+	run_sql_source1 "alter table ${shardddl1}.${tb1} add column c text;"
+	run_sql_source1 "insert into ${shardddl1}.${tb1} values(9,'eee','eee');"
+	run_sql_source1 "alter table ${shardddl1}.${tb2} drop column b;"
+	run_sql_source1 "insert into ${shardddl1}.${tb2} values(10);"
+	run_sql_source2 "alter table ${shardddl1}.${tb1} add column c text;"
+	run_sql_source2 "insert into ${shardddl1}.${tb1} values(11,'fff','fff');"
+	run_sql_source2 "alter table ${shardddl1}.${tb2} drop column b;"
+	run_sql_source2 "insert into ${shardddl1}.${tb2} values(12);"
+
+	run_sql_tidb_with_retry "select count(1) from ${shardddl}.${tb}" "count(1): 12"
+
+	run_dm_ctl $WORK_DIR "127.0.0.1:$MASTER_PORT" \
+		"stop-task test -s mysql-replica-02" \
+		"\"result\": true" 2
+
+	run_sql_source1 "alter table ${shardddl1}.${tb1} drop column b;"
+	run_sql_source1 "insert into ${shardddl1}.${tb1} values(13,'ggg');"
+	run_sql_source1 "alter table ${shardddl1}.${tb2} add column c text;"
+	run_sql_source1 "insert into ${shardddl1}.${tb2} values(14,'hhh');"
+
+	run_sql_tidb_with_retry "select count(1) from ${shardddl}.${tb}" "count(1): 14"
+	run_sql_tidb_with_retry "select count(1) from INFORMATION_SCHEMA.COLUMNS where TABLE_SCHEMA='${shardddl}' AND TABLE_NAME='${tb}';" \
+		"count(1): 2"
+
+	run_dm_ctl $WORK_DIR "127.0.0.1:$MASTER_PORT" \
+		"start-task $cur/conf/double-source-optimistic.yaml -s mysql-replica-02" \
+		"\"result\": true" 2
+
+	run_sql_source1 "insert into ${shardddl1}.${tb1} values(15,'iii');"
+	run_sql_source1 "insert into ${shardddl1}.${tb1} values(16,'jjj');"
+	run_sql_source2 "alter table ${shardddl1}.${tb1} drop column b;"
+	run_sql_source2 "insert into ${shardddl1}.${tb1} values(17,'kkk');"
+	run_sql_source2 "alter table ${shardddl1}.${tb2} add column c text;"
+	run_sql_source2 "insert into ${shardddl1}.${tb2} values(18,'lll');"
+
+	check_sync_diff $WORK_DIR $cur/conf/diff_config.toml
+}


These two functions look exactly the same.

lichunzhu

LGTM

lichunzhu · 2022-02-07T10:21:12Z

/unhold

lichunzhu · 2022-02-07T10:21:18Z

/merge

ti-chi-bot · 2022-02-07T10:21:21Z

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

Commit hash: 41cf696

init logic

cf3881c

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 16, 2021

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

GMHDBJD changed the base branch from optimistDDLDev to master December 16, 2021 05:41

remove old code

3022cc5

asddongmen added the area/dm Issues or PRs related to DM. label Dec 16, 2021

Merge remote-tracking branch 'upstream/master' into supportInconsiste…

d509b0c

…ntSchema

remove meta when stop task

25c1d59

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

GMHDBJD added 2 commits December 19, 2021 08:49

Merge remote-tracking branch 'upstream/master' into supportInconsiste…

7b0ad14

…ntSchema

fix sequence_sharding_optimistic

296fca9

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

remove old code

f9a691f

fix lint

48b58b7

GMHDBJD added 2 commits December 19, 2021 22:11

fix update ba rule test

462cc78

Merge remote-tracking branch 'upstream/master' into supportInconsiste…

7034319

…ntSchema

GMHDBJD added 2 commits December 20, 2021 00:55

fix ut

d61d3fe

Merge remote-tracking branch 'upstream/master' into supportInconsiste…

fabbfd5

…ntSchema

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 Jan 26, 2022

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

GMHDBJD and others added 2 commits January 26, 2022 14:17

Merge branch 'master' into supportInconsistentSchema

7f296c3

Update dm/pkg/shardddl/optimism/keeper.go

c9a821c

Co-authored-by: lance6716 <lance6716@gmail.com>

lichunzhu reviewed Jan 26, 2022

View reviewed changes

GMHDBJD and others added 2 commits January 26, 2022 20:44

address comment

5afac75

Merge branch 'master' into supportInconsistentSchema

8564fdf

glorv reviewed Jan 27, 2022

View reviewed changes

dm/pkg/shardddl/optimism/lock.go Show resolved Hide resolved

Merge branch 'master' into supportInconsistentSchema

762fb91

ti-chi-bot added do-not-merge/needs-linked-issue do-not-merge/needs-triage-completed and removed do-not-merge/needs-linked-issue labels Feb 7, 2022

ti-chi-bot removed the do-not-merge/needs-triage-completed label Feb 7, 2022

Merge branch 'master' into supportInconsistentSchema

96aee90

lichunzhu reviewed Feb 7, 2022

View reviewed changes

address comment

41cf696

lichunzhu approved these changes Feb 7, 2022

View reviewed changes

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

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

ti-chi-bot merged commit 5c7d238 into pingcap:master Feb 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimistic: support start task with inconsistent upstream table schema #3903

Optimistic: support start task with inconsistent upstream table schema #3903

GMHDBJD commented Dec 16, 2021 •

edited

Loading

ti-chi-bot commented Dec 16, 2021 •

edited

Loading

GMHDBJD commented Dec 16, 2021

GMHDBJD commented Dec 16, 2021

GMHDBJD commented Dec 16, 2021

GMHDBJD commented Dec 17, 2021

GMHDBJD commented Dec 19, 2021

GMHDBJD commented Dec 19, 2021

GMHDBJD commented Dec 19, 2021

GMHDBJD commented Dec 19, 2021

GMHDBJD commented Dec 20, 2021

GMHDBJD commented Dec 20, 2021

lance6716 commented Jan 26, 2022

lichunzhu left a comment

lichunzhu Jan 26, 2022

GMHDBJD Jan 26, 2022

GMHDBJD Feb 7, 2022

lichunzhu Jan 26, 2022

GMHDBJD Jan 26, 2022

GMHDBJD commented Jan 26, 2022

GMHDBJD commented Jan 26, 2022

GMHDBJD commented Jan 26, 2022

GMHDBJD commented Feb 7, 2022

lichunzhu left a comment

lichunzhu Feb 7, 2022 •

edited

Loading

GMHDBJD Feb 7, 2022

lichunzhu Feb 7, 2022

lichunzhu Feb 7, 2022

GMHDBJD Feb 7, 2022

lichunzhu left a comment

lichunzhu commented Feb 7, 2022

lichunzhu commented Feb 7, 2022

ti-chi-bot commented Feb 7, 2022

	// filter info which doesn't have SourceTable
	// SourceTable will be changed after user update block-allow-list
	// But old infos still remain in etcd.
	// TODO: add a mechanism to remove all outdated infos in etcd.
	if !lock.TableExist(info.Source, info.UpSchema, info.UpTable) {
	delete(ifm[task][source][schema], table)
	continue
	}

Optimistic: support start task with inconsistent upstream table schema #3903

Optimistic: support start task with inconsistent upstream table schema #3903

Conversation

GMHDBJD commented Dec 16, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Dec 16, 2021 • edited Loading

GMHDBJD commented Dec 16, 2021

GMHDBJD commented Dec 16, 2021

GMHDBJD commented Dec 16, 2021

GMHDBJD commented Dec 17, 2021

GMHDBJD commented Dec 19, 2021

GMHDBJD commented Dec 19, 2021

GMHDBJD commented Dec 19, 2021

GMHDBJD commented Dec 19, 2021

GMHDBJD commented Dec 20, 2021

GMHDBJD commented Dec 20, 2021

lance6716 commented Jan 26, 2022

lichunzhu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GMHDBJD commented Jan 26, 2022

GMHDBJD commented Jan 26, 2022

GMHDBJD commented Jan 26, 2022

GMHDBJD commented Feb 7, 2022

lichunzhu left a comment

Choose a reason for hiding this comment

lichunzhu Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lichunzhu left a comment

Choose a reason for hiding this comment

lichunzhu commented Feb 7, 2022

lichunzhu commented Feb 7, 2022

ti-chi-bot commented Feb 7, 2022

GMHDBJD commented Dec 16, 2021 •

edited

Loading

ti-chi-bot commented Dec 16, 2021 •

edited

Loading

lichunzhu Feb 7, 2022 •

edited

Loading