From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations. References: <1f4e0a5e-30f2-f36a-3970-ec4eeb14a456@tarantool.org> <4a4c5106-1d2d-553f-c8f1-5c031e91c9ac@tarantool.org> <8a909d97-3f64-207d-fe25-a4462f814d38@tarantool.org> <0f5103aa-cbd6-8ccb-326d-51875e0f561d@tarantool.org> <42c7e5e1-2803-f3f7-6c7e-11c778399fd9@tarantool.org> <20180418122842.pkpq3mktqwjobppm@esperanza> From: Kirill Shcherbatov Message-ID: <18a8749f-ba4e-61a8-a58d-12257f2d0f32@tarantool.org> Date: Wed, 18 Apr 2018 15:55:40 +0300 MIME-Version: 1.0 In-Reply-To: <20180418122842.pkpq3mktqwjobppm@esperanza> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit To: tarantool-patches@freelists.org Cc: Vladimir Davydov , "v.shpilevoy@tarantool.org" List-ID: > Why is tuple.transform so special? Why can't it derive the original > tuple format, like tuple.update and tuple.upsert? Transform should use runtime format. Vlad insisted on to do not redesign public API to contain format, just only in-line required code. > This doesn't check "transform integrity" AFAICS - the test passes even > if we use the original tuple's format instead of runtime_tuple_format. It's an integrity check to ensure that transform method hasn't degraded. The functional path part below: >From b97bc4639cb2a6814ff2ca739cc9e678e4addbe8 Mon Sep 17 00:00:00 2001 Message-Id: In-Reply-To: References: From: Kirill Shcherbatov Date: Fri, 6 Apr 2018 19:21:32 +0300 Subject: [PATCH 2/2] tuple: fix lost format on update and upsert operations The format was lost when performing update operations as new tuple was referenced to default runtime format. Fixes #3051 --- src/box/lua/tuple.c | 21 +++++++++++--- src/box/memtx_tuple.cc | 1 + src/box/tuple.c | 23 ++++++++------- src/box/tuple.h | 10 ++++--- src/box/tuple_format.h | 14 +++++++-- src/box/vy_stmt.c | 7 +++++ test/engine/update.result | 71 +++++++++++++++++++++++++++++++++++++++++++++ test/engine/update.test.lua | 27 +++++++++++++++++ 8 files changed, 153 insertions(+), 21 deletions(-) diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c index 47b33c9..566cbec 100644 --- a/src/box/lua/tuple.c +++ b/src/box/lua/tuple.c @@ -29,6 +29,7 @@ * SUCH DAMAGE. */ #include "box/lua/tuple.h" +#include "box/tuple_update.h" #include "lua/utils.h" /* luaT_error() */ #include "lua/msgpack.h" /* luamp_encode_XXX() */ @@ -390,12 +391,24 @@ 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); + struct tuple *new_tuple = NULL; + 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); + if (new_data != NULL) + 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 */ + luaT_pushtuple(L, new_tuple); ibuf_reset(buf); return 1; 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 @@ -90,6 +90,7 @@ memtx_tuple_free(void) struct tuple_format_vtab memtx_tuple_format_vtab = { memtx_tuple_delete, + memtx_tuple_new, }; struct tuple * diff --git a/src/box/tuple.c b/src/box/tuple.c index d4760f3..6f89fe5 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); +static struct tuple * +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) +static struct tuple * +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) + @@ -396,8 +400,6 @@ 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; @@ -410,8 +412,8 @@ box_tuple_update(const box_tuple_t *tuple, const char *expr, 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); region_truncate(region, used); if (ret != NULL) return tuple_bless(ret); @@ -422,8 +424,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; - uint32_t new_size = 0, bsize; const char *old_data = tuple_data_range(tuple, &bsize); struct region *region = &fiber()->gc; @@ -437,7 +437,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); 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..d119909 100644 --- a/src/box/tuple.h +++ b/src/box/tuple.h @@ -430,10 +430,12 @@ tuple_extra(const struct tuple *tuple) * \param end the end of \a data * \retval tuple on success * \retval NULL on out of memory - * \sa \code box.tuple.new(data) \endcode */ -struct tuple * -tuple_new(struct tuple_format *format, const char *data, const char *end); +static inline struct tuple * +tuple_new(struct tuple_format *format, const char *data, const char *end) +{ + return format->vtab.tuple_new(format, data, end); +} /** * Free the tuple of any engine. @@ -445,7 +447,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..5121cba 100644 --- a/src/box/tuple_format.h +++ b/src/box/tuple_format.h @@ -66,9 +66,19 @@ struct tuple_format; /** Engine-specific tuple format methods. */ struct tuple_format_vtab { - /** Free allocated tuple using engine-specific memory allocator. */ + /** + * 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. + */ + struct tuple* + (*tuple_new)(struct tuple_format *format, const char *data, + const char *end); }; /** Tuple field meta information for tuple_format. */ diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c index 84182e7..8035cc6 100644 --- a/src/box/vy_stmt.c +++ b/src/box/vy_stmt.c @@ -45,8 +45,15 @@ #include "xrow.h" #include "fiber.h" +static struct tuple * +vy_tuple_new(struct tuple_format *format, const char *data, const char *end) +{ + return vy_stmt_new_insert(format, data, end); +} + struct tuple_format_vtab vy_tuple_format_vtab = { vy_tuple_delete, + vy_tuple_new, }; size_t vy_max_tuple_size = 1024 * 1024; diff --git a/test/engine/update.result b/test/engine/update.result index 653ebec..0ae22ac 100644 --- a/test/engine/update.result +++ b/test/engine/update.result @@ -683,3 +683,74 @@ space:select{} space:drop() --- ... +-- +-- gh-3051 Lost format while tuple update +-- +format = {} +--- +... +format[1] = {name = 'KEY', type = 'unsigned'} +--- +... +format[2] = {name = 'VAL', type = 'string'} +--- +... +s = box.schema.space.create('tst_sample', {engine = engine, format = format}) +--- +... +pk = s:create_index('pk') +--- +... +s:insert({1, 'sss', '3', '4', '5', '6', '7'}) +--- +- [1, 'sss', '3', '4', '5', '6', '7'] +... +aa = box.space.tst_sample:get(1) +--- +... +aa.VAL +--- +- sss +... +aa = aa:update({{'=',2,'ssss'}}) +--- +... +aa.VAL +--- +- ssss +... +-- invalid update +aa:update({{'=',2, 666}}) +--- +- error: 'Tuple field 2 type does not match one required by operation: expected string' +... +-- test transform integrity +aa:transform(-1, 1) +--- +- [1, 'ssss', '3', '4', '5', '6'] +... +aa = nil +--- +... +s:upsert({2, 'wwwww'}, {{'=', 2, 'wwwww'}}) +--- +... +box.space.tst_sample:get(2).VAL +--- +- wwwww +... +s:upsert({2, 'wwwww2'}, {{'=', 2, 'wwwww2'}}) +--- +... +box.space.tst_sample:get(2).VAL +--- +- wwwww2 +... +-- invalid upsert +s:upsert({2, 666}, {{'=', 2, 666}}) +--- +- error: 'Tuple field 2 type does not match one required by operation: expected string' +... +s:drop() +--- +... diff --git a/test/engine/update.test.lua b/test/engine/update.test.lua index 0252b8a..3ff7e62 100644 --- a/test/engine/update.test.lua +++ b/test/engine/update.test.lua @@ -94,3 +94,30 @@ space:select{} space:update({2}, {}) space:select{} space:drop() + +-- +-- gh-3051 Lost format while tuple update +-- +format = {} +format[1] = {name = 'KEY', type = 'unsigned'} +format[2] = {name = 'VAL', type = 'string'} +s = box.schema.space.create('tst_sample', {engine = engine, format = format}) +pk = s:create_index('pk') +s:insert({1, 'sss', '3', '4', '5', '6', '7'}) +aa = box.space.tst_sample:get(1) +aa.VAL +aa = aa:update({{'=',2,'ssss'}}) +aa.VAL +-- invalid update +aa:update({{'=',2, 666}}) +-- test transform integrity +aa:transform(-1, 1) +aa = nil + +s:upsert({2, 'wwwww'}, {{'=', 2, 'wwwww'}}) +box.space.tst_sample:get(2).VAL +s:upsert({2, 'wwwww2'}, {{'=', 2, 'wwwww2'}}) +box.space.tst_sample:get(2).VAL +-- invalid upsert +s:upsert({2, 666}, {{'=', 2, 666}}) +s:drop() -- 2.7.4 On 18.04.2018 15:28, Vladimir Davydov wrote: > On Mon, Apr 16, 2018 at 08:14:10PM +0300, Vladislav Shpilevoy wrote: >> Vova, can you please take a look on the patch? > > Pasting here the latest version of the patch for review. > >> From b97bc4639cb2a6814ff2ca739cc9e678e4addbe8 Mon Sep 17 00:00:00 2001 >> From: Kirill Shcherbatov >> Date: Fri, 6 Apr 2018 19:21:32 +0300 >> Subject: [PATCH] tuple: fix lost format on update and upsert operations >> >> The format was lost when performing update operations as new tuple >> was referenced to default runtime format. >> >> Fixes #3051 >> >> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c >> index 47b33c90..566cbec1 100644 >> --- a/src/box/lua/tuple.c >> +++ b/src/box/lua/tuple.c >> @@ -390,12 +391,24 @@ 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); >> + struct tuple *new_tuple = NULL; >> + 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); >> + if (new_data != NULL) >> + new_tuple = tuple_new(box_tuple_format_default(), >> + new_data, new_data + new_size); >> + region_truncate(region, used); >> + > > Why is tuple.transform so special? Why can't it derive the original > tuple format, like tuple.update and tuple.upsert? > >> diff --git a/test/engine/update.result b/test/engine/update.result >> index 653ebec2..0ae22ac8 100644 >> --- a/test/engine/update.result >> +++ b/test/engine/update.result >> @@ -683,3 +683,74 @@ space:select{} >> space:drop() >> --- >> ... >> +-- >> +-- gh-3051 Lost format while tuple update >> +-- >> +format = {} >> +--- >> +... >> +format[1] = {name = 'KEY', type = 'unsigned'} >> +--- >> +... >> +format[2] = {name = 'VAL', type = 'string'} >> +--- >> +... >> +s = box.schema.space.create('tst_sample', {engine = engine, format = format}) >> +--- >> +... >> +pk = s:create_index('pk') >> +--- >> +... >> +s:insert({1, 'sss', '3', '4', '5', '6', '7'}) >> +--- >> +- [1, 'sss', '3', '4', '5', '6', '7'] >> +... >> +aa = box.space.tst_sample:get(1) >> +--- >> +... >> +aa.VAL >> +--- >> +- sss >> +... >> +aa = aa:update({{'=',2,'ssss'}}) >> +--- >> +... >> +aa.VAL >> +--- >> +- ssss >> +... >> +-- invalid update >> +aa:update({{'=',2, 666}}) >> +--- >> +- error: 'Tuple field 2 type does not match one required by operation: expected string' >> +... >> +-- test transform integrity >> +aa:transform(-1, 1) >> +--- >> +- [1, 'ssss', '3', '4', '5', '6'] > > This doesn't check "transform integrity" AFAICS - the test passes even > if we use the original tuple's format instead of runtime_tuple_format. >