Code review comment for lp:~akopytov/percona-server/backup-locks

    If time is short, this may go in as-is and the review comments can
    be addressed in a follow-up as needed.

    core:

    - Is the binlogging of "ROLLBACK" statement properly protected,
      i.e. does the ordered_commit call in MYSQL_BIN_LOG::rollback
      need the protection too? The testcase would be: start
      transaction, write to both InnoDB and MyISAM tables, ROLLBACK.

    - I was not able to figure out why do we need binlog lock
      protection in Rotate_log_event::do_update_pos?

    - Nor I was able to figure out why dispatch_command added

@@ -1877,6 +1877,14 @@

   THD_STAGE_INFO(thd, stage_cleaning_up);

+ if (thd->killed == THD::KILL_QUERY ||
+ thd->killed == THD::KILL_TIMEOUT ||
+ thd->killed == THD::KILL_BAD_DATA)
+ {
+ thd->killed= THD::NOT_KILLED;
+ thd->mysys_var->abort= 0;
+ }
+
   thd->reset_query();
   thd->set_command(COM_SLEEP);

    - Dead store error= 1 in the case of
      backup_binlog_lock.acquire_protection failure.

    - Indentation error at sql_parse.cc line 4233

    parser:

    - The parser changes cause "unlock" as a statement label to be
      treated as ER_PARSE_ERROR instead ER_SP_BADSTATEMENT, as
      recorded in funcs_1/storedproc. Which leaves the if
      (lex->sphead) ... in unlock symbol rule as dead code. Is that
      intentional? It is possible to avoid this by rewriting it as

unlock:
          UNLOCK_SYM
          {
            LEX *lex= Lex;

            if (lex->sphead)
            {
              my_error(ER_SP_BADSTATEMENT, MYF(0), "UNLOCK");
              MYSQL_YYABORT;
            }
          }
          unlock_variant
          {}
 ;

unlock_variant:
          BINLOG_SYM
          {
            Lex->sql_command= SQLCOM_UNLOCK_BINLOG;
          }
        | table_or_tables
          {
            Lex->sql_command= SQLCOM_UNLOCK_TABLES;
          }
        ;

      which is also a bit more localized change against trunk.

    mysqldump:

    - might be good to include a note in docs that --lock-for-backup
      always disables --lock-tables (by --single-transaction if
      specified or by --lock-for-backup converted to --lock-all-tables
      otherwise).

    - It looks like mysqldump will issue an unbalanced UNLOCK TABLES
      if --single-transaction is the only option given. This appears
      to be benign and pre-existing.

    MTR:

    - possible to do s/STOP SLAVE;--source
      include/wait_for_slave_to_stop.inc/--source
      include/stop_slave.inc, likewise for START SLAVE, unless for
      some reason you prefer the current form?

    - It is of course true that there are many possible test
      combinations that could be added to the testcases. I tried to
      list a selected few that are single-threaded and IMHO required:

    - Need a test that checks for a LTFB/LBFB failure if issued by a
      user without RELOAD privilege.

    - Need a test for crash-safe slave: slave server binlog on, log
      slave updates off, --master-info-repository=TABLE,
      --relay-log-info-repository=TABLE, LOCK BINLOG on the slave,
      verify that updates from the master are not blocked.

    - I was not able to find a test where a single connection would
      take either backup lock, and attempt to issue FTWRL.

    - Some cases which would be interesting to test, but IMHO less
      value than above:

    - I am not too familiar with MTS slave, but I see MTS
      coordination calls next to binlog protection in
      Rotate_log_event::do_update_pos. Is it worthwhile to add a
      simple MTS testcase?

    - EVENTs. Both that CREATE EVENT is blocked on mysql.event or
      its corresponding binlogging, and that firing EVENT obeys the
      locks.

    - XA transactions and more statements that write to mysql schema:
      CREATE USER, GRANT, etc.