Re: bzr commit into mysql-6.0-backup branch (rsomla:2672)

[Øystein Grøvlen]

Rafal,

Here are my comments/questions to this patch:

  - You have removed the test:
        if (m_thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))
    Is not that a necessary test?

  - When compiling, I get this warning:
         kernel.cc:943: warning: control reaches end of non-void function

  - Why commit twice?  I do not understand why you want to commit
    before creating triggers and events.

  - There is nothing backup/restore specific about the code in
    commit_changes.  Hence, it does not seem to me that
    Backup_restore_ctx is the right class for this code.

  - That this issue was not detected before the code was pushed,
    indicates that a test is missing.  I think such a test should be
    added as part of this patch.

--
Øystein

Rafal Somla wrote:
 > #At file:///ext/mysql/bzr/mysql-6.0-backup/
 >
 >  2672 Rafal Somla	2008-07-21
 >       Post-merge fix.
 >
 >       During merge the code was changed so that implicit commit of 
transaction is done
 >       inside Backup_restore_ctx::close().This means that the implicit 
commit was done also
 >       for BACKUP command which is wrong - only RESTORE command should 
commit the new data
 >       it restores.
 >
 >       This patch moves implicit commit back into 
Backup_restore_ctx::do_restore().
 > modified:
 >   sql/backup/backup_kernel.h
 >   sql/backup/kernel.cc
 >
 > per-file messages:
 >   sql/backup/backup_kernel.h
 >     Add new helper method Backup_restore_ctx::commit_changes().
 >   sql/backup/kernel.cc
 >     - Remove impicit commit from Backup_restore_ctx::close().
 >     - Implement commit_changes() method of Backup_restore_ctx.
 >     - Add commits in Backup_restore_ctx::do_restore().
 > === modified file 'sql/backup/backup_kernel.h'
 > --- a/sql/backup/backup_kernel.h	2008-07-07 12:51:56 +0000
 > +++ b/sql/backup/backup_kernel.h	2008-07-21 08:00:07 +0000
 > @@ -122,6 +122,7 @@ class Backup_restore_ctx: public backup:
 >
 >    int lock_tables_for_restore();
 >    int unlock_tables();
 > +  int commit_changes();
 >
 >    friend class Backup_info;
 >    friend class Restore_info;
 >
 > === modified file 'sql/backup/kernel.cc'
 > --- a/sql/backup/kernel.cc	2008-07-09 08:19:03 +0000
 > +++ b/sql/backup/kernel.cc	2008-07-21 08:00:07 +0000
 > @@ -732,15 +732,6 @@ int Backup_restore_ctx::close()
 >
 >    time_t when= my_time(0);
 >
 > -  // If auto commit is turned off, be sure to commit the transaction
 > -  // TODO: move it to the big switch, case: MYSQLCOM_BACKUP?
 > -
 > -  if (m_thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))
 > -  {
 > -    ha_autocommit_or_rollback(m_thd, 0);
 > -    end_active_trans(m_thd);
 > -  }
 > -
 >    // unlock tables if they are still locked
 >
 >    unlock_tables();
 > @@ -945,6 +936,12 @@ int Backup_restore_ctx::restore_triggers
 >
 >    @todo Remove the @c reset_diagnostic_area() hack.
 >  */
 > +int Backup_restore_ctx::commit_changes()
 > +{
 > +  ha_autocommit_or_rollback(m_thd, 0);
 > +  end_active_trans(m_thd);
 > +}
 > +
 >  int Backup_restore_ctx::do_restore()
 >  {
 >    DBUG_ENTER("do_restore");
 > @@ -994,9 +991,16 @@ int Backup_restore_ctx::do_restore()
 >
 >    unlock_tables();
 >
 > +  /*
 > +    We don't commit changes in case of error to increase chances of 
leaving
 > +    server in a consistent state.
 > +  */
 >    if (err)
 >      DBUG_RETURN(ER_BACKUP_RESTORE);
 >
 > +  // Commit new table data before creating triggers and events.
 > +  commit_changes();
 > +
 >    /*
 >     Re-create all triggers and events (it was not done in @c 
bcat_create_item()).
 >
 > @@ -1007,6 +1011,9 @@ int Backup_restore_ctx::do_restore()
 >    if (restore_triggers_and_events())
 >       DBUG_RETURN(ER_BACKUP_RESTORE);
 >
 > +  // Final commit (perhaps redundant but should not hurt).
 > +  commit_changes();
 > +
 >    DBUG_PRINT("restore",("Done."));
 >
 >    if (read_summary(info, s))
 >
 >


-- 
Øystein Grøvlen, Senior Staff Engineer
Sun Microsystems, Database Group
Trondheim, Norway