From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f194.google.com (mail-lj1-f194.google.com [209.85.208.194]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 88F68469710 for ; Tue, 19 May 2020 12:08:19 +0300 (MSK) Received: by mail-lj1-f194.google.com with SMTP id z6so1899494ljm.13 for ; Tue, 19 May 2020 02:08:19 -0700 (PDT) Date: Tue, 19 May 2020 12:08:15 +0300 From: Cyrill Gorcunov Message-ID: <20200519090815.GB121099@grain> References: <858f68519d647afe2c4194e52446a31ff5c8e74e.1589804001.git.sergepetrenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <858f68519d647afe2c4194e52446a31ff5c8e74e.1589804001.git.sergepetrenko@tarantool.org> 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: Serge Petrenko Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org 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