Code review comment for lp:~ihanick/percona-server/5.1-innodb-log_archiving

Nickolay,

I didn't review everything. Just thought it would be more efficient to do it iteratively.

Generally, I first suggest trying to make a debug build with your patch (I had a few issues see below), and then trying to run the percona_log_archiving test (it crashes due to an assertion failure, see below).

So the comments I have so far:

   - percona_server_variables_debug.result should also be adjusted
   - compiler warning in srv0srv.c. I suggest replacing

    if(expire_sec <= 0 || (ut_time() - fileinfo.mtime) <= expire_sec) {
  with
    if (ut_time() <= (ib_time_t)
        (fileinfo.mtime + expire_sec)) {
   - there is a compile-time assertion in debug builds, because the
     patch introduces 2 new SQLCOM_ commands, but does not add the
     corresponding SHOW STATUS variables to com_status_vars.
   - there are unused 'protocol' variables in ha_purge_archive_logs()
     and ha_purge_archive_logs_to()
   - after fixing the above issues with debug builds, I got a runtime
     assertion failure when trying to run the percona_log_archive
     test. The reason is that PURGE ARCHIVED LOGS sends neither OK nor
     error packet to the client. You should either call my_ok() and set
     res to FALSE in mysql_execute_command() or call my_error() and set
     res to TRUE, see purge_master_logs() for an example
   - there are main.percona_log_archiving failures in release builds in
     http://jenkins.percona.com/view/Percona%20Server%205.1/job/percona-server-5.1-param/254/#showFailuresLink
   - I think it's better to add the test to the innodb_plugin suite
     rather than main one
   - a more common way to pass test-specific options to mysqld is an
     .opt file, not a .cnf file. there are lots of examples in existing
     tests
   - the test case does "drop table if exists t1;" but then creates and
     uses table with the name 't'
   - some unnecessary changes:

  > @@ -645,5 +646,5 @@
  > { "TRIM", SYM(TRIM)},
  > { "VARIANCE", SYM(VARIANCE_SYM)},
  > { "VAR_POP", SYM(VARIANCE_SYM)},
  > - { "VAR_SAMP", SYM(VAR_SAMP_SYM)},
  > + { "VAR_SAMP", SYM(VAR_SAMP_SYM)}
  > };
  >

  and

  > @@ -4560,6 +4560,8 @@
  > return FALSE;
  > }
  >
  > +
  > +
  > bool ha_show_status(THD *thd, handlerton *db_type, enum ha_stat_type stat)
  > {
  > List<Item> field_list;
  >

  and

  > @@ -10709,9 +10710,12 @@ select_var_ident:
  > ;
  >
  > purge_options:
  > + ARCHIVED_SYM LOGS_SYM purge_archive_option
  > + |
  > master_or_binary LOGS_SYM purge_option
  > ;
  >
  > +
  > purge_option:
  > TO_SYM TEXT_STRING_sys
  > {
  >

   - please remove commented code, e.g.:

  > + //res = purge_master_logs(thd, lex->to_log);
  > + //fprintf(stderr, "purge archive logs command called %s\n", lex->to_log);
  >

   - SLEEPs in test cases almost never work. They definitely don't work
     for synchronization, but unfortunately there's no way to
     synchronize between mtr and InnoDB, especially when a background
     InnoDB thread has to be executed synchronously. Which means we have
     to modify tests to remove SLEEP()s possibly at the cost of test
     coverage. But I'm not quite sure what exactly is being tested there.
     We can discuss this further on IRC.