Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@freelists.org,
	Kirill Shcherbatov <kshcherbatov@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:28:42 +0300	[thread overview]
Message-ID: <20180418122842.pkpq3mktqwjobppm@esperanza> (raw)
In-Reply-To: <42c7e5e1-2803-f3f7-6c7e-11c778399fd9@tarantool.org>

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:28 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 [this message]
2018-04-18 12:55                       ` Kirill Shcherbatov
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=20180418122842.pkpq3mktqwjobppm@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --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