maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

nirbhay@xxxxxxxxxxx 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(&gtid_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(&gtid_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.