Hello,
On Thu, Mar 06, 2008 at 05:51:55PM +0200, Sanja Byelkin wrote:
> Hi!
>
> On Thu, Mar 06, 2008 at 02:02:12PM +0100, Guilhem Bichot wrote:
> > Hello Sanja,
> >
> > I have questions:
> >
> > On Thu, Mar 06, 2008 at 08:31:40AM +0200, sanja@stripped wrote:
> > > ChangeSet@stripped, 2008-03-06 08:31:35+02:00, bell@stripped
> +1 -0
> > > Avoiding changing log on flush call
> > > if nothing was added to the log (BUG#34712)
> >
> > If possible, add the bug's title too.
> >
> > > storage/maria/ma_loghandler.c@stripped, 2008-03-06 08:31:33+02:00,
> bell@stripped +19 -1
> > > Flag that signaling about "everythig-flushed" state of
> > > the log added, to avoid changing log on flush request
> > > if nothing was written but horizon placed on the page boarder.
> > >
> > > diff -Nrup a/storage/maria/ma_loghandler.c b/storage/maria/ma_loghandler.c
> > > --- a/storage/maria/ma_loghandler.c 2008-02-29 13:11:24 +02:00
> > > +++ b/storage/maria/ma_loghandler.c 2008-03-06 08:31:33 +02:00
> > > @@ -297,6 +297,7 @@ struct st_translog_descriptor
> > > pthread_mutex_t purger_lock;
> > > /* last low water mark checked */
> > > LSN last_lsn_checked;
> > > + my_bool is_everything_flushed;
> > > };
> > >
> > > static struct st_translog_descriptor log_descriptor;
> > > @@ -3771,6 +3772,7 @@ my_bool translog_init_with_table(const c
> > > It is beginning of the log => there is no LSNs in the log
> =>
> > > There is no harm in leaving it "as-is".
> > > */
> > > + log_descriptor.is_everything_flushed= 1;
> > > DBUG_RETURN(0);
> > > }
> > > file_no--;
> > > @@ -3846,7 +3848,7 @@ my_bool translog_init_with_table(const c
> > > translog_free_record_header(&rec);
> > > }
> > > }
> > > -
> > > + log_descriptor.is_everything_flushed= 1;
> > > DBUG_RETURN(0);
> > > }
> >
> > Is it possible to just put
> > log_descriptor.is_everything_flushed= 1;
> > at the start of translog_init_with_table() (log manager starts so it's
> > clear it has nothing to flush) ?
>
> Actually you are right, if initialization failed nobody will check the
> variable.
good
> > > @@ -4632,6 +4634,7 @@ translog_write_variable_record_1group(LS
> > > translog_buffer_lock_assert_owner(buffer_to_flush);
> > >
> > > *lsn= horizon= 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 &&
> > > @@ -4782,6 +4785,7 @@ translog_write_variable_record_1chunk(LS
> > > header_length,
> chunk0_header);
> > >
> > > *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 &&
> > > @@ -5469,6 +5473,7 @@ translog_write_variable_record_mgroup(LS
> > > {
> > > first_chunk0= 0;
> > > *lsn= horizon;
> > > + log_descriptor.is_everything_flushed= 0;
> > > if (log_record_type_descriptor[type].inwrite_hook &&
> > > (*log_record_type_descriptor[type].inwrite_hook) (type, trn,
> > > tbl_info,
> > > @@ -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.
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Mr. Guilhem Bichot <guilhem@stripped>
/ /|_/ / // /\ \/ /_/ / /__ MySQL AB, Lead Software Engineer
/_/ /_/\_, /___/\___\_\___/ Bordeaux, France
<___/ www.mysql.com
