Tarantool development patches archive
 help / color / mirror / Atom feed
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
To: tarantool-patches@freelists.org
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>,
	"v.shpilevoy@tarantool.org" <v.shpilevoy@tarantool.org>
Subject: Re: [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.
Date: Wed, 18 Apr 2018 15:55:40 +0300	[thread overview]
Message-ID: <18a8749f-ba4e-61a8-a58d-12257f2d0f32@tarantool.org> (raw)
In-Reply-To: <20180418122842.pkpq3mktqwjobppm@esperanza>

> 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: <b97bc4639cb2a6814ff2ca739cc9e678e4addbe8.1524055471.git.kshcherbatov@tarantool.org>
In-Reply-To: <cover.1524055471.git.kshcherbatov@tarantool.org>
References: <cover.1524055471.git.kshcherbatov@tarantool.org>
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
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 <kshcherbatov@tarantool.org>
>> 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.
> 

  reply	other threads:[~2018-04-18 12:55 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-06 16:24 [tarantool-patches] [PATCH v2 0/2] Fix lost format on update operation Kirill Shcherbatov
2018-04-06 16:24 ` [tarantool-patches] [PATCH v2 1/2] Fixed invalid check in lbox_tuple_transform Kirill Shcherbatov
2018-04-06 16:24 ` [tarantool-patches] [PATCH v2 2/2] Fixed lost format on update and upsert operations Kirill Shcherbatov
2018-04-08 13:56   ` [tarantool-patches] " Vladislav Shpilevoy
2018-04-10 10:23     ` Kirill Shcherbatov
2018-04-10 10:44       ` Vladislav Shpilevoy
2018-04-15 10:03         ` Kirill Shcherbatov
2018-04-15 13:18           ` Vladislav Shpilevoy
2018-04-16  7:47             ` Kirill Shcherbatov
2018-04-16 10:07               ` Vladislav Shpilevoy
2018-04-16 16:51                 ` Kirill Shcherbatov
2018-04-16 17:14                   ` Vladislav Shpilevoy
2018-04-18 12:28                     ` Vladimir Davydov
2018-04-18 12:55                       ` Kirill Shcherbatov [this message]
2018-04-22 15:15                         ` Vladislav Shpilevoy
2018-04-28  6:56                           ` Kirill Shcherbatov
2018-04-28  9:29 ` [tarantool-patches] Re: [PATCH v2 0/2] Fix lost format on update operation Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=18a8749f-ba4e-61a8-a58d-12257f2d0f32@tarantool.org \
    --to=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --cc=vdavydov.dev@gmail.com \
    --subject='Re: [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox