From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 9A825469710 for ; Tue, 19 May 2020 13:30:11 +0300 (MSK) References: <858f68519d647afe2c4194e52446a31ff5c8e74e.1589804001.git.sergepetrenko@tarantool.org> <20200519090815.GB121099@grain> From: Serge Petrenko Message-ID: <6b83f635-bd42-9a49-bfbd-69778f749d12@tarantool.org> Date: Tue, 19 May 2020 13:30:10 +0300 MIME-Version: 1.0 In-Reply-To: <20200519090815.GB121099@grain> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: ru Subject: Re: [Tarantool-patches] [PATCH 1/2] wal: fix tx boundaries List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org 19.05.2020 12:08, Cyrill Gorcunov пишет: > On Mon, May 18, 2020 at 03:24:04PM +0300, Serge Petrenko wrote: > ... >> --- a/src/box/wal.c >> +++ b/src/box/wal.c >> @@ -956,24 +956,33 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base, >> struct xrow_header **end) >> { >> int64_t tsn = 0; >> + struct xrow_header **start = row; >> + struct xrow_header **first_glob_row = end; >> /** Assign LSN to all local rows. */ >> for ( ; row < end; row++) { >> if ((*row)->replica_id == 0) { >> /* >> * All rows representing local space data >> - * manipulations are signed wth a zero >> + * manipulations are signed with a zero >> * instance id. This is also true for >> * anonymous replicas, since they are >> * only capable of writing to local and >> * temporary spaces. >> */ >> - if ((*row)->group_id != GROUP_LOCAL) >> + if ((*row)->group_id != GROUP_LOCAL) { >> (*row)->replica_id = instance_id; >> + } >> >> (*row)->lsn = vclock_inc(vclock_diff, (*row)->replica_id) + >> vclock_get(base, (*row)->replica_id); >> - /* Use lsn of the first local row as transaction id. */ >> - tsn = tsn == 0 ? (*row)->lsn : tsn; >> + /* >> + * Use lsn of the first global row as >> + * transaction id. >> + */ >> + if ((*row)->group_id != GROUP_LOCAL && tsn == 0) { >> + tsn = (*row)->lsn; >> + first_glob_row = row; >> + } >> (*row)->tsn = tsn; > 1) ^^^ > >> (*row)->is_commit = row == end - 1; >> } else { >> @@ -993,6 +1002,15 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base, >> } >> } >> } >> + if (tsn == 0) >> + tsn = (*start)->lsn; >> + /* >> + * Fill transaction id for all the local rows preceding >> + * the first global row. tsn was yet unknown when those >> + * rows were processed. >> + */ >> + for (row = start; row < first_glob_row; row++) >> + (*row)->tsn = tsn; >> } > Wait, the chunk above -- lets assume we've all rows in GROUP_LOCAL thus > in (1) we already set this member to 0 and now we re-walk the entries > again and assign lsn to the first row's lsn. This is ugly as hell, > if only I didn't miss something obvious. > > Can't we do something like below to eliminate needless rewalk over > all rows? Do I make a mistake? > --- > diff --git a/src/box/wal.c b/src/box/wal.c > index d36d4259e..ef4d84920 100644 > --- a/src/box/wal.c > +++ b/src/box/wal.c > @@ -957,7 +957,7 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base, > { > int64_t tsn = 0; > struct xrow_header **start = row; > - struct xrow_header **first_glob_row = end; > + struct xrow_header **first_glob_row = row; > /** Assign LSN to all local rows. */ > for ( ; row < end; row++) { > if ((*row)->replica_id == 0) { > @@ -981,9 +981,12 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base, > */ > if ((*row)->group_id != GROUP_LOCAL && tsn == 0) { > tsn = (*row)->lsn; > + /* > + * Remember the tail being processed. > + */ > first_glob_row = row; > } > - (*row)->tsn = tsn; > + (*row)->tsn = tsn == 0 ? (*start)->lsn : tsn; > (*row)->is_commit = row == end - 1; > } else { > int64_t diff = (*row)->lsn - vclock_get(base, (*row)->replica_id); > @@ -1002,8 +1005,7 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base, > } > } > } > - if (tsn == 0) > - tsn = (*start)->lsn; > + > /* > * Fill transaction id for all the local rows preceding > * the first global row. tsn was yet unknown when those Hi! Thanks for the review! You're right, I applied your diff. -- Serge Petrenko