MDEV-23077: flashbacking the update/delete + join statements has some bugs #1674

lukemakeit · 2020-10-04T07:49:21Z

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);

CLAassistant · 2020-10-04T07:49:27Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

lukemakeit · 2020-10-04T07:50:31Z

In 2016 we signed an MCA with the name of Tencent Game DBA Team, and I am one of the team members.
I confirm the code being submitted is offered under the terms of the MCA, and that I am authorized to contribute it.

robertbindar

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!

robertbindar · 2020-10-23T13:12:55Z

client/mysqlbinlog.cc

-  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.


I think this is a residual line from the previous version of the path. No need for --databases anymore right?

robertbindar · 2020-10-23T13:16:01Z

client/mysqlbinlog.cc

+  if(log_dbname == NULL)
+    return false;
+
+  if (one_database)


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.

robertbindar · 2020-10-23T13:19:46Z

client/mysqlbinlog.cc

+    return false;
+
+  if (one_database)
+    return filter_databases.count(log_dbname) == 0;


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?

Hi @andrelkin ,I need your help.

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)

The implicit cast from const char* to std::string works here too.

#include <string> #include <set> int main() { const char* str = "world"; std::set<std::string> set = {str}; return set.count(str) != 1; }

I ran cmake --build … --target mariadb-binlog and it succeeded too with unrelated hiccups; how about rebasing first so our CI can take over?

Submodule libmariadb contains modified content diff --git a/libmariadb/cmake/ConnectorName.cmake b/libmariadb/cmake/ConnectorName.cmake index b7bbbad..357b8ac 100644 --- a/libmariadb/cmake/ConnectorName.cmake +++ b/libmariadb/cmake/ConnectorName.cmake @@ -22,7 +22,7 @@ IF(CMAKE_SYSTEM_NAME MATCHES "Windows") SET(MACHINE_NAME "x64") ELSE() SET(MACHINE_NAME "32") - END() + ENDIF() ENDIF() SET(product_name "mysql-connector-c-${CPACK_PACKAGE_VERSION}-${PLATFORM_NAME}${CONCAT_SIGN}${MACHINE_NAME}") diff --git a/mysys/lf_alloc-pin.c b/mysys/lf_alloc-pin.c index 2162e1771b..fc118744c5 100644 --- a/mysys/lf_alloc-pin.c +++ b/mysys/lf_alloc-pin.c @@ -230,7 +230,7 @@ void lf_pinbox_put_pins(LF_PINS *pins) { lf_pinbox_real_free(pins); if (pins->purgatory_count) - pthread_yield(); + {} } top_ver= pinbox->pinstack_top_ver; do diff --git a/mysys_ssl/my_crypt.cc b/mysys_ssl/my_crypt.cc index e512eee906..ba4d0fdd40 100644 --- a/mysys_ssl/my_crypt.cc +++ b/mysys_ssl/my_crypt.cc @@ -134,7 +134,7 @@ class MyCTX_nopad : public MyCTX buf_len %= MY_AES_BLOCK_SIZE; if (buf_len) { - uchar *buf= EVP_CIPHER_CTX_buf_noconst(ctx); + uchar *buf= nullptr; /* Not much we can do, block ciphers cannot encrypt data that aren't a multiple of the block length. At least not without padding. diff --git a/mysys_ssl/my_sha.ic b/mysys_ssl/my_sha.ic index 6bba614765..0f13dfbee0 100644 --- a/mysys_ssl/my_sha.ic +++ b/mysys_ssl/my_sha.ic @@ -96,7 +96,7 @@ static void sha_result(CONTEXT *context, uchar digest[HASH_SIZE]) static void sha_init(CONTEXT *context) { - SHA_Init(context); } static void sha_init_fast(CONTEXT *context) @@ -106,12 +106,12 @@ static void sha_init_fast(CONTEXT *context) static void sha_input(CONTEXT *context, const uchar *buf, unsigned len) { - SHA_Update(context, buf, len); } static void sha_result(CONTEXT *context, uchar digest[HASH_SIZE]) { - SHA_Final(digest, context); } #endif /* HAVE_WOLFSSL */

an3l · 2021-03-25T17:51:09Z

Hi @lukexwang any update regarding this patch?

ParadoxV5

So the review’s approved and just gotta port to C++11?
Sure thing; although looks like no changes required.

One concern, though: This patch adds multiple-selection support to --database and --table, but the MDEV and PR are worded as bugfixes.
Please confirm.

ParadoxV5 · 2025-03-30T02:24:02Z

client/mysqlbinlog.cc

+    return false;
+
+  if (one_database)
+    return filter_databases.count(log_dbname) == 0;


The implicit cast from const char* to std::string works here too.

#include <string> #include <set> int main() { const char* str = "world"; std::set<std::string> set = {str}; return set.count(str) != 1; }

I ran cmake --build … --target mariadb-binlog and it succeeded too with unrelated hiccups; how about rebasing first so our CI can take over?

Submodule libmariadb contains modified content diff --git a/libmariadb/cmake/ConnectorName.cmake b/libmariadb/cmake/ConnectorName.cmake index b7bbbad..357b8ac 100644 --- a/libmariadb/cmake/ConnectorName.cmake +++ b/libmariadb/cmake/ConnectorName.cmake @@ -22,7 +22,7 @@ IF(CMAKE_SYSTEM_NAME MATCHES "Windows") SET(MACHINE_NAME "x64") ELSE() SET(MACHINE_NAME "32") - END() + ENDIF() ENDIF() SET(product_name "mysql-connector-c-${CPACK_PACKAGE_VERSION}-${PLATFORM_NAME}${CONCAT_SIGN}${MACHINE_NAME}") diff --git a/mysys/lf_alloc-pin.c b/mysys/lf_alloc-pin.c index 2162e1771b..fc118744c5 100644 --- a/mysys/lf_alloc-pin.c +++ b/mysys/lf_alloc-pin.c @@ -230,7 +230,7 @@ void lf_pinbox_put_pins(LF_PINS *pins) { lf_pinbox_real_free(pins); if (pins->purgatory_count) - pthread_yield(); + {} } top_ver= pinbox->pinstack_top_ver; do diff --git a/mysys_ssl/my_crypt.cc b/mysys_ssl/my_crypt.cc index e512eee906..ba4d0fdd40 100644 --- a/mysys_ssl/my_crypt.cc +++ b/mysys_ssl/my_crypt.cc @@ -134,7 +134,7 @@ class MyCTX_nopad : public MyCTX buf_len %= MY_AES_BLOCK_SIZE; if (buf_len) { - uchar *buf= EVP_CIPHER_CTX_buf_noconst(ctx); + uchar *buf= nullptr; /* Not much we can do, block ciphers cannot encrypt data that aren't a multiple of the block length. At least not without padding. diff --git a/mysys_ssl/my_sha.ic b/mysys_ssl/my_sha.ic index 6bba614765..0f13dfbee0 100644 --- a/mysys_ssl/my_sha.ic +++ b/mysys_ssl/my_sha.ic @@ -96,7 +96,7 @@ static void sha_result(CONTEXT *context, uchar digest[HASH_SIZE]) static void sha_init(CONTEXT *context) { - SHA_Init(context); } static void sha_init_fast(CONTEXT *context) @@ -106,12 +106,12 @@ static void sha_init_fast(CONTEXT *context) static void sha_input(CONTEXT *context, const uchar *buf, unsigned len) { - SHA_Update(context, buf, len); } static void sha_result(CONTEXT *context, uchar digest[HASH_SIZE]) { - SHA_Final(digest, context); } #endif /* HAVE_WOLFSSL */

ParadoxV5 · 2025-03-30T03:13:02Z

client/mysqlbinlog.cc

+static std::set<std::string> filter_databases;
+static std::set<std::string> filter_tables;


Ayo, we usin’ C++ string and (tree)set instead of our String and (C-compatible?) HASH now? 😀 Count me in.

By the way, we don’t need the elements sorted, right? So how about an unordered hashset?

Suggested change

static std::set<std::string> filter_databases;

static std::set<std::string> filter_tables;

static std::unordered_set<std::string> filter_databases;

static std::unordered_set<std::string> filter_tables;

https://en.cppreference.com/w/cpp/container/unordered_set

Also, while neither set compares char* contents out-of-the-box (they only compare the *), we can optionally supply our own comparators in the template arguments, and even a memory allocator (my_malloc) too!

A good point that the unordered-ness. I don't see either where the ordered access is required.

andrelkin · 2025-03-31T08:58:39Z

So the review’s approved and just gotta port to C++11? Sure thing; although looks like no changes required.

One concern, though: This patch adds multiple-selection support to --database and --table, but the MDEV and PR are worded as bugfixes. Please confirm.

It should be fine to regard this one as bugfixes. It's a client code and the options' types extension is backward compatible.

ParadoxV5

To: @andrelkin; CC: @bnestere, ~~@robertbindar~~

One concern, though: This patch adds multiple-selection support to --database and --table, but the MDEV and PR are worded as bugfixes. Please confirm.

It should be fine to regard this one as bugfixes. It's a client code and the options' types extension is backward compatible.

No. Searching filter_ on the diff shows that this new feature has nothing to do with the reported bugs ~~on their own.~~ [EDIT: at all!]
And, pedantically considering, feature-wise compatible doesn’t mean 100% compatible: the behavior of argumets with ,s changes from print nothing (if not error) to print something.

With good commit practices in mind, I’m thinking about closing this PR unmerged and splitting it into

a bugfix PR specific to MDEV-23077 (with rebase to 10.11), and
in the next feature sprint, a feature PR (no separate MDEV currently).

(I could just force-push into the contributor’s branch since “✔️ Maintainers are allowed to edit this pull request.”, but I prefer keeping their record intact.)

ParadoxV5 · 2025-04-03T01:33:50Z

client/mysqlbinlog.cc

+             copy_event_cache_to_file_and_reinit(&print_event_info->tail_cache,
+                                                 result_file)))
+          return 1;
+      }else if(opt_flashback && events_in_stmt.elements > 0){


Suggested change

}else if(opt_flashback && events_in_stmt.elements > 0){

}

else if (opt_flashback && events_in_stmt.elements > 0)

{

Agreed with the good practice which always prevail.
Sure it's a new feature, but let me be pedantic too 😃 to ask what specifically
do you mean with

the behavior of arguments with ,s

?

Is it that the OLD would error out in case of the comma separated list?
If something else, could you please come back with an example?

Thank you Jimmy!

Yes. OLD: school,class outputs no row events given tables cannot have , in their names.

The consideration is pedantic because, of course, relying on whether inputs are unsupported to sanitize them is not a secure practice.
What’s broken is an anti-feature, not a feature.

vuvova · 2025-04-15T15:35:52Z

This is taken into #3968 and will be fixed there. With additional fixes and acknowledging the original author.

1. Extend the --table --database option of mysqlbinlog to allow --tab…

f826448

…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);

lukemakeit mentioned this pull request Oct 4, 2020

1. mysqlbinlog add options: --databases/-L,--flashback-databases,--fl… #1568

Closed

robertbindar self-assigned this Oct 5, 2020

robertbindar requested review from robertbindar and andrelkin October 5, 2020 16:31

robertbindar approved these changes Oct 23, 2020

View reviewed changes

an3l added this to the 10.5 milestone Mar 25, 2021

LinuxJedi self-assigned this Sep 12, 2023

cvicentiu added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Nov 26, 2024

svoj assigned andrelkin and unassigned LinuxJedi and robertbindar Feb 14, 2025

bnestere added the Replication Patches involved in replication label Mar 25, 2025

ParadoxV5 reviewed Mar 30, 2025

View reviewed changes

ParadoxV5 requested changes Apr 3, 2025

View reviewed changes

ParadoxV5 self-assigned this Apr 9, 2025

ParadoxV5 mentioned this pull request Apr 12, 2025

MDEV-23077: --flashback + --table produces buggy results #3968

Open

4 tasks

vuvova closed this Apr 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MDEV-23077: flashbacking the update/delete + join statements has some bugs #1674

MDEV-23077: flashbacking the update/delete + join statements has some bugs #1674

lukemakeit commented Oct 4, 2020

CLAassistant commented Oct 4, 2020 •

edited

Loading

lukemakeit commented Oct 4, 2020

robertbindar left a comment

robertbindar Oct 23, 2020

robertbindar Oct 23, 2020

robertbindar Oct 23, 2020

lukemakeit Dec 8, 2020

andrelkin Dec 11, 2020

ParadoxV5 Mar 30, 2025

an3l commented Mar 25, 2021

ParadoxV5 left a comment

ParadoxV5 Mar 30, 2025

ParadoxV5 Mar 30, 2025 •

edited

Loading

andrelkin Mar 31, 2025

andrelkin commented Mar 31, 2025

ParadoxV5 left a comment •

edited

Loading

ParadoxV5 Apr 3, 2025

andrelkin Apr 3, 2025 •

edited

Loading

ParadoxV5 Apr 3, 2025

vuvova commented Apr 15, 2025

		static std::set<std::string> filter_databases;
		static std::set<std::string> filter_tables;

MDEV-23077: flashbacking the update/delete + join statements has some bugs #1674

MDEV-23077: flashbacking the update/delete + join statements has some bugs #1674

Conversation

lukemakeit commented Oct 4, 2020

CLAassistant commented Oct 4, 2020 • edited Loading

lukemakeit commented Oct 4, 2020

robertbindar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

an3l commented Mar 25, 2021

ParadoxV5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParadoxV5 Mar 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrelkin commented Mar 31, 2025

ParadoxV5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrelkin Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vuvova commented Apr 15, 2025

CLAassistant commented Oct 4, 2020 •

edited

Loading

ParadoxV5 Mar 30, 2025 •

edited

Loading

ParadoxV5 left a comment •

edited

Loading

andrelkin Apr 3, 2025 •

edited

Loading