Re: [Tarantool-patches] [PATCH 1/2] wal: fix tx boundaries

[Cyrill Gorcunov <gorcunov@gmail.com>]

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