-
Notifications
You must be signed in to change notification settings - Fork 188
loader, syncer: support create/drop view #1310
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
waiting pingcap/tidb-tools#405 (but I think it's OK to merge this PR before tidb-tools. In days before v2.0.1 release will update dependencies as well) |
@@ -1272,8 +1420,18 @@ func (l *Loader) restoreData(ctx context.Context) error { | |||
dispatchMap[fmt.Sprintf("%s_%s_%s", db, table, file)] = j | |||
} | |||
} | |||
|
|||
for view := range views { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to create the view in a different database with the table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess user may create that type of view. And dumpling will generate a "table file" for each view at view's database, I guess this logic is fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in d168716
if err != nil { | ||
return terror.WithScope(err, terror.ScopeInternal) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sames that loader
will ignore all SET
sqls here. Is this by design?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgot 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, query
will be added in L1222. We just didn't rename them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, yes. My fault. Maybe you can add a comment before L1191 🤣
for view := range views { | ||
viewFile := fmt.Sprintf("%s/%s.%s-schema-view.sql", l.cfg.Dir, db, view) | ||
l.logger.Info("start to create view", zap.String("view file", viewFile)) | ||
err := l.restoreView(ctx, dbConn, viewFile, db, view) | ||
if err != nil { | ||
return err | ||
} | ||
l.logger.Info("finish to create view", zap.String("view file", viewFile)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if some views needed tables in other databases are not created yet? I think we should create views when all tables in all databases have been created correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in d168716
updated code and PR description, PTAL @lichunzhu @GMHDBJD @csuzhangxc |
1f24a99
to
a91adcb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PTAL @GMHDBJD |
@@ -1868,7 +1872,18 @@ func (s *Syncer) handleQueryEvent(ev *replication.QueryEvent, ec eventContext) e | |||
return err | |||
} | |||
|
|||
if s.cfg.ShardMode == "" { | |||
// VIEW statements are not needed sharding synchronized, so pretend this is no-shard-mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the behavior when view statement in resharding sync? such as ddl1 view ddl2
. How about add a test for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in pessimistic mode, DDL lock will block sync of VIEW. in other cases VIEW is executed directly in downstream
data race recorded in #1351
|
run_sql_source1 "create view ${shardddl1}.v1 as select * from ${shardddl1}.${tb1};" | ||
sleep 1 | ||
run_sql_tidb "show create view ${shardddl}.v" | ||
check_contains "View: v" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems view is created here in pessimistic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. VIEW is pretend to sync in none ShardMode, try to immediately execute to downstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the table may not be created?
create table <-- not sync
create view <-- sync
create table
create view
@lance6716: PR needs rebase. 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 kubernetes/test-infra repository. |
/hold |
What problem does this PR solve?
close #1300
What is changed and how it works?
support dumpling's view file in load unit, support sync view as well.
sync of view didn't need a sharding sync lock, that is to say we could directly execute CREATE VIEW IF NOT EXISTS / DROP VIEW IF EXISTS to downstream when any shard send it
Check List
Tests
Code changes
Side effects
Related changes