On Thu, Mar 06, 2008 at 06:16:56PM +0100, Guilhem Bichot wrote:
[skip]
> > > > @@ -5743,6 +5748,7 @@ static my_bool translog_write_fixed_reco
> > > > }
> > > >
> > > > *lsn= log_descriptor.horizon;
> > > > + log_descriptor.is_everything_flushed= 0;
> > > > if (translog_set_lsn_for_files(LSN_FILE_NO(*lsn),
> LSN_FILE_NO(*lsn),
> > > > *lsn, TRUE) ||
> > > > (log_record_type_descriptor[type].inwrite_hook &&
> > >
> > > Instead of the three places above, why not put
> > > log_descriptor.is_everything_flushed= 0;
> > > only at the start of translog_write_record() ?
> > > If the three functions above are always called via
> > > translog_write_record(), and *only* via it (is it true?), it sounds
> > > safe to do.
> >
> > It is right way. we have nothing to flush untill we get new LSN. for
> > simple cases like fixed length record it does not matter (i.e. it is the
> > same because done under the same log lock) but for multigroup records it
> > is important (setting it is the begining is too early (we unlock/lock
> > the log several times), so I decide to do it where LSN appers for all
> > record types.
>
> Ahah, it is a thread-safety issue, ok.
>
> Then, a suggestion (feel free to ignore it):
> to make sure that a future dev, if he codes a variant of one of the
> three functions above, sets *lsn and does not forget to set
> log_descriptor.is_everything_flushed= 0;
> (if dev forgot that, it would be a serious bug: translog_flush() would
> do nothing!), how about linking "setting *lsn" and "setting
> log_descriptor.is_everything_flushed to 0" together with an inline
> function (or a macro if you prefer):
>
> static inline void set_lsn(LSN *lsn, LSN value)
> {
> // maybe assert the right mutex (safe_mutex_assert_owner)
> *lsn= value;
> /* we generate LSN so something is not flushed in log */
> log_descriptor.is_everything_flushed= 0;
> }
> and use this function in the above three functions?
> Assuming these three functions are used as a model by the dev, he may
> this way copy the function's call and thus do the right thing?
>
> I also suggest to put a comment near the declaration of
> is_everything_flushed (in the struct):
> /**
> Must be set to 0 under mutex (which one?) every time a record is
> written or a LSN is generated (Sanja please fix this bad comment)
> */
>
> which makes a second safety net for the developer.
Ok, I agree.
About mutexes. Actually it is 8 byte => operations are atomic. We drop the
flag before sending LSN to upper leven => nobody can use it for flushing
in the same moment and all previous flushes done (in other case
assignment do nothing). So we protects about only "flush everything
commands" for such commands IMHO it is ok if in next minisecoonds after
it appered something to flush. But "totally accidentally" we drop the
flag when have loghandler locked, and check it under serialization mutex
lock just to avoid potential problems.
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Mr. Oleksandr Byelkin <sanja@stripped>
/ /|_/ / // /\ \/ /_/ / /__ MySQL AB, Full-Time Developer
/_/ /_/\_, /___/\___\_\___/ Lugansk, Ukraine
<___/ www.mysql.com
