From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 E9937469710 for ; Thu, 7 May 2020 03:29:23 +0300 (MSK) Date: Thu, 7 May 2020 00:29:22 +0000 From: Nikita Pettik Message-ID: <20200507002922.GA9992@tarantool.org> References: <1a626bac0c6d0d79934f1e144bb92709f4bd4963.1587948306.git.korablev@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v3 2/3] vinyl: clean-up unprocessed read views in *_build_read_views() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aleksandr Lyapunov Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org 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; }