Skip to content

CopySchemaShard: apply changes with foreign key checks disabled#4722

Closed
dweitzman wants to merge 1 commit intovitessio:masterfrom
dweitzman:copy_schema_shard_fk_checks
Closed

CopySchemaShard: apply changes with foreign key checks disabled#4722
dweitzman wants to merge 1 commit intovitessio:masterfrom
dweitzman:copy_schema_shard_fk_checks

Conversation

@dweitzman
Copy link
Copy Markdown
Collaborator

@dweitzman dweitzman commented Mar 13, 2019

People who don't have foreign keys won't care, and people who
do have FKs need the checks disabled because the logic is not yet clever enough
to create the tables in an order which is compatible with their
foreign key dependencies

@dweitzman dweitzman requested a review from sougou as a code owner March 13, 2019 21:53
@dweitzman
Copy link
Copy Markdown
Collaborator Author

Not yet tested

Copy link
Copy Markdown
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it's better to do this at the beginning of SplitClone instead? And then, SplitClone can enable checks just before setting up vreplication.

I can accordingly change vreplication copy do the same thing.

@dweitzman
Copy link
Copy Markdown
Collaborator Author

CopySchemaShard will need to run before SplitClone, so it seems like they both need something like this.

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Mar 31, 2019

CopySchemaShard itself should succeed without the check, right? The tables have no data in them.

@derekperkins
Copy link
Copy Markdown
Member

It still errors out if the tables aren't created in the right order.

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Mar 31, 2019

Ahh, of course...
Should re-enable checks after the apply?
Then we should have SplitClone disable for its own sake.

@dweitzman
Copy link
Copy Markdown
Collaborator Author

Ahh, of course...
Should re-enable checks after the apply?
Then we should have SplitClone disable for its own sake.

set foreign_key_checks = 0 should only be setting a session variable, not a global, so if usePool=false does what it sounds like it does I would think there shouldn't be a need to reset it on the connection after copying the schema.

I could potentially add set foreign_key_checks = @@foreign_key_checks afterwards to copy the global back into the session as a precaution in case someone removes the usePool=false in the future. Was trying to discourage that with a comment, but a little extra caution seems harmless. I don't have strong feelings either way.

What do you think?

sougou
sougou previously approved these changes Mar 31, 2019
Copy link
Copy Markdown
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, this is good to go. I've restarted the failed tests. We can merge once they pass.

@dweitzman dweitzman force-pushed the copy_schema_shard_fk_checks branch from fda5de4 to 3d868a0 Compare March 31, 2019 08:36
@sougou
Copy link
Copy Markdown
Contributor

sougou commented Mar 31, 2019

Looks like the tests need fixing after all.

@sougou sougou dismissed their stale review March 31, 2019 15:57

Tests need fixing

@dweitzman
Copy link
Copy Markdown
Collaborator Author

Oh, I thought a dba query would allow running an arbitrary set but that must not be true. Need to look into this:

TabletManager.ExecuteFetchAsDba on cell1-0000000010 error: rpc error: code = Unknown desc = unknown error: query: 'set foreign_key_checks = 0' is not supported on destinationMasterDb (errno 1105) (sqlstate HY000) during query: set foreign_key_checks = 0: rpc error: code = Unknown desc = unknown error: query: 'set foreign_key_checks = 0' is not supported on destinationMasterDb (errno 1105) (sqlstate HY000) during query: set foreign_key_checks = 0

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Mar 31, 2019

I think these tests use mocks. You'll just have to add that query to the expected list.

People who don't have foreign keys won't care, and people who
do need checks disabled because the logic is not yet clever enough
to create the tables in an order which is compatible with their
foreign key dependencies

Signed-off-by: David Weitzman <dweitzman@pinterest.com>
@dweitzman dweitzman force-pushed the copy_schema_shard_fk_checks branch from 3d868a0 to fbd0a63 Compare March 31, 2019 16:23
@sougou
Copy link
Copy Markdown
Contributor

sougou commented Aug 17, 2019

@dweitzman we should rework this PR into the newer vreplication. Disable FK checks during vrepl and enable it just before accepting writes in MigrateWrites.

@deepthi
Copy link
Copy Markdown
Collaborator

deepthi commented Oct 6, 2020

This has been addressed in #6284. Please feel free to re-open if that doesn't cover all situations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants