Re: [Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails

[Nikita Pettik <korablev@tarantool.org>]

On 10 Apr 17:13, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 2 comments below.
> 
> > diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> > index 7a6a20627..f6e6ed4d2 100644
> > --- a/src/box/vy_write_iterator.c
> > +++ b/src/box/vy_write_iterator.c
> > @@ -961,8 +961,11 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
> >  	size_t used = region_used(region);
> >  	stream->rv_used_count = 0;
> >  	if (vy_write_iterator_build_history(stream, &raw_count,
> > -					    &is_first_insert) != 0)
> > +					    &is_first_insert) != 0) {
> > +		for (int i = 0; i < stream->rv_count; ++i)
> > +			stream->read_views[i].history = NULL;
> >  		goto error;
> 
> 1. Kostja probably meant encapsulation of function
> vy_write_iterator_build_history(). Currently that function
> leaves garbage in case of a fail. Therefore it becomes not
> self sufficient. Better cleanup the views in the same place
> where they are created - inside build_history().
> 
> Imagine, if build_history() would be called not in a single
> place. We would need to cleanup its garbage in each one.

Ah, of course I agree. Applied.
 
> Diff below and attached as a file.
> ====================
> diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> index f6e6ed4d2..7d8c60d73 100644
> --- a/src/box/vy_write_iterator.c
> +++ b/src/box/vy_write_iterator.c
> @@ -790,8 +790,11 @@ next_lsn:
>  	 * statement around if this is major compaction, because
>  	 * there's no tuple it could overwrite.
>  	 */
> -	if (rc == 0 && stream->is_last_level &&
> -	    stream->deferred_delete_stmt != NULL) {
> +	if (rc != 0) {
> +		for (int i = 0; i < stream->rv_count; ++i)
> +			stream->read_views[i].history = NULL;
> +	} else if (stream->is_last_level &&
> +		   stream->deferred_delete_stmt != NULL) {
>  		vy_stmt_unref_if_possible(stream->deferred_delete_stmt);
>  		stream->deferred_delete_stmt = NULL;
>  	}
> @@ -961,11 +964,8 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
>  	size_t used = region_used(region);
>  	stream->rv_used_count = 0;
>  	if (vy_write_iterator_build_history(stream, &raw_count,
> -					    &is_first_insert) != 0) {
> -		for (int i = 0; i < stream->rv_count; ++i)
> -			stream->read_views[i].history = NULL;
> +					    &is_first_insert) != 0)
>  		goto error;
> -	}
>  	if (raw_count == 0) {
>  		/* A key is fully optimized. */
>  		region_truncate(region, used);
> ====================
> 
> 2. I rolled back the patch and run the test - it passed. What can be a reason?

Hm, it fails not each run, but like every second/third execution.
I guess it depends on state of memory which has been truncated
(position of particular slabs in area etc). Not sure if we can
affect it.

> diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> index f6e6ed4d2..7d8c60d73 100644
> --- a/src/box/vy_write_iterator.c
> +++ b/src/box/vy_write_iterator.c
> @@ -790,8 +790,11 @@ next_lsn:
>  	 * statement around if this is major compaction, because
>  	 * there's no tuple it could overwrite.
>  	 */
> -	if (rc == 0 && stream->is_last_level &&
> -	    stream->deferred_delete_stmt != NULL) {
> +	if (rc != 0) {
> +		for (int i = 0; i < stream->rv_count; ++i)
> +			stream->read_views[i].history = NULL;
> +	} else if (stream->is_last_level &&
> +		   stream->deferred_delete_stmt != NULL) {
>  		vy_stmt_unref_if_possible(stream->deferred_delete_stmt);
>  		stream->deferred_delete_stmt = NULL;
>  	}
> @@ -961,11 +964,8 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
>  	size_t used = region_used(region);
>  	stream->rv_used_count = 0;
>  	if (vy_write_iterator_build_history(stream, &raw_count,
> -					    &is_first_insert) != 0) {
> -		for (int i = 0; i < stream->rv_count; ++i)
> -			stream->read_views[i].history = NULL;
> +					    &is_first_insert) != 0)
>  		goto error;
> -	}
>  	if (raw_count == 0) {
>  		/* A key is fully optimized. */
>  		region_truncate(region, used);