maria-developers team mailing list archive

[Nirbhay Choubey <nirbhay@xxxxxxxxxxx>]

Hi Kristian,

On Mon, Feb 2, 2015 at 5:06 AM, Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>
wrote:

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

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(&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'.
>

Renamed.


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

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