From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f65.google.com (mail-lf1-f65.google.com [209.85.167.65]) (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 65B57469710 for ; Mon, 25 May 2020 18:13:30 +0300 (MSK) Received: by mail-lf1-f65.google.com with SMTP id u16so8921592lfl.8 for ; Mon, 25 May 2020 08:13:30 -0700 (PDT) Date: Mon, 25 May 2020 18:13:27 +0300 From: Cyrill Gorcunov Message-ID: <20200525151327.GB2464@grain> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v2 2/2] wal: reorder tx rows so that a tx ends on a global row 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 25, 2020 at 01:58:56PM +0300, Serge Petrenko wrote: ... > diff --git a/src/box/wal.c b/src/box/wal.c > index ef4d84920..0921c7261 100644 > --- a/src/box/wal.c > +++ b/src/box/wal.c > @@ -958,6 +958,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 = row; > + struct xrow_header **last_glob_row = end - 1; > /** Assign LSN to all local rows. */ > for ( ; row < end; row++) { > if ((*row)->replica_id == 0) { > @@ -971,6 +972,7 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base, > */ > if ((*row)->group_id != GROUP_LOCAL) { > (*row)->replica_id = instance_id; > + last_glob_row = row; > } > > (*row)->lsn = vclock_inc(vclock_diff, (*row)->replica_id) + > @@ -987,7 +989,7 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base, > first_glob_row = row; > } > (*row)->tsn = tsn == 0 ? (*start)->lsn : tsn; > - (*row)->is_commit = row == end - 1; > + (*row)->is_commit = false; > } else { > int64_t diff = (*row)->lsn - vclock_get(base, (*row)->replica_id); > if (diff <= vclock_get(vclock_diff, > @@ -1013,6 +1015,21 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base, > */ > for (row = start; row < first_glob_row; row++) > (*row)->tsn = tsn; > + > + /* > + * If a mixed transaction ends on a local row, float up > + * the last global row, so that the upper tx boundary is > + * delivered to the replica. > + */ > + if (last_glob_row < end - 1) { > + struct xrow_header *tmp = *last_glob_row; > + memmove(last_glob_row, last_glob_row + 1, > + (end - 1 - last_glob_row) * > + sizeof(struct xrow_header *)); > + *(end - 1) = tmp; > + } Sergey, I see no errors here still this memove call somehow worries me, can't explain why, some gut feeling. Maybe because we're mangling arguments. That said while it fixes the issue (and we really need this fix asap) Reviewed-by: Cyrill Gorcunov but maybe we should come back to this code a bit later and rethink it. > + > + (*(end - 1))->is_commit = true; > }