[Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Apr 10 18:13:14 MSK 2020
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?
-------------- next part --------------
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);
More information about the Tarantool-patches
mailing list