From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 AA6654696C4 for ; Fri, 10 Apr 2020 18:13:16 +0300 (MSK) References: <73e1f0baf18ec008312d91db4449447b3c06aa86.1586381297.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <67dd4512-2e87-b9f1-79d4-b7d182c0634e@tarantool.org> Date: Fri, 10 Apr 2020 17:13:14 +0200 MIME-Version: 1.0 In-Reply-To: <73e1f0baf18ec008312d91db4449447b3c06aa86.1586381297.git.korablev@tarantool.org> Content-Type: multipart/mixed; boundary="------------742A3FDE204D091798986B39" Content-Language: en-US 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: Nikita Pettik , tarantool-patches@dev.tarantool.org This is a multi-part message in MIME format. --------------742A3FDE204D091798986B39 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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. 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? --------------742A3FDE204D091798986B39 Content-Type: text/plain; charset=UTF-8; x-mac-type="0"; x-mac-creator="0"; name="patch.txt" Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="patch.txt" ZGlmZiAtLWdpdCBhL3NyYy9ib3gvdnlfd3JpdGVfaXRlcmF0b3IuYyBiL3NyYy9ib3gvdnlf d3JpdGVfaXRlcmF0b3IuYwppbmRleCBmNmU2ZWQ0ZDIuLjdkOGM2MGQ3MyAxMDA2NDQKLS0t IGEvc3JjL2JveC92eV93cml0ZV9pdGVyYXRvci5jCisrKyBiL3NyYy9ib3gvdnlfd3JpdGVf aXRlcmF0b3IuYwpAQCAtNzkwLDggKzc5MCwxMSBAQCBuZXh0X2xzbjoKIAkgKiBzdGF0ZW1l bnQgYXJvdW5kIGlmIHRoaXMgaXMgbWFqb3IgY29tcGFjdGlvbiwgYmVjYXVzZQogCSAqIHRo ZXJlJ3Mgbm8gdHVwbGUgaXQgY291bGQgb3ZlcndyaXRlLgogCSAqLwotCWlmIChyYyA9PSAw ICYmIHN0cmVhbS0+aXNfbGFzdF9sZXZlbCAmJgotCSAgICBzdHJlYW0tPmRlZmVycmVkX2Rl bGV0ZV9zdG10ICE9IE5VTEwpIHsKKwlpZiAocmMgIT0gMCkgeworCQlmb3IgKGludCBpID0g MDsgaSA8IHN0cmVhbS0+cnZfY291bnQ7ICsraSkKKwkJCXN0cmVhbS0+cmVhZF92aWV3c1tp XS5oaXN0b3J5ID0gTlVMTDsKKwl9IGVsc2UgaWYgKHN0cmVhbS0+aXNfbGFzdF9sZXZlbCAm JgorCQkgICBzdHJlYW0tPmRlZmVycmVkX2RlbGV0ZV9zdG10ICE9IE5VTEwpIHsKIAkJdnlf c3RtdF91bnJlZl9pZl9wb3NzaWJsZShzdHJlYW0tPmRlZmVycmVkX2RlbGV0ZV9zdG10KTsK IAkJc3RyZWFtLT5kZWZlcnJlZF9kZWxldGVfc3RtdCA9IE5VTEw7CiAJfQpAQCAtOTYxLDEx ICs5NjQsOCBAQCB2eV93cml0ZV9pdGVyYXRvcl9idWlsZF9yZWFkX3ZpZXdzKHN0cnVjdCB2 eV93cml0ZV9pdGVyYXRvciAqc3RyZWFtLCBpbnQgKmNvdW50KQogCXNpemVfdCB1c2VkID0g cmVnaW9uX3VzZWQocmVnaW9uKTsKIAlzdHJlYW0tPnJ2X3VzZWRfY291bnQgPSAwOwogCWlm ICh2eV93cml0ZV9pdGVyYXRvcl9idWlsZF9oaXN0b3J5KHN0cmVhbSwgJnJhd19jb3VudCwK LQkJCQkJICAgICZpc19maXJzdF9pbnNlcnQpICE9IDApIHsKLQkJZm9yIChpbnQgaSA9IDA7 IGkgPCBzdHJlYW0tPnJ2X2NvdW50OyArK2kpCi0JCQlzdHJlYW0tPnJlYWRfdmlld3NbaV0u aGlzdG9yeSA9IE5VTEw7CisJCQkJCSAgICAmaXNfZmlyc3RfaW5zZXJ0KSAhPSAwKQogCQln b3RvIGVycm9yOwotCX0KIAlpZiAocmF3X2NvdW50ID09IDApIHsKIAkJLyogQSBrZXkgaXMg ZnVsbHkgb3B0aW1pemVkLiAqLwogCQlyZWdpb25fdHJ1bmNhdGUocmVnaW9uLCB1c2VkKTsK --------------742A3FDE204D091798986B39--