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

accepting --assume-rbr, remove SUPER requirement #156

Merged
merged 3 commits into from
Aug 18, 2016

Conversation

shlomi-noach
Copy link
Contributor

@shlomi-noach shlomi-noach commented Aug 12, 2016

Related issue: #132

Avoid restarting replication, avoid need for SUPER

gh-ost cannot verify the replication thread truly uses RBR, and wishes to restart replication (stop slave; start slave;). However that requires SUPER privilege.
This PR supports --assume-rbr where the user clearly indicates "yes, this is truly and really RBR, and the replica is using RBR". If binlog_format=ROW then gh-ost is happy to proceed without restarting replication, hence without requiring SUPER privilege.

This PR [briefly explain what is does]

  • contributed code is using same conventions as original code
  • code is formatted via gofmt (please avoid goimports)
  • code is built via ./build.sh

- avoiding need to restart replication
- in turn avoiding need for SUPER
@SuperQ
Copy link
Contributor

SuperQ commented Aug 12, 2016

Normally I would call this --assume-rbr, but "trust" is also fine.

👍

@pbitty
Copy link
Contributor

pbitty commented Aug 12, 2016

Hi @shlomi-noach, have you considered the approach of detecting whether ROW format is actually being used? By sending some test updates to a table, we could see the format of the events arriving in the binlog.

Something like this:

  1. On master: insert into _rbrtest_gho (a) values (1), (2), (3);
  2. Wait for the corresponding Write_rows events in the binlog

If the event arrives as a Query event then we know we're using STATEMENT format and can return the appropriate error message.

This may be out of scope of the actual PR, and I can open an issue if you prefer to have the discussion there.

PS. Kudos on coming up with such a novel approach to live schema changes.

@shlomi-noach
Copy link
Contributor Author

@pbitty this is a good suggestion, thank you! It may complicate stuff a bit, but seems worthwhile. I think I might first iterate with --trust-rbr and then iterate again with your idea.

@shlomi-noach
Copy link
Contributor Author

@SuperQ --assume-rbr does sound better. Will change.

@SuperQ
Copy link
Contributor

SuperQ commented Aug 12, 2016

Maybe we can file a feature request upstream to extend the SHOW SLAVE STATUS to include the binlog mode for the running thread.

@shlomi-noach shlomi-noach changed the title accepting --trust-rbr accepting --assume-rbr Aug 15, 2016
@SuperQ
Copy link
Contributor

SuperQ commented Aug 15, 2016

👍 Nice

@shlomi-noach
Copy link
Contributor Author

Noting down that in the event gh-ost thinks replication uses RBR where in fact it does not, the current behavior for migration is to just not work, because gh-ost will not be able to intercept its very own changelog table events.

I'm actually good to merge this, and iterate later with double-safety checks, such as:

  • oh, I'm waiting for too long for my own changelog event, and replication seems fine, let's inject a noop DDL statement (drop view if exists no_such_view_1234567890randomnumber) and see if we're able to intercept that, hence SBR.

@shlomi-noach
Copy link
Contributor Author

Need to add documentation:

  • mentioning this flag
  • discussing SUPER
  • and anywhere SUPER or RDS is discussed

- introducing --assume-rbr
- discussing the implication of being able to run without SUPER
@shlomi-noach shlomi-noach changed the title accepting --assume-rbr accepting --assume-rbr, remove SUPER requirement Aug 18, 2016
@shlomi-noach shlomi-noach merged commit d9ae2f3 into master Aug 18, 2016
@shlomi-noach shlomi-noach deleted the avoid-restarting-replication branch August 18, 2016 08:11
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.

3 participants