Bug #38261 Online backup: BACKUP command does implicit commit.
Submitted: 21 Jul 2008 14:37 Modified: 18 Oct 2008 15:47
Reporter: Rafal Somla Email Updates:
Status: Closed Impact on me:
None 
Category:MySQL Server: Backup Severity:S3 (Non-critical)
Version:mysql-6.0-backup OS:Any
Assigned to: Rafal Somla CPU Architecture:Any
Triage: Triaged: D2 (Serious) / R1 (None/Negligible) / E1 (None/Negligible)

[21 Jul 2008 14:37] Rafal Somla
Description:
There is code in backup kernel which performs implicit commit during RESTORE operation (introduced ad fix for BUG#36828). In latest merge, this code was moved to a wrong place (Backup_restore_ctx::close() method) and as a result, it happens also during BACKUP command which is wrong.

How to repeat:
Code inspection.

Suggested fix:
Move the implicit commit inside Backup_restore_ctx::do_restore() method.
[21 Jul 2008 14:38] Rafal Somla
Here is a patch fixing this issue: http://lists.mysql.com/commits/50097
[23 Jul 2008 14:19] Rafal Somla
Fixed synopis.
[24 Jul 2008 11:44] Rafal Somla
Test script showing that BACKUP does implicit commit

Attachment: bug.test (application/octet-stream, text), 43 bytes.

[24 Jul 2008 11:45] Rafal Somla
Test script showing that BACKUP does implicit commit (corrected).

Attachment: bug.test (application/octet-stream, text), 285 bytes.

[24 Jul 2008 11:51] Rafal Somla
REFINED PROBLEM DESCRIPTIONS

See the attached test. After doing BACKUP in a connection, all data changes made in that connection are commited which is wrong.

There are actually 2 reasons for that.

First reason is that an implicit commit, which was originally done only for RESTORE, now is done also after BACKUP. This is because of an incorrect manual merge of the code.

Another reason is that the built-in CS backup driver is starting a consistent snapshot transaction internally. This happens in the same thread which executed BACKUP command. Starting new transaction commits any on-going transactions so the result is that BACKUP commits implicitly if CS driver is used.

Here is the code from be_snapshot.cc which starts the transaction:

result_t Backup::lock()
{
  DBUG_ENTER("Snapshot_backup::lock()");
  /*
    We must fool the locking code to think this is a select because
    any other command type places the engine in a non-consistent read
    state. 
  */
  locking_thd->m_thd->lex->sql_command= SQLCOM_SELECT; 
  locking_thd->m_thd->lex->start_transaction_opt|=
    MYSQL_START_TRANS_OPT_WITH_CONS_SNAPSHOT;
  int res= begin_trans(locking_thd->m_thd);      <====== HERE
  if (res)
    DBUG_RETURN(ERROR);
  m_trans_start= TRUE;
  locking_thd->lock_state= LOCK_ACQUIRED;
  DBUG_ASSERT(locking_thd->m_thd == current_thd);
  DEBUG_SYNC(locking_thd->m_thd, "after_backup_cs_locked");
  DBUG_RETURN(OK);
}
[24 Jul 2008 11:54] Rafal Somla
The patch submitted before fixes the first problem. That is, it moves the implicit commit so that it is done only inside RESTORE, not in BACKUP.

The problem with implicit commit done by CS driver remains unsolved.
[31 Jul 2008 13:18] Rafal Somla
Note: tehere is a new code developed in the server, which does implicit commit at the beginning and end of a statement if appropriate flags are set. When merging with this code, any explicit commit statements should be removed from the backup code.
[1 Aug 2008 8:23] Rafal Somla
On a second thought, I think it is ok and even desirable that BACKUP also performs implicit commit. The point is that it will commit ongoing transaction only in the connection which executes BACKUP. As in this scenario:

BEGIN
<change some data (1)>
BACKUP ...;
<change some data (2)>
COMMIT

If BACKUP does not commit the ongoing transaction, then according to the rules all data changes coming from that transaction will not be included in the backup image. This means that data changes (1) will not be present in the backup image which would probably be not what the user expects.

Instead, the above sequence should be equivalent to

BEGIN
<change some data (1)>
COMMIT
BACKUP ...;
BEGIN
<change some data (2)>
COMMIT

so that changes (1) are included in the backup image. Of course, any transactions running in *other* connections should not be disturbed by BACKUP.
[28 Aug 2008 19:49] Lars Thalmann
1. I think backup and restore should both do implicit commit.  

   By "implicit commit" it is assumed that it is for the *same
   connection* as the BACKUP and RESTORE command.

2. BACKUP/RESTORE should never do commit for any other connection.
[28 Aug 2008 19:58] Davi Arnaut
I guess the fix for this has hit main. As part of WL#4284, a new flag was introduced to signal that certain commands will ensure a implicit command before and after its execution. This flag was add to BACKUP and RESTORE commands.
[3 Sep 2008 9:40] Rafal Somla
TODO

1. Remove any commit statements from backup code - rely on the parser to do implicit commits, marking BACKUP/RESTORE statements with appropriate flags.

2. Extend backup_commit_restore test so that it also checks BACKUP behaviour.
[3 Sep 2008 13:31] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/53164

2688 Rafal Somla	2008-09-03
      BUG#38261 (Online backup: BACKUP command does implicit commit)
      
      It was clarified that it is OK for BACKUP to perform implicit commit, so it 
      is not a problem. Also, the server was modified so that SQL commands marked with 
      CF_AUTO_COMMIT_TRANS flag automatically perform implicit commits at the 
      beginning and at the end of execution. The BACKUP and RESTORE commands are 
      already marked with the flag.
      
      Hence, there is no need for doing any commits inside backup kernel code - calls 
      to ha_autocommit_or_rollback() and end_active_trans() are removed from kernel.cc 
      by this patch.
      
      The backup_commit_restore test is extended to check that also BACKUP does
      an implicit commit. A new test scenario is added to check how BACKUP and
      RESTORE commands interact with transactions running in other connections.
[11 Sep 2008 14:01] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/53827

2688 Rafal Somla	2008-09-11
      BUG#38261 (Online backup: BACKUP command does implicit commit)
            
      It was clarified that it is OK for BACKUP to perform implicit commit, so it 
      is not a problem. Also, the server was modified so that SQL commands marked with 
      CF_AUTO_COMMIT_TRANS flag automatically perform implicit commits at the 
      beginning and at the end of execution. The BACKUP and RESTORE commands are 
      already marked with the flag.
            
      Hence, there is no need for doing any commits inside backup kernel code - calls 
      to ha_autocommit_or_rollback() and end_active_trans() are removed from kernel.cc 
      by this patch.
            
      The patch adds new test backup_commit_backup which tests that BACKUP command 
      performs implicit commit for the current transaction and does not interfere with 
      transactions in other connections.
[12 Sep 2008 17:20] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/53993

2688 Rafal Somla	2008-09-12
      BUG#38261 (Online backup: BACKUP command does implicit commit)
      
      It was clarified that it is OK for BACKUP to perform implicit commit, so it 
      is not a problem. Also, the server was modified so that SQL commands marked with 
      CF_AUTO_COMMIT_TRANS flag automatically perform implicit commits at the 
      beginning and at the end of execution. The BACKUP and RESTORE commands are 
      already marked with the flag.
      
      Hence, there is no need for doing any commits inside backup kernel code - calls 
      to ha_autocommit_or_rollback() and end_active_trans() are removed from kernel.cc 
      by this patch.
      
      The patch adds new test backup_commit_backup which tests that BACKUP command 
      performs implicit commit for the current transaction and does not interfere with 
      transactions in other connections.
[15 Sep 2008 8:00] Jørgen Løland
Good to push
[21 Sep 2008 13:18] Øystein Grøvlen
Patch looks good. Backup tests run successful. Good to push.
[26 Sep 2008 16:10] Rafal Somla
Pushed into mysql-6.0-backup tree.
[17 Oct 2008 17:24] Bugs System
Pushed into 6.0.8-alpha  (revid:rafal.somla@sun.com-20080912171902-syt5bn26wjmxr2qq) (version source revid:oystein.grovlen@sun.com-20080928182351-x4ojdursc9gl8z4o) (pib:5)
[18 Oct 2008 15:47] Paul Dubois
Noted in 6.0.8 changelog.

BACKUP DATABASE now performs an implicit commit, like RESTORE.

Also added BACKUP DATABASE and RESTORE to the list of statements that perform implicit commit:
http://dev.mysql.com/doc/refman/6.0/en/implicit-commit.html