Lixun Peng <lixun.peng@mariadb.com> writes:

*Currently, flashback will continue to run when faced a error, because the usage is using flashback option to output the flashback events into a file, and then import the file into mysql.*

*Do you think we should stop to process when we found DDL?* *Because flashback feature just output the events that can rollback the database, so if we found the query event, we can stop to output the events anymore.* *But in my opinion, I think flashback it's a geek feature, the users know what they are doing. We can provide a option that if continue to process when faced a query event, what do you think?*

Yes, I agree. It's definitely fine to have an option to continue to process even in case of query event. As you explained, the result of flashback is at first just an SQL script, only when run will this script modify anything. So it might be ok go give the output by default even in case when query events are encountered. But I think there should be at least a warning produced, and or an optional error. That should be easy to do (just set a flag if query event seen), and would help user to detect the problem.

*Yes, the test cases are not contain the --review option, because in Alibaba the DBAs consider this option is useless in most cases, then I'm not maintain this part of code so long, we should evaluate is this feature

Right, I understand. My general opinion is that features pushed into the main tree should be reasonably complete, or not pushed at all. But I agree that some feature might not be priority at the moment, and so could be omitted rather than spend a lot of time polishing.

...

Did you test this feature with binlog checksums enabled?

*Now, that's a problem, if we support checksum, I should re-calculate the checksum after modified the events, that maybe cost about half of month. I

Well, at least it should be tested so that it doesn't crash etc. Binlog checksums is a constant source of pain, unfortunately. One thing that can go wrong is to compute the length of events incorrectly, that is easy to fix if the case (just subtract 4 in case of checksum). I don't think flashback needs to do extra work to eg. verify the checksums or any such. If there is a problem with the code rejecting the modified events, then I think it's not hard just to call the function that re-computes the checksum. But if it turns out to be harder, I'm fine with just putting in the documentation that flashback is not supported, and detecting checksums and giving an error. Just things should still behave somehow reasonable in case of checksums, not like crash or silently produce garbage results. And it should be tested.

...

^^^ "Check if the table was already created" ?

*Yes, you're right. Sorry for my poor English.*

Sure, actually your English is just fine, I have no problems understanding :-) I'm not a native English speaker myself either, so just when I notice something that seems off a bit, I may suggest better wording.

...

...

+SET binlog_format= ROW;

This should not be needed, since you have the proper have_binlog_format_row.inc. Or is there some specific reason it is needed, and if so, what?

*I don't know why, in my environment, event I sourced have_binlog_format_row.inc, mtr will also run with statement format.* *So I added the set command.*

Ok, in any case, there is no harm in having SET binlog_format=ROW, I was just wondering... not important.

...

Can you come up with a better name for "only_parse"? It is not clear to me what it means, I think it is an option if the function needs to instead generate the SQL for filling-in the flashback review table?

*Yes, you're right.* *How do you think "no_fill_output"?*

Yes, that's fine, thanks.

*In my understanding, `` can always be used.*

Yes, I think you are right. Probably just my memory playing tricks on me.

These comments are very very useful for me, learned so many things!

Ok, great, glad that you found them useful. Thanks, - Kristian.