Hi Kristian, On Mon, Feb 2, 2015 at 5:06 AM, Kristian Nielsen <[email protected]> wrote:
> [email protected] 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. >
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
