Code review comment for lp:~sergei.glushchenko/percona-xtrabackup/xb20-databases

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

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

review: Needs Fixing

« Back to merge proposal