From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 4DE9C2C207 for ; Tue, 10 Apr 2018 06:45:02 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 9B0MVvCXvQ7b for ; Tue, 10 Apr 2018 06:45:02 -0400 (EDT) Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id DD12628331 for ; Tue, 10 Apr 2018 06:45:01 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations. References: <1f4e0a5e-30f2-f36a-3970-ec4eeb14a456@tarantool.org> From: Vladislav Shpilevoy Message-ID: <4a4c5106-1d2d-553f-c8f1-5c031e91c9ac@tarantool.org> Date: Tue, 10 Apr 2018 13:44:58 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Kirill Shcherbatov , tarantool-patches@freelists.org Hello. See my 19 comments below. 1. Build fails: /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/lua/tuple.c:405:6: error: variable 'new_tuple' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] if (new_data == NULL) { ^~~~~~~~~~~~~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/lua/tuple.c:420:6: note: uninitialized use occurs here if (new_tuple == NULL) ^~~~~~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/lua/tuple.c:405:2: note: remove the 'if' if its condition is always false if (new_data == NULL) { ^~~~~~~~~~~~~~~~~~~~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/lua/tuple.c:409:2: note: variable 'new_tuple' is declared here struct tuple *new_tuple = tuple_new(box_tuple_format_default(), ^ 1 error generated. On 10/04/2018 13:23, Kirill Shcherbatov wrote: > From c268dedd32ed124fb25c46e3fb0bf1cd507fda62 Mon Sep 17 00:00:00 2001 > From: Kirill Shcherbatov > Date: Fri, 6 Apr 2018 19:21:32 +0300 > Subject: [PATCH] Fixed lost format on update and upsert operations. 2. Please, describe a problem, which the patch solves. > > Fixes #3051 > --- > src/box/lua/tuple.c | 31 +++++++++++-- > src/box/memtx_tuple.cc | 1 + > src/box/tuple.c | 31 ++++++++----- > src/box/tuple.h | 2 +- > src/box/tuple_format.h | 5 +- > src/box/vy_stmt.c | 7 +++ > src/box/vy_stmt.h | 9 ++++ > test/box/update.result | 118 +++++++++++++++++++++++++++++++++++++++++++++++ > test/box/update.test.lua | 41 ++++++++++++++++ > 9 files changed, 226 insertions(+), 19 deletions(-) > > diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c > index 47b33c9..9bb62b0 100644 > --- a/src/box/lua/tuple.c > +++ b/src/box/lua/tuple.c > @@ -36,6 +36,7 @@ > #include > #include > #include > +#include 3. Do not put project headers in <>. > > #include "box/tuple.h" > #include "box/tuple_convert.h" > @@ -390,15 +391,35 @@ lbox_tuple_transform(struct lua_State *L) > } > mpstream_flush(&stream); > > - /* Execute tuple_update */ > - struct tuple *new_tuple = > - box_tuple_update(tuple, buf->buf, buf->buf + ibuf_used(buf)); > + uint32_t new_size = 0, bsize; > + const char *old_data = tuple_data_range(tuple, &bsize); > + struct region *region = &fiber()->gc; > + size_t used = region_used(region); > + const char *new_data = tuple_update_execute(region_aligned_alloc_cb, > + region, > + buf->buf, > + buf->buf + ibuf_used(buf), > + old_data, > + old_data + bsize, > + &new_size, 1, NULL); 4. Please, do not put each argument on a separate line when possible. > + if (new_data == NULL) { > + region_truncate(region, used); > + goto error; 5. You can remove goto error, if instead of new_data == NULL with do if new_data != NULL then new_tuple = tuple_new(...) end region_truncate(...) if new_tuple == NULL then lua_error(...) end > + } > + struct tuple *new_tuple = tuple_new(box_tuple_format_default(), > + new_data, new_data + new_size); > + region_truncate(region, used); > if (new_tuple == NULL) > - luaT_error(L); > - /* box_tuple_update() doesn't leak on exception, see public API doc */ > + goto error; > + tuple_bless(new_tuple); 6. AFAIK tuple_bless here is not needed, because lua_pushtuple already does tuple reference count increment. > + > luaT_pushtuple(L, new_tuple); > ibuf_reset(buf); > return 1; > +error: > + if (new_tuple == NULL) > + luaT_error(L); > + return 0; 7. Error label will be removed, but please answer yourself why error label can return non-error code. > } > > /** > diff --git a/src/box/memtx_tuple.cc b/src/box/memtx_tuple.cc > index 70453bd..1990bdf 100644 > --- a/src/box/memtx_tuple.cc > +++ b/src/box/memtx_tuple.cc > diff --git a/src/box/tuple.c b/src/box/tuple.c > index d4760f3..cb57d81 100644 > --- a/src/box/tuple.c > +++ b/src/box/tuple.c > @@ -61,15 +61,19 @@ struct tuple_format *tuple_format_runtime; > static void > runtime_tuple_delete(struct tuple_format *format, struct tuple *tuple); > > +struct tuple * 8. Please, make this function static. > +runtime_tuple_new(struct tuple_format *format, const char *data, const char *end); > + > /** A virtual method table for tuple_format_runtime */ > static struct tuple_format_vtab tuple_format_runtime_vtab = { > runtime_tuple_delete, > + runtime_tuple_new, > }; > > struct tuple * > -tuple_new(struct tuple_format *format, const char *data, const char *end) > +runtime_tuple_new(struct tuple_format *format, const char *data, const char *end) > { > - assert(format->vtab.destroy == tuple_format_runtime_vtab.destroy); > + assert(format->vtab.tuple_delete == tuple_format_runtime_vtab.tuple_delete); > > mp_tuple_assert(data, end); > size_t data_len = end - data; > @@ -102,7 +106,7 @@ tuple_new(struct tuple_format *format, const char *data, const char *end) > static void > runtime_tuple_delete(struct tuple_format *format, struct tuple *tuple) > { > - assert(format->vtab.destroy == tuple_format_runtime_vtab.destroy); > + assert(format->vtab.tuple_delete == tuple_format_runtime_vtab.tuple_delete); > say_debug("%s(%p)", __func__, tuple); > assert(tuple->refs == 0); > size_t total = sizeof(struct tuple) + tuple_format_meta_size(format) + > @@ -147,6 +151,12 @@ tuple_validate_raw(struct tuple_format *format, const char *tuple) > return 0; > } > > +struct tuple * 9. Please make this wrapper be in tuple.h, static inline and add a comment. It is a common practice for our virtual methods wrappers. See key_def.h for examples. > +tuple_new(struct tuple_format *format, const char *data, const char *end) > +{ > + return format->vtab.tuple_new(format, data, end); > +} > + > /** > * Incremented on every snapshot and is used to distinguish tuples > * which were created after start of a snapshot (these tuples can > @@ -396,22 +406,20 @@ box_tuple_t * > box_tuple_update(const box_tuple_t *tuple, const char *expr, > const char *expr_end) > { > - struct tuple_format *format = tuple_format_runtime; > - > uint32_t new_size = 0, bsize; > const char *old_data = tuple_data_range(tuple, &bsize); > struct region *region = &fiber()->gc; > size_t used = region_used(region); > const char *new_data = > tuple_update_execute(region_aligned_alloc_cb, region, expr, > - expr_end, old_data, old_data + bsize, > - &new_size, 1, NULL); > + expr_end, old_data, old_data + bsize, > + &new_size, 1, NULL); 10. Garbage diff. > if (new_data == NULL) { > region_truncate(region, used); > return NULL; > } > - > - struct tuple *ret = tuple_new(format, new_data, new_data + new_size); > + struct tuple *ret = tuple_new(tuple_format(tuple), new_data, > + new_data + new_size); 11. Same. > region_truncate(region, used); > if (ret != NULL) > return tuple_bless(ret); > @@ -422,8 +430,6 @@ box_tuple_t * > box_tuple_upsert(const box_tuple_t *tuple, const char *expr, > const char *expr_end) > { > - struct tuple_format *format = tuple_format_runtime; 12. Same. > - > uint32_t new_size = 0, bsize; > const char *old_data = tuple_data_range(tuple, &bsize); > struct region *region = &fiber()->gc; > @@ -437,7 +443,8 @@ box_tuple_upsert(const box_tuple_t *tuple, const char *expr, > return NULL; > } > > - struct tuple *ret = tuple_new(format, new_data, new_data + new_size); > + struct tuple *ret = tuple_new(tuple_format(tuple), > + new_data, new_data + new_size); 13. Same. > region_truncate(region, used); > if (ret != NULL) > return tuple_bless(ret); > diff --git a/src/box/tuple.h b/src/box/tuple.h > index 6ebedf5..d8138e2 100644 > --- a/src/box/tuple.h > +++ b/src/box/tuple.h > @@ -445,7 +445,7 @@ tuple_delete(struct tuple *tuple) > say_debug("%s(%p)", __func__, tuple); > assert(tuple->refs == 0); > struct tuple_format *format = tuple_format(tuple); > - format->vtab.destroy(format, tuple); > + format->vtab.tuple_delete(format, tuple); > } > > /** > diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h > index d35182d..cc727ec 100644 > --- a/src/box/tuple_format.h > +++ b/src/box/tuple_format.h > @@ -68,7 +68,10 @@ struct tuple_format; > struct tuple_format_vtab { > /** Free allocated tuple using engine-specific memory allocator. */ > void > - (*destroy)(struct tuple_format *format, struct tuple *tuple); > + (*tuple_delete)(struct tuple_format *format, struct tuple *tuple); > + /** Allocates a new tuple on the same allocator and with the same format. */ 14. Out of 66 symbols. > + struct tuple* > + (*tuple_new)(struct tuple_format *format, const char *data, const char *end); 15. Out of 80 symbols. > }; > > /** Tuple field meta information for tuple_format. */ > diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c > index 84182e7..3135c58 100644 > --- a/src/box/vy_stmt.c > +++ b/src/box/vy_stmt.c > @@ -47,6 +47,7 @@ > > struct tuple_format_vtab vy_tuple_format_vtab = { > vy_tuple_delete, > + vy_tuple_new, > }; > > size_t vy_max_tuple_size = 1024 * 1024; > @@ -69,6 +70,12 @@ vy_tuple_delete(struct tuple_format *format, struct tuple *tuple) > free(tuple); > } > > +struct tuple * 16. Static. > +vy_tuple_new(struct tuple_format *format, const char *data, const char *end) > +{ > + return vy_stmt_new_insert(format, data, end); > +} > + > /** > * Allocate a vinyl statement object on base of the struct tuple > * with malloc() and the reference counter equal to 1. > diff --git a/src/box/vy_stmt.h b/src/box/vy_stmt.h > index a33739d..efc99c8 100644 > --- a/src/box/vy_stmt.h > +++ b/src/box/vy_stmt.h > @@ -206,6 +206,15 @@ void > vy_tuple_delete(struct tuple_format *format, struct tuple *tuple); > > /** > + * Create the tuple of a vinyl space. > + * @param format Target tuple format. > + * @param data Target tuple data. > + * @param size Size of the data. > + */ > +struct tuple * > +vy_tuple_new(struct tuple_format *format, const char *data, const char *end); 17. Used in vy_stmt.c only. You can remove it from the header. > diff --git a/test/box/update.test.lua b/test/box/update.test.lua > index c455bc1..1dc1751 100644 > --- a/test/box/update.test.lua > +++ b/test/box/update.test.lua > @@ -269,3 +269,44 @@ t:update({{'=', 3, map}}) > s:update(1, {{'=', 3, map}}) > > s:drop() > + > +-- > +-- gh-3051 Lost format while tuple update > +-- > +format = {} > +format[1] = {name = 'KEY', type = 'unsigned'} > +format[2] = {name = 'VAL', type = 'string'} > +s = box.schema.create_space('tst_sample', {format = format}) > +pk = s:create_index('pk') > +s:insert({1, 'sss'}) > +aa = box.space.tst_sample:get(1) > +aa.VAL > +aa = aa:update({{'=',2,'ssss'}}) > +collectgarbage() 18. Even if a tuple leaks, this code does not detect it. Either create a correct check via Lua weak references (see __mode = 'k'/'v' and net.box.test.lua), or remove this code. > +aa.VAL > +s:drop() > +aa = nil > +collectgarbage() > + > +s = box.schema.space.create('tst_sample', {engine = 'vinyl', format = format}) 19. When you need multiple engines for the same test, you must use engine/ test suite. > +pk = s:create_index('pk') > +s:insert({1, 'sss'}) > +aa = box.space.tst_sample:get(1) > +aa.VAL > +aa = aa:update({{'=',2,'ssss'}}) > +collectgarbage() > +aa.VAL > +s:drop() > +aa = nil > +collectgarbage() > + > +-- test transform integrity > +space = box.schema.space.create('tweedledum') > +tmp = space:create_index('primary', { type = 'hash', parts = {1, 'string'}, unique = true }) > +t = space:insert{'1', '2', '3', '4', '5', '6', '7'} > +t:transform(-1, 1) > +n = 2000 > +tab = {}; for i=1,n,1 do table.insert(tab, i) end > +t = box.tuple.new(tab) > +t:transform(1, n - 1) > +t = nil >