From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp21.mail.ru (smtp21.mail.ru [94.100.179.250]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A33D24696C3 for ; Fri, 10 Apr 2020 18:47:14 +0300 (MSK) Date: Fri, 10 Apr 2020 15:47:13 +0000 From: Nikita Pettik Message-ID: <20200410154713.GE9428@tarantool.org> References: <73e1f0baf18ec008312d91db4449447b3c06aa86.1586381297.git.korablev@tarantool.org> <67dd4512-2e87-b9f1-79d4-b7d182c0634e@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <67dd4512-2e87-b9f1-79d4-b7d182c0634e@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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);