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

Nickolay,

The patch looks generally good to me, but I have a bunch of relatively minor comments:

   - the patch makes ARCHIVED a reserved keyword (so, for example,
     "CREATE TABLE archived(...)" succeeds in upstream server, but fails
     in PS). Please add ARCHIVED_SYM to the keyword_sp rule in sql_yacc.yy

   - innobase_purge_archive_logs() and purge_archived_logs() do
     something odd. So innobase_purge_archive_logs() takes the point in
     time up to which logs must be purged as its 'before_date'
     argument. Basically, all we need to do is to traverse all files in
     the archived logs directory and delete those with mtime <
     before_date, right? Here's what actually happens:

     - innobase_purge_archive_logs() takes the current time, subtracts
       before_date and passes that as the expire_sec argument to
       purge_archived_logs()
     - expire_sec argument is unsigned. So if you specify a point in
       time in the future, i.e. "PURGE ARCHIVED LOGS BEFORE NOW() +
       INTERVAL 1 MONTH" an overflow occurs and unexpected things may
       happen depending on sizeof(time_t) in the OS
     - purge_archived_logs() iterates the log files, takes their mtime,
       adds expire_sec and compares it with _current_ time again. So the
       exact cut-off time is drifting, which may become a problem when
       file deletion takes a long time (which is quite possible)
     - on a side note, purge_archive_logs() skips all files if
       expired_sec is 0. Why?

     So why not just pass before_date to purge_archived_logs() and
     compare mtime with it directly?

   - I think fil_swap_file() does too much unnecessary work on POSIX
     systems. It flushes the file being swapped, then waits for flush to
     complete, then closes the file, just because allegedly "we do not
     trust the operating systems can rename an open file". Well, they
     actually can. It is in the POSIX standard. Windows can do that too,
     but that requires a special flag when opening the file. Since
     Windows is not a priority for us, I suggest enclosing the
     unnecessary for POSIX code in #ifdef __WIN__. We can fix Windows
     later and remove that code in fil_swap_file() completely.

   - fil_swap_file() could use some descriptive comments for the
     function itself and its arguments. Also, fil_rotate_file() would
     probably be a bit more descriptive name.

   - innobase_log_archive_update() has a race condition. The entire
     function body must be protected by log_sys->archived_mutex,
     otherwise multiple concurrent threads calling that function can
     result in multiple log archive threads being created.

   - in innobase_log_archive_update() and
     innobase_log_archive_expire_update() you show update *var_ptr
     rather than innodb_* variables directly. The comment "where the
     formal string goes" in the var_ptr comment looks to be copy-pasted
     and not applicable to those 2 functions?

   - please don't use the Yoda notation, as in "if (FALSE ==
     log_xtra_log_archive_on)"

   - please use OS_FILE_MAX_PATH instead of 10000 in
     purge_archived_logs() and srv_log_archive_thread()

   - there are 2 open_or_create_new_log_file() calls in
     srv_log_archive_thread(): one right before the main loop, and
     another one at the start of the main loop. I think the first one is
     unnecessary.

   - in innobase_init() you assign values of innobase_xtra_log_archive to
     log_xtra_log_archive_on and innodb_xtra_expire_archive_logs_sec to
     log_xtra_log_archive_on. That's not necessary, since you have the
     *_update() functions.

   - in srv_log_archive_thread() there must be a buffer overflow
     check/assertion before copying log_xtra_log_archive_dir to
     final_file_name

   - the event names in log_sys are very confusing. I suggest renaming
     archive_thread to arch_event_rotate_start and archive_event to
     arch_event_rotate_done. This way their purpose would be
     self-explaining.

   - "ib_log_archive_" occurs 6 times in the code. Please consider
     adding a #define constant, e.g.:

     #define IB_ARCHIVED_LOGS_PREFIX "ib_log_archive_"

   - C++ style comment in C code:

     // move innobase_xtra_log_archive_directory

   - I see that SLEEPs are still used in the test case. As I said, I
     don't think it's a good idea, but let's discuss it separately.