[tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.

[Vladimir Davydov]

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 at 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.