Re: [Maria-developers] [Commits] 101d7eb: MDEV-4987: Sort by domain_id when list of GTIDs are output
nirbhay@mariadb.com writes:
revision-id: 101d7eb963816514362da8a98fc7db7135181910 committer: Nirbhay Choubey timestamp: 2015-01-31 21:48:14 -0500 message:
MDEV-4987: Sort by domain_id when list of GTIDs are output
Added logic to sort gtid list based on domain_id before populating them in string. Added a test case.
Thanks! The patch looks ok to push after fixing / considering below in-line comments:
diff --git a/mysql-test/suite/rpl/t/rpl_gtid_sort.test b/mysql-test/suite/rpl/t/rpl_gtid_sort.test new file mode 100644 index 0000000..d3b8c6d --- /dev/null +++ b/mysql-test/suite/rpl/t/rpl_gtid_sort.test
+--connection server_2 +SHOW VARIABLES LIKE '%GTID%'; +#SET GLOBAL gtid_slave_pos="";
This commented-out SET looks like some left-over, probably you meant to remove it?
diff --git a/sql/rpl_gtid.cc b/sql/rpl_gtid.cc index e5620ec..a0471f3 100644 --- a/sql/rpl_gtid.cc +++ b/sql/rpl_gtid.cc @@ -247,11 +247,13 @@ { my_hash_init(&hash, &my_charset_bin, 32, offsetof(element, domain_id), sizeof(uint32), NULL, rpl_slave_state_free_element, HASH_UNIQUE); + my_init_dynamic_array(>id_cache, sizeof(rpl_gtid), 8, 8, MYF(0));
The name 'cache' confused me slightly, as I think of a cache as something that stores some state between invocations. But it is actually just a temporary buffer used for sorting. A suggestion for a better name could be 'gtid_sort_array'.
rpl_slave_state::~rpl_slave_state() { + delete_dynamic(>id_cache); }
The other parts of rpl_slave_state are freed in rpl_slave_state::deinit(). I think this is because there is a global variable of this class, and destructors in global variables can cause various issues. So better put this delete_dynamic() into deinit() as well (for consistency, if nothing else).
@@ -705,7 +707,13 @@ class Gtid_db_intact : public Table_check_intact return sub_id; }
+/* A callback used in sorting of gtid list based on domain_id. */ +static int rpl_gtid_cmp_cb(const void *id1, const void *id2) +{ + return (((rpl_gtid *)id1)->domain_id - ((rpl_gtid *)id2)->domain_id); +}
I think this is wrong, you can get signed/unsigned integer overflow (eg. it will consider 0xffffffff as being less than 0x00000000). We might be able to do the subtraction in longlong with a cast (though that would still be susceptible to overflow when converting to the int return value, right?), but it's probably clearer to just do: uint32 d1= ((rpl_gtid *)id1)->domain_id; uint32 d2= ((rpl_gtid *)id2)->domain_id; if (d1 < d2) return -1; else if (d1 > f2) return 1; else return 0; You should probably also modify the test case to have a domain id > 0x80000000, just to check. Otherwise looks good, thanks! - Kristian.
Hi Kristian,
On Mon, Feb 2, 2015 at 5:06 AM, Kristian Nielsen
nirbhay@mariadb.com writes:
revision-id: 101d7eb963816514362da8a98fc7db7135181910 committer: Nirbhay Choubey timestamp: 2015-01-31 21:48:14 -0500 message:
MDEV-4987: Sort by domain_id when list of GTIDs are output
Added logic to sort gtid list based on domain_id before populating them in string. Added a test case.
Thanks! The patch looks ok to push after fixing / considering below in-line comments:
diff --git a/mysql-test/suite/rpl/t/rpl_gtid_sort.test b/mysql-test/suite/rpl/t/rpl_gtid_sort.test new file mode 100644 index 0000000..d3b8c6d --- /dev/null +++ b/mysql-test/suite/rpl/t/rpl_gtid_sort.test
+--connection server_2 +SHOW VARIABLES LIKE '%GTID%'; +#SET GLOBAL gtid_slave_pos="";
This commented-out SET looks like some left-over, probably you meant to remove it?
Yes, it was a leftover, removed.
diff --git a/sql/rpl_gtid.cc b/sql/rpl_gtid.cc index e5620ec..a0471f3 100644 --- a/sql/rpl_gtid.cc +++ b/sql/rpl_gtid.cc @@ -247,11 +247,13 @@ { my_hash_init(&hash, &my_charset_bin, 32, offsetof(element, domain_id), sizeof(uint32), NULL, rpl_slave_state_free_element, HASH_UNIQUE); + my_init_dynamic_array(>id_cache, sizeof(rpl_gtid), 8, 8, MYF(0));
The name 'cache' confused me slightly, as I think of a cache as something that stores some state between invocations. But it is actually just a temporary buffer used for sorting.
A suggestion for a better name could be 'gtid_sort_array'.
Renamed.
rpl_slave_state::~rpl_slave_state() { + delete_dynamic(>id_cache); }
The other parts of rpl_slave_state are freed in rpl_slave_state::deinit(). I think this is because there is a global variable of this class, and destructors in global variables can cause various issues.
So better put this delete_dynamic() into deinit() as well (for consistency, if nothing else).
I agree, done.
@@ -705,7 +707,13 @@ class Gtid_db_intact : public Table_check_intact return sub_id; }
+/* A callback used in sorting of gtid list based on domain_id. */ +static int rpl_gtid_cmp_cb(const void *id1, const void *id2) +{ + return (((rpl_gtid *)id1)->domain_id - ((rpl_gtid *)id2)->domain_id); +}
I think this is wrong, you can get signed/unsigned integer overflow (eg. it will consider 0xffffffff as being less than 0x00000000).
We might be able to do the subtraction in longlong with a cast (though that would still be susceptible to overflow when converting to the int return value, right?), but it's probably clearer to just do:
uint32 d1= ((rpl_gtid *)id1)->domain_id; uint32 d2= ((rpl_gtid *)id2)->domain_id; if (d1 < d2) return -1; else if (d1 > f2) return 1; else return 0;
You should probably also modify the test case to have a domain id > 0x80000000, just to check.
You are right. It's been modified now. I have also added some related tests to check this. I originally thought of pushing it to 10.1 (10.1.4). Do you want me to push it to 10.0 as well? Best, Nirbhay
Otherwise looks good, thanks!
- Kristian.
Nirbhay Choubey
I originally thought of pushing it to 10.1 (10.1.4). Do you want me to push it to 10.0 as well?
Hm. I guess it's best to push only to 10.1? - Kristian.
participants (2)
-
Kristian Nielsen -
Nirbhay Choubey