Mats Kindahl wrote:
> Hi Jason!
>
> I have two comments inline below.
>
> He Zhenxing wrote:
>
> [snip]
>
> > === modified file 'sql/log.cc'
> > --- a/sql/log.cc 2008-07-25 17:21:55 +0000
> > +++ b/sql/log.cc 2008-09-20 09:17:36 +0000
>
> [snip]
>
> > @@ -4048,14 +4053,21 @@ bool MYSQL_BIN_LOG::write(Log_event *eve
> > if (event_info->write(file))
> > goto err;
> >
> > + error=0;
> > if (file == &log_file) // we are writing to the real log (disk)
> > {
> > - if (flush_and_sync())
> > + bool synced;
> > + if (flush_and_sync(&synced))
> > goto err;
>
> If you take the branch here, it will mean that the "err" label will be reached
> with error=0, which indicates that there were no error.
>
right
> > +
> > + if (RUN_HOOK(binlog_storage, after_flush,
> > + (thd, log_file_name, file->pos_in_file, synced))) {
> > + goto err;
>
> Same here.
>
> > + }
> > +
> > signal_update();
> > rotate_and_purge(RP_LOCK_LOG_IS_ALREADY_LOCKED);
> > }
> > - error=0;
> >
> > err:
> > if (error)
>
> Here is the label to handle the error. Note that the above code falls through to
> this label if there were no errors, so that if 'error' is 0, it means that the
> "no error code" is executed. However, by moving the assignment, you have made it
> so that error will always be 0.
>
right, the assignment should not be moved, I don't know how it happened,
but definitely a mistake, thank you for the finding.
> [snip]
>
> > === modified file 'sql/transaction.cc'
> > --- a/sql/transaction.cc 2008-08-12 22:30:55 +0000
> > +++ b/sql/transaction.cc 2008-09-20 09:17:36 +0000
> > @@ -20,6 +20,7 @@
> >
> > #include "transaction.h"
> > #include "mysql_priv.h"
> > +#include "rpl_handler.h"
> >
> > #ifdef WITH_MARIA_STORAGE_ENGINE
> > #include "../storage/maria/ha_maria.h"
> > @@ -192,6 +193,13 @@ bool trans_commit_stmt(THD *thd)
> > int res= FALSE;
> > if (thd->transaction.stmt.ha_list)
> > res= ha_commit_trans(thd, FALSE);
> > + else
> > + {
> > + /* call hooks for statement commit of non-transactional tables,
> > + for transactional tables, the hooks will be called in
> > + ha_commit_trans */
> > + RUN_HOOK(transaction, after_commit, (thd, FALSE));
> > + }
> > DBUG_RETURN(test(res));
> > }
> >
> > @@ -215,6 +223,13 @@ bool trans_rollback_stmt(THD *thd)
> > if (thd->transaction_rollback_request && !thd->in_sub_stmt)
> > ha_rollback_trans(thd, TRUE);
> > }
> > + else
> > + {
> > + /* call hooks for statement rollback of non-transactional tables,
> > + for transactional tables, the hooks will be called in
> > + ha_rollback_trans */
> > + RUN_HOOK(transaction, after_rollback, (thd, FALSE));
> > + }
>
> I agree with Davi: keep the hook calls either in the trans_* functions or in the
> ha_* functions.
>
Agree, I'll move them all to trans_* functions.
> Just my few cents,
> Mats Kindahl
>
