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.
> >
> > @@ -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.
> > @@ -7292,6 +7298,11 @@ my_bool translog_flush(TRANSLOG_ADDRESS
> > LINT_INIT(sent_to_disk);
> >
> > pthread_mutex_lock(&log_descriptor.log_flush_lock);
> > + if (log_descriptor.is_everything_flushed)
> > + {
> > + DBUG_PRINT("info", ("everything is flushed"));
> > + goto out;
> > + }
> > translog_lock();
> > flush_horizon= LSN_IMPOSSIBLE;
> > old_flushed= log_descriptor.flushed;
> > @@ -7338,7 +7349,14 @@ my_bool translog_flush(TRANSLOG_ADDRESS
> > then was at the moment of start flushing);
> > */
> > if (buffer_start == log_descriptor.bc.buffer_no)
> > + {
> > + /*
> > + We are going to flush last buffer, and will not release
> > + log_flush_lock until it happened, so we can set the flag here
> > + */
> > + log_descriptor.is_everything_flushed= 1;
> > translog_force_current_buffer_to_finish();
> > + }
> > }
> > break;
> > }
>
> Thanks for enlightening me.
> And by the way, the patch above does fix the bug, I tested.
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Mr. Oleksandr Byelkin <sanja@stripped>
/ /|_/ / // /\ \/ /_/ / /__ MySQL AB, Full-Time Developer
/_/ /_/\_, /___/\___\_\___/ Lugansk, Ukraine
<___/ www.mysql.com
