Hi Chuck,
Sorry for sitting on it for so long. The patch is approved now.
Chuck Bell wrote:
> Rafal,
>
> All changes made with the following exceptions.
>
> See new patch:
>
> http://lists.mysql.com/commits/46693
>
> Chuck
>
>>> + if (hdl)
>>> + end_tbl_read();
>>> + if (locking_thd)
>>> + locking_thd->kill_locking_thread();
>>> + DBUG_RETURN(OK);
>> Why close_thread_tables() is not called here as it is in the
>> CS driver?
>
> Because the default driver uses the be_thread class to open and lock tables
> in a separate thread. The CS driver does not -- it opens and closes its own
> tables. This was necessary because of the unique requirements of using a
> transaction -- it must be handled in the same thread as the tables in the
> transaction are referenced.
>
Ok, now I recall how it was done.
>
>> Why you don't use kill_locking_thread() method as it is done
>> in the default
>> driver? This method is carefully written to avoid race
>> conditions and other
>> synchronisation issues and I don't think it can be replaced
>> by such simple
>> statement.
>
> See above.
>
Right, we have no locking thread to kill here.
>> Is it correct to kill the table locking thread before the
>> operations below are
>> performed, for example closing the consistent read transaction?
>
> The locking thread isn't used here.
>
>>> + if (m_trans_start)
>>> + {
>>> + ha_autocommit_or_rollback(locking_thd->m_thd, 0);
>>> + end_active_trans(locking_thd->m_thd);
>>> + m_trans_start= FALSE;
>>> + }
>>> + if (tables_open)
>>> + {
>>> + if (hdl)
>>> + default_backup::Backup::end_tbl_read();
>>> + close_thread_tables(locking_thd->m_thd);
>> Why close_thrad_tables() is called here but not in the default driver?
>
> See above.
>
>>> + tables_open= FALSE;
>>> + }
>>> + DBUG_RETURN(OK);
>>> +}
>> Class snapshot_backup::Backup inherits from
>> default_backup::Backup. Thus it
>> would make sense if snapshot_backup::Backup::cleanup() calls
>> default_backup::Backup::cleanup(). This way, whenever we add
>> something to
>> cleanup procedure of the default backup driver we won't
>> forget to do it also in
>> the CS driver. Of course, the CS driver must do some
>> additional cleanup actions
>> like ending the transactions and then call the inherited
>> cleanup() method.
>
> The cleanup() methods have to be different for the CS and default driver
> since one uses the locking thread and the other has a transaction. See
> above.
>
Ok.
>>> private:
>>> my_bool tables_open; ///< Indicates if tables are open
>>> + my_bool m_cancel; ///< Cancel backup
>> Why m_cancel flag if we have CANCEL mode?
>
> Because this cancel is for controlling the CS driver code. The variable mode
> is private to the default driver. The CS driver has its own cleanup() method
> that is different from the default driver's cancel() method.
>
What is private can always be made protected. But ok, this is your code and if
you think you need both CANCEL mode and m_cancel flag I'm fine with that.
Rafal
