Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: tarantool-patches@freelists.org,
	Kirill Shcherbatov <kshcherbatov@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.
Date: Sun, 8 Apr 2018 16:56:38 +0300	[thread overview]
Message-ID: <1f4e0a5e-30f2-f36a-3970-ec4eeb14a456@tarantool.org> (raw)
In-Reply-To: <ae6f6cf2eaeb6598f20493e145ed006e89fb509e.1523031806.git.kshcherbatov@tarantool.org>

As we discussed verbally - Kirill will try to inline box_tuple_update 
into tuple:transform() to avoid creating box_tuple_update_ex().

On 06/04/2018 19:24, Kirill Shcherbatov wrote:
> Fixes #3051
> ---
>   src/box/lua/tuple.c      |  4 ++-
>   src/box/memtx_tuple.cc   |  1 +
>   src/box/tuple.c          | 42 ++++++++++++++++--------
>   src/box/tuple.h          |  6 +++-
>   src/box/tuple_format.h   |  5 ++-
>   src/box/vy_stmt.c        |  7 ++++
>   src/box/vy_stmt.h        |  9 +++++
>   test/box/update.result   | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
>   test/box/update.test.lua | 28 ++++++++++++++++
>   9 files changed, 170 insertions(+), 17 deletions(-)
> 
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index 47b33c9..dffd5a5 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -36,6 +36,7 @@
>   #include <small/ibuf.h>
>   #include <small/region.h>
>   #include <fiber.h>
> +#include <box/tuple_update.h>
>   
>   #include "box/tuple.h"
>   #include "box/tuple_convert.h"
> @@ -392,7 +393,8 @@ lbox_tuple_transform(struct lua_State *L)
>   
>   	/* Execute tuple_update */
>   	struct tuple *new_tuple =
> -		box_tuple_update(tuple, buf->buf, buf->buf + ibuf_used(buf));
> +		box_tuple_update_ex(tuple, buf->buf, buf->buf + ibuf_used(buf),
> +		                    box_tuple_format_default());
>   	if (new_tuple == NULL)
>   		luaT_error(L);
>   	/* box_tuple_update() doesn't leak on exception, see public API doc */
> 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..ec2c3b5 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 *
> +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 *
> +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
> @@ -393,25 +403,23 @@ box_tuple_next(box_tuple_iterator_t *it)
>   }
>   
>   box_tuple_t *
> -box_tuple_update(const box_tuple_t *tuple, const char *expr,
> -		 const char *expr_end)
> +box_tuple_update_ex(const box_tuple_t *tuple, const char *expr,
> +                    const char *expr_end, struct tuple_format *format)
>   {
> -	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);
>   	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(format, new_data,
> +	                              new_data + new_size);
>   	region_truncate(region, used);
>   	if (ret != NULL)
>   		return tuple_bless(ret);
> @@ -419,11 +427,16 @@ box_tuple_update(const box_tuple_t *tuple, const char *expr,
>   }
>   
>   box_tuple_t *
> -box_tuple_upsert(const box_tuple_t *tuple, const char *expr,
> +box_tuple_update(const box_tuple_t *tuple, const char *expr,
>   		 const char *expr_end)
>   {
> -	struct tuple_format *format = tuple_format_runtime;
> +	return box_tuple_update_ex(tuple, expr, expr_end, tuple_format(tuple));
> +}
>   
> +box_tuple_t *
> +box_tuple_upsert(const box_tuple_t *tuple, const char *expr,
> +		 const char *expr_end)
> +{
>   	uint32_t new_size = 0, bsize;
>   	const char *old_data = tuple_data_range(tuple, &bsize);
>   	struct region *region = &fiber()->gc;
> @@ -437,7 +450,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..d5b5bdb 100644
> --- a/src/box/tuple.h
> +++ b/src/box/tuple.h
> @@ -282,6 +282,10 @@ box_tuple_update(const box_tuple_t *tuple, const char *expr, const
>   		 char *expr_end);
>   
>   box_tuple_t *
> +box_tuple_update_ex(const box_tuple_t *tuple, const char *expr,
> +                    const char *expr_end, struct tuple_format *format);
> +
> +box_tuple_t *
>   box_tuple_upsert(const box_tuple_t *tuple, const char *expr, const
>   		 char *expr_end);
>   
> @@ -445,7 +449,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. */
> +	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..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 *
> +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);
> +
> +/**
>    * Duplicate the statememnt.
>    *
>    * @param stmt statement
> diff --git a/test/box/update.result b/test/box/update.result
> index a3f731b..b8e94b1 100644
> --- a/test/box/update.result
> +++ b/test/box/update.result
> @@ -849,3 +849,88 @@ s:update(1, {{'=', 3, map}})
>   s:drop()
>   ---
>   ...
> +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'})
> +---
> +- [1, 'sss']
> +...
> +aa = box.space.tst_sample:get(1)
> +---
> +...
> +aa.VAL
> +---
> +- sss
> +...
> +aa = aa:update({{'=',2,'ssss'}})
> +---
> +...
> +collectgarbage()
> +---
> +- 0
> +...
> +aa.VAL
> +---
> +- ssss
> +...
> +s:drop()
> +---
> +...
> +aa = nil
> +---
> +...
> +collectgarbage()
> +---
> +- 0
> +...
> +s = box.schema.space.create('tst_sample', {engine = 'vinyl', format = format})
> +---
> +...
> +pk = s:create_index('pk')
> +---
> +...
> +s:insert({1, 'sss'})
> +---
> +- [1, 'sss']
> +...
> +aa = box.space.tst_sample:get(1)
> +---
> +...
> +aa.VAL
> +---
> +- sss
> +...
> +aa = aa:update({{'=',2,'ssss'}})
> +---
> +...
> +collectgarbage()
> +---
> +- 0
> +...
> +aa.VAL
> +---
> +- ssss
> +...
> +s:drop()
> +---
> +...
> +aa = nil
> +---
> +...
> +collectgarbage()
> +---
> +- 0
> +...
> diff --git a/test/box/update.test.lua b/test/box/update.test.lua
> index c455bc1..37be3a0 100644
> --- a/test/box/update.test.lua
> +++ b/test/box/update.test.lua
> @@ -269,3 +269,31 @@ t:update({{'=', 3, map}})
>   s:update(1, {{'=', 3, map}})
>   
>   s:drop()
> +
> +
> +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()
> +aa.VAL
> +s:drop()
> +aa = nil
> +collectgarbage()
> +
> +s = box.schema.space.create('tst_sample', {engine = 'vinyl', 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()
> +aa.VAL
> +s:drop()
> +aa = nil
> +collectgarbage()
> 

  reply	other threads:[~2018-04-08 13:56 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   ` Vladislav Shpilevoy [this message]
2018-04-10 10:23     ` [tarantool-patches] " 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
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=1f4e0a5e-30f2-f36a-3970-ec4eeb14a456@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[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