nirbhay(a)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.
