- the commit message does not describe the changes done to
xtrabackup.c.
- please make static the variables and functions that are local to
xtrabackup.c.
- I don't think table_hash_entries_num is required. Its role can
be handled by tables_hash != NULL check.
- Likewise for database_hash_entries_num.
- use my_malloc(.., MYF(MY_FAE)); instead of plain malloc and its
result check in xb_new_filter_entry(). Then also the return
value of xb_new_filter_entry() itself needs not to be checked.
- please assert in xb_new_filter_entry() that the name does not
violate MySQL name limits: 64 chars for db name, 64 chars for
table name, and a dot.
- Line 243: missing || xtrabackup_database_file? Shouldn't this
have caused a crash for the new ib_databases_file.sh test? I'd
replace this by lazy initialization of hash tables that actually
get any insertions.
- Lines 243--248: why both hashes are created unconditionally? I
think the logic should be: init table hash, if any of table or db
options given; init db hash, if any of db options given.
- Lines 244 and 246: redundant comments.
- Line 255: why strtok_r instead of strtok? I don't think the
reentrancy is needed here?
- The hash insert code is duplicated for the xtrabackup_databases
and x_d_file hash insertion in xb_filters_init(), please make a
function.
- Please validate the provided names in xb_filter_init against
length.
- How are duplicate names handled in the hash tables?
Testcases:
- The existing testcases test the IB options and how IB works with
XB fine. But I think that an XB-specific testcase is necessary
as well. It should:
- test the XB options;
- test invalid input (too long table and database names, non
existing tables and databases, illegal characters, duplicate
filters, etc).
Documentation:
- diff lines 8 and 9: use "either ... or" instead of "whether", s/-
or a/, or a.
- diff line 43: cannot parse "names" in "Only names databases and tables will
be backed up."
Code:
- the commit message does not describe the changes done to
xtrabackup.c.
- please make static the variables and functions that are local to
xtrabackup.c.
- I don't think table_hash_ entries_ num is required. Its role can
be handled by tables_hash != NULL check.
- Likewise for database_ hash_entries_ num.
- use my_malloc(.., MYF(MY_FAE)); instead of plain malloc and its filter_ entry() . Then also the return filter_ entry() itself needs not to be checked.
result check in xb_new_
value of xb_new_
- please assert in xb_new_ filter_ entry() that the name does not
violate MySQL name limits: 64 chars for db name, 64 chars for
table name, and a dot.
- Line 243: missing || xtrabackup_ database_ file? Shouldn't this file.sh test? I'd
have caused a crash for the new ib_databases_
replace this by lazy initialization of hash tables that actually
get any insertions.
- Lines 243--248: why both hashes are created unconditionally? I
think the logic should be: init table hash, if any of table or db
options given; init db hash, if any of db options given.
- Lines 244 and 246: redundant comments.
- Line 255: why strtok_r instead of strtok? I don't think the
reentrancy is needed here?
- The hash insert code is duplicated for the xtrabackup_ databases
and x_d_file hash insertion in xb_filters_init(), please make a
function.
- Please validate the provided names in xb_filter_init against
length.
- How are duplicate names handled in the hash tables?
Testcases:
- The existing testcases test the IB options and how IB works with
XB fine. But I think that an XB-specific testcase is necessary
as well. It should:
- test the XB options;
- test invalid input (too long table and database names, non
existing tables and databases, illegal characters, duplicate
filters, etc).
Documentation:
- diff lines 8 and 9: use "either ... or" instead of "whether", s/-
or a/, or a.
- diff line 43: cannot parse "names" in "Only names databases and tables will
be backed up."