[Tarantool-patches] [PATCH v3 2/3] vinyl: clean-up unprocessed read views in *_build_read_views()

[Nikita Pettik]

On 06 May 12:56, Aleksandr Lyapunov wrote:
> This will work, but I think we have to make the patch better.
> 
> The cause of the issue is that the state of an iterator is not obvious. Is
> the history
> collected or not? Is it merged already? Must it be cleaned up? Even the
> cleanup
> is split to two stages - tuple unrefs and region truncate.
> 
> Now you propose to call  vy_read_view_stmt_destroy from several places that
> also
> unrefs tuples from read view (not only from its history). This makes  the
> state
> of an iterator even more unpredictable: when vy_write_iterator_stop is
> called
> sometimes read views are already cleaned up, sometimes are not.

On the other hand clean-up provided by vy_read_view_stmt_destroy()
is safe: it nullifies rv->tuple and rv->history so that next call
of vy_read_view_stmt_destroy() will be no-op in fact.
Anyway, I've refactored this code according to your plan, so
vy_read_view_stmt_destroy() now unrefs only resulting tuple.

> Let's fix this mess.
> 1. History is collected (rallocated and refed) only in
> vy_write_iterator_build_history.

Check.

> 2. The history must be destructed after call to
> vy_write_iterator_build_history in any case,
> whether an error occurred or not (comment about it?).

But histories are used in vy_read_view_merge(). Do you mean destruct all
histories after they are processed (i.e. before region_truncate())?

> 3. There must be a method for history destruction with two steps: unref all
> and rtruncate.
> 4. Only vy_write_iterator_build_read_views must call this method.
> vy_write_iterator_stop
> or anybody else shouldn't be concerned about history.
> 5. vy_write_iterator_build_read_views must not unref resulting tuples
> (entry.stmt) in case of error,
> vy_write_iterator_stop must do it.

Ok, I agree. I've taken these points into consideration while preparing
next version. Please, check it out. Also here's diff between versions:

diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index efb88d1ae..7784dd13a 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -151,9 +151,11 @@ vy_read_view_stmt_destroy(struct vy_read_view_stmt *rv)
        if (rv->tuple != NULL)
                vy_stmt_unref_if_possible(rv->tuple);
        rv->tuple = NULL;
-       if (rv->history != NULL)
-               vy_write_history_destroy(rv->history);
-       rv->history = NULL;
+       /*
+        * History must be already cleaned up in
+        * vy_write_iterator_build_read_views().
+        */
+       assert(rv->history == NULL);
 }
 
 /* @sa vy_write_iterator.h */
@@ -790,11 +792,7 @@ next_lsn:
         * statement around if this is major compaction, because
         * there's no tuple it could overwrite.
         */
-       if (rc != 0) {
-               for (int i = 0; i < stream->rv_count; ++i)
-                       vy_read_view_stmt_destroy(&stream->read_views[i]);
-       } else if (stream->is_last_level &&
-                  stream->deferred_delete_stmt != NULL) {
+       if (stream->is_last_level && stream->deferred_delete_stmt != NULL) {
                vy_stmt_unref_if_possible(stream->deferred_delete_stmt);
                stream->deferred_delete_stmt = NULL;
        }
@@ -952,6 +950,25 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
        return 0;
 }
 
+/**
+ * Clean up all histories related to given write iterator.
+ * Particular history is allocated using region, so single
+ * region truncation is enough to release all memory at once.
+ * Before that we should also unref tuples stored in those
+ * histories (which is done in vy_write_history_destroy()).
+ */
+static void
+vy_write_iterator_history_destroy(struct vy_write_iterator *stream,
+                                 struct region *region, size_t used)
+{
+       for (int i = 0; i < stream->rv_count; ++i) {
+               if (stream->read_views[i].history != NULL)
+                       vy_write_history_destroy(stream->read_views[i].history);
+               stream->read_views[i].history = NULL;
+       }
+       region_truncate(region, used);
+}
+
 /**
  * Split the current key into a sequence of read view
  * statements. @sa struct vy_write_iterator comment for details
@@ -972,9 +989,12 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
        struct region *region = &fiber()->gc;
        size_t used = region_used(region);
        stream->rv_used_count = 0;
+       int rc = 0;
        if (vy_write_iterator_build_history(stream, &raw_count,
-                                           &is_first_insert) != 0)
-               goto error;
+                                           &is_first_insert) != 0) {
+               rc = -1;
+               goto cleanup;
+       }
        if (raw_count == 0) {
                /* A key is fully optimized. */
                region_truncate(region, used);
@@ -996,11 +1016,8 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
                        continue;
                if (vy_read_view_merge(stream, hint, rv,
                                       is_first_insert) != 0) {
-                       while (rv >= &stream->read_views[0]) {
-                               vy_read_view_stmt_destroy(rv);
-                               rv--;
-                       }
-                       goto error;
+                       rc = -1;
+                       goto cleanup;
                }
                assert(rv->history == NULL);
                if (rv->tuple == NULL)
@@ -1009,11 +1026,10 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
                ++*count;
                hint = rv->tuple;
        }
-       region_truncate(region, used);
-       return 0;
-error:
-       region_truncate(region, used);
-       return -1;
+
+cleanup:
+       vy_write_iterator_history_destroy(stream, region, used);
+       return rc;
 }