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

[Serge Petrenko <sergepetrenko@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