Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drainer: bugfix, handle "missing column" when a column is getting dropped #803

Merged
merged 20 commits into from
Nov 21, 2019

Conversation

suzaku
Copy link
Contributor

@suzaku suzaku commented Nov 11, 2019

What problem does this PR solve?

DML binlogs may be wrong if there's a concurrent drop column DDL running.
The column being dropped may be missing before the corresponding DDL binlog reached drainer, because TiDB stops sending the dropped column starting from the DeleteOnly state.

What is changed and how it works?

  1. Add an integration test to expose the problem.
  2. TiDB would send an extra DDL binlog when the job reaches DeleteOnly state ddl: add a binlog when dropping column tidb#13536
  3. Drainer use this extra binlog to decide that a table is dropping column
  4. If a column is missing while the table is dropping column, we fill the column with default values.

Check List

Tests

  • Integration test

Code changes

Side effects

Related changes

@suzaku
Copy link
Contributor Author

suzaku commented Nov 11, 2019

/run-integration-tests

2 similar comments
@suzaku
Copy link
Contributor Author

suzaku commented Nov 11, 2019

/run-integration-tests

@suzaku
Copy link
Contributor Author

suzaku commented Nov 13, 2019

/run-integration-tests

@suzaku
Copy link
Contributor Author

suzaku commented Nov 14, 2019

/run-integration-tests

@suzaku
Copy link
Contributor Author

suzaku commented Nov 14, 2019

/run-unit-tests

@IANTHEREAL
Copy link
Collaborator

LGTM now

@suzaku suzaku changed the title tests: Add a case in integration tests to expose a bug drainer: bugfix, handle "missing column" when a column is getting dropped Nov 20, 2019
@suzaku
Copy link
Contributor Author

suzaku commented Nov 20, 2019

/run-all-tests

3 similar comments
@suzaku
Copy link
Contributor Author

suzaku commented Nov 20, 2019

/run-all-tests

@suzaku
Copy link
Contributor Author

suzaku commented Nov 20, 2019

/run-all-tests

@suzaku
Copy link
Contributor Author

suzaku commented Nov 20, 2019

/run-all-tests

@suzaku
Copy link
Contributor Author

suzaku commented Nov 20, 2019

/run-all-tests

@suzaku
Copy link
Contributor Author

suzaku commented Nov 20, 2019

/run-unit-tests

@suzaku
Copy link
Contributor Author

suzaku commented Nov 20, 2019

/run-all-tests

1 similar comment
@suzaku
Copy link
Contributor Author

suzaku commented Nov 20, 2019

/run-all-tests

@suzaku
Copy link
Contributor Author

suzaku commented Nov 20, 2019

/run-all-tests

@ericsyh
Copy link
Contributor

ericsyh commented Nov 20, 2019

@suzaku pls fix the conflicts.

@suzaku suzaku added status/PTAL type/bug This issue is a bug type/bug-fix and removed type/bug This issue is a bug labels Nov 21, 2019
Copy link
Contributor

@july2993 july2993 left a comment

Choose a reason for hiding this comment

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

LGTM
Note after added the one more ddl binlog event, with the new version tidb, the old version pump
will or will not execute the ddl(when receiving the newly added binlog event)

@WangXiangUSTC
Copy link
Contributor

LGTM

@WangXiangUSTC
Copy link
Contributor

need to add release note, and update upgrade document(may not compatible with old tidb?)

@suzaku
Copy link
Contributor Author

suzaku commented Nov 21, 2019

need to add release note, and update upgrade document(may not compatible with old tidb?)

  1. Where can I add a release note?
  2. It's compatible with older TiDB, with no extra binlog it works the same way as before (with the bug). But older Drainers is not compatible with newer TiDBs because they can't handle the extra binlog correctly.

@suzaku
Copy link
Contributor Author

suzaku commented Nov 21, 2019

/run-cherry-picker

@suzaku suzaku merged commit 72a15d4 into pingcap:master Nov 21, 2019
@sre-bot
Copy link

sre-bot commented Nov 21, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link

sre-bot commented Nov 21, 2019

cherry pick to release-3.0 failed

@suzaku suzaku deleted the test-drop-column branch November 21, 2019 07:04
suzaku added a commit to suzaku/tidb-binlog that referenced this pull request Nov 21, 2019
suzaku added a commit to suzaku/tidb-binlog that referenced this pull request Nov 21, 2019
suzaku added a commit to suzaku/tidb-binlog that referenced this pull request Nov 21, 2019
suzaku added a commit to suzaku/tidb-binlog that referenced this pull request Nov 21, 2019
suzaku added a commit to suzaku/tidb-binlog that referenced this pull request Nov 21, 2019
suzaku added a commit to suzaku/tidb-binlog that referenced this pull request Nov 22, 2019
IANTHEREAL pushed a commit that referenced this pull request Nov 22, 2019
…pped (#827)

* drainer: bugfix, handle "missing column" when a column is getting dropped (#803)
suzaku added a commit to suzaku/tidb-binlog that referenced this pull request Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants