[Tarantool-patches] [PATCH v4 3/4] memtx: allow quota overuse for truncation and deletion
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat Feb 15 02:50:07 MSK 2020
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;
More information about the Tarantool-patches
mailing list