From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 B424B4696C3 for ; Sat, 15 Feb 2020 02:50:11 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: Date: Sat, 15 Feb 2020 00:50:07 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v4 3/4] memtx: allow quota overuse for truncation and deletion List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ilya Kosarev , tarantool-patches@dev.tarantool.org Thanks for the patch! See 7 comments below. On 14/02/2020 20:39, Ilya Kosarev wrote: > Trying to perform space:truncate() and space:delete() while reaching > memtx_memory limit we could experience slab allocator failure. This > behavior seems to be quite surprising for users. Now we are allowing > to overuse memtx quota if needed for truncation or deletion, using flag > in struct quota. After performing truncation or deletion we reset the > flag so that quota can't be overused any more. > > Closes #3807 > --- > src/box/box.cc | 27 ++++++++++++++++++++++++++- > src/box/memtx_space.c | 18 ++++++++++++++---- > 2 files changed, 40 insertions(+), 5 deletions(-) > > diff --git a/src/box/box.cc b/src/box/box.cc > index 1b2b27d61..1a63f25a2 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1321,9 +1321,34 @@ space_truncate(struct space *space) > ops_buf_end = mp_encode_uint(ops_buf_end, 1); > assert(ops_buf_end < buf + buf_size); 1. Please, add a comment why do you open a transaction. > + struct txn *txn = NULL; > + struct txn_savepoint *txn_svp = NULL; > + if (!box_txn()) { > + txn = txn_begin(); > + if (txn == NULL) > + diag_raise(); > + } else { > + txn_svp = box_txn_savepoint(); > + if (txn_svp == NULL) > + diag_raise(); > + } > + struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID); > + quota_on_off(&((struct memtx_engine *)truncate_space->engine)->quota, false); 2. This line is out of 80 symbols. This is a very simple rule. If a line is longer than 80 symbols, then it is not correct. Please, customize your text editor. Every editor has an option to show so called 'rulers' on certain columns. I set mine to 80, 66, and 50 for commit titles. Then you will see too long lines before sending a patch. This helps a lot. 3. You repeat this super long thing 3 times: &((struct memtx_engine *)truncate_space->engine)->quota in one function. Wouldn't it be better to save it once into a 'struct quota *' variable and use it? > if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end, > - ops_buf, ops_buf_end, 0, NULL) != 0) > + ops_buf, ops_buf_end, 0, NULL) != 0) { > + quota_on_off(&((struct memtx_engine *)truncate_space->engine)->quota, true); 4. Now when I see quota_on_off() in action I think the name is really confusing. For example: quota_on_off(true) What does it mean? True means 'on' or 'off'? I propose to make two functions: quota_enable() and quota_disable(), with one 'struct quota *' argument. > + if (txn != NULL) > + txn_rollback(txn); > + else > + box_txn_rollback_to_savepoint(txn_svp); > + fiber_gc(); > diag_raise(); > + } > + quota_on_off(&((struct memtx_engine *)truncate_space->engine)->quota, true); > + if (txn != NULL && txn_commit(txn) != 0) { > + fiber_gc(); > + diag_raise(); > + } > } > > int > diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c > index 6ef84e045..3542450f7 100644 > --- a/src/box/memtx_space.c > +++ b/src/box/memtx_space.c > @@ -28,6 +28,7 @@ > * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > * SUCH DAMAGE. > */ > +#include "exception.h" > #include "memtx_space.h" > #include "space.h" > #include "iproto_constants.h" > @@ -354,10 +355,19 @@ memtx_space_execute_delete(struct space *space, struct txn *txn, > struct tuple *old_tuple; > if (index_get(pk, key, part_count, &old_tuple) != 0) > return -1; > - if (old_tuple != NULL && > - memtx_space->replace(space, old_tuple, NULL, > - DUP_REPLACE_OR_INSERT, &stmt->old_tuple) != 0) > - return -1; > + if (old_tuple != NULL) { > + bool oom = !diag_is_empty(diag_get()) && > + diag_last_error(diag_get())->type == &type_OutOfMemory; 5. Please, rename to 'is_oom'. For flags we always use 'is_/has_/...' prefix. Except some places, where it was missed at review stage. 6. This line is also out of 80 symbols. And two lines below about quota enabling/disabling. 7. I am not sure what is happening here. You check for 'oom' *before* doing actual change? Why?! An oom can happen only below, in memtx_space->replace(). > + if (oom) > + quota_on_off(&((struct memtx_engine *)space->engine)->quota, false); > + int rc = memtx_space->replace(space, old_tuple, NULL, > + DUP_REPLACE_OR_INSERT, > + &stmt->old_tuple); > + if (oom) > + quota_on_off(&((struct memtx_engine *)space->engine)->quota, true); > + if (rc != 0) > + return -1; > + } > stmt->engine_savepoint = stmt; > *result = stmt->old_tuple; > return 0;