MDEV-23077: flashbacking the update/delete + join statements has some bugs #1674
Conversation
lukemakeit
commented
Oct 4, 2020
lukemakeit
commented
- Extend the --table --database option of mysqlbinlog to allow --table and --database to support plural values;
- Fix bug 2 and bug 3 described by MDEV-23077;
…le and --database to support plural values; 2. Fix bug 2 and bug 3 described by MDEV-23077(https://jira.mariadb.org/browse/MDEV-23077);
|
|
|
In 2016 we signed an MCA with the name of Tencent Game DBA Team, and I am one of the team members. |
There was a problem hiding this comment.
Hi @lukexwang ! Many thanks for submitting a new PR :-) From the point of view of code structure and basic functionality, the patch looks good, all comments from the previous PR seem to be addressed. There are some more minor comments inline below. I'll again leave it to @andrelkin for checking the replication side of the patch, followed by me thoroughly testing it. Good job and thanks again for the contribution!
| Indicates whether the given database should be filtered out, | ||
| according to the --database=X option. | ||
| Indicates whether the given databases should be filtered out, | ||
| according to the --database=X or --databases=X1,X2,X3 option. |
There was a problem hiding this comment.
I think this is a residual line from the previous version of the path. No need for --databases anymore right?
| if(log_dbname == NULL) | ||
| return false; | ||
|
|
||
| if (one_database) |
There was a problem hiding this comment.
It seems to me given your current implementation that we don't need this variable anymore. We can just replace its usage with std::set's .empty() function. Let me know if I'm mistaken.
| return false; | ||
|
|
||
| if (one_database) | ||
| return filter_databases.count(log_dbname) == 0; |
There was a problem hiding this comment.
As far as I know std::set's .count() overload that would take a const char* in this situation is only available starting with c++14. Pre c++14, .count() only takes Key type which is std::string in this case. I don't know if c++14 is available on all platforms for the server codebase. @andrelkin can you help me out here?
There was a problem hiding this comment.
Hi @andrelkin ,I need your help.
There was a problem hiding this comment.
Hi! Sorry for delay (I thought there's some amount of review work), to the question
https://en.cppreference.com/w/cpp/container/set/count
specifies as c++14 only the template version of the method:
template< class K > size_type count( const K& x ) const; | (2) | (since C++14)
|
Hi @lukexwang any update regarding this patch? |
