On Thu, Mar 06, 2008 at 07:30:33PM +0200, Sanja Byelkin wrote:
> 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.
Operations on 8 bytes are not atomic on all CPUs, there are
exceptions. And there can be CPU caching effects.
> 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.
Ok, I am super-confused now.
I realize that you read the variable under log_flush_lock and
without translog_lock(), while you set it to 0 under translog_lock(),
i.e. read and set using two different mutexes, I need to check the
logic more (you talked about it above, but I have to think more).
Without thinking, I'd protect the variable (reading and setting) just
with translog_lock(), that would solve any potential issue.
And there is also the problem that the log_flush_lock will be removed
in the future, according to a comment in ma_loghandler.c.
