* [tarantool-patches] [PATCH v2 0/2] Fix lost format on update operation
@ 2018-04-06 16:24 Kirill Shcherbatov
2018-04-06 16:24 ` [tarantool-patches] [PATCH v2 1/2] Fixed invalid check in lbox_tuple_transform Kirill Shcherbatov
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Kirill Shcherbatov @ 2018-04-06 16:24 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
Branch: http://github.com/tarantool/tarantool/tree/gh-3051-lost-format-while-tuple-update
Issue: https://github.com/tarantool/tarantool/issues/3051
Kirill Shcherbatov (2):
Fixed invalid check in lbox_tuple_transform.
Fixed lost format on update and upsert operations.
src/box/lua/tuple.c | 6 ++--
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, 171 insertions(+), 18 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] [PATCH v2 1/2] Fixed invalid check in lbox_tuple_transform.
2018-04-06 16:24 [tarantool-patches] [PATCH v2 0/2] Fix lost format on update operation Kirill Shcherbatov
@ 2018-04-06 16:24 ` 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-28 9:29 ` [tarantool-patches] Re: [PATCH v2 0/2] Fix lost format on update operation Vladislav Shpilevoy
2 siblings, 0 replies; 17+ messages in thread
From: Kirill Shcherbatov @ 2018-04-06 16:24 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
Tuple has been checked instead of new_tuple returned as box_tuple_update result.
---
src/box/lua/tuple.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 7ca4299..47b33c9 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -393,7 +393,7 @@ 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));
- if (tuple == NULL)
+ if (new_tuple == NULL)
luaT_error(L);
/* box_tuple_update() doesn't leak on exception, see public API doc */
luaT_pushtuple(L, new_tuple);
--
2.7.4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] [PATCH v2 2/2] Fixed lost format on update and upsert operations.
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 ` Kirill Shcherbatov
2018-04-08 13:56 ` [tarantool-patches] " Vladislav Shpilevoy
2018-04-28 9:29 ` [tarantool-patches] Re: [PATCH v2 0/2] Fix lost format on update operation Vladislav Shpilevoy
2 siblings, 1 reply; 17+ messages in thread
From: Kirill Shcherbatov @ 2018-04-06 16:24 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
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()
--
2.7.4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.
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
2018-04-10 10:23 ` Kirill Shcherbatov
0 siblings, 1 reply; 17+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-08 13:56 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov
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()
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.
2018-04-08 13:56 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-04-10 10:23 ` Kirill Shcherbatov
2018-04-10 10:44 ` Vladislav Shpilevoy
0 siblings, 1 reply; 17+ messages in thread
From: Kirill Shcherbatov @ 2018-04-10 10:23 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
From c268dedd32ed124fb25c46e3fb0bf1cd507fda62 Mon Sep 17 00:00:00 2001
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Date: Fri, 6 Apr 2018 19:21:32 +0300
Subject: [PATCH] Fixed lost format on update and upsert operations.
Fixes #3051
---
src/box/lua/tuple.c | 31 +++++++++++--
src/box/memtx_tuple.cc | 1 +
src/box/tuple.c | 31 ++++++++-----
src/box/tuple.h | 2 +-
src/box/tuple_format.h | 5 +-
src/box/vy_stmt.c | 7 +++
src/box/vy_stmt.h | 9 ++++
test/box/update.result | 118 +++++++++++++++++++++++++++++++++++++++++++++++
test/box/update.test.lua | 41 ++++++++++++++++
9 files changed, 226 insertions(+), 19 deletions(-)
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 47b33c9..9bb62b0 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"
@@ -390,15 +391,35 @@ 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);
+ 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) {
+ region_truncate(region, used);
+ goto error;
+ }
+ struct tuple *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 */
+ goto error;
+ tuple_bless(new_tuple);
+
luaT_pushtuple(L, new_tuple);
ibuf_reset(buf);
return 1;
+error:
+ if (new_tuple == NULL)
+ luaT_error(L);
+ return 0;
}
/**
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..cb57d81 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
@@ -396,22 +406,20 @@ 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;
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(tuple_format(tuple), new_data,
+ new_data + new_size);
region_truncate(region, used);
if (ret != NULL)
return tuple_bless(ret);
@@ -422,8 +430,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 +443,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..d8138e2 100644
--- a/src/box/tuple.h
+++ b/src/box/tuple.h
@@ -445,7 +445,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..7b9e295 100644
--- a/test/box/update.result
+++ b/test/box/update.result
@@ -849,3 +849,121 @@ s:update(1, {{'=', 3, map}})
s:drop()
---
...
+--
+-- gh-3051 Lost format while tuple update
+--
+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
+...
+-- test transform integrity
+space = box.schema.space.create('tweedledum')
+---
+...
+tmp = space:create_index('primary', { type = 'hash', parts = {1, 'string'}, unique = true })
+---
+...
+t = space:insert{'1', '2', '3', '4', '5', '6', '7'}
+---
+...
+t:transform(-1, 1)
+---
+- ['1', '2', '3', '4', '5', '6']
+...
+n = 2000
+---
+...
+tab = {}; for i=1,n,1 do table.insert(tab, i) end
+---
+...
+t = box.tuple.new(tab)
+---
+...
+t:transform(1, n - 1)
+---
+- [2000]
+...
+t = nil
+---
+...
diff --git a/test/box/update.test.lua b/test/box/update.test.lua
index c455bc1..1dc1751 100644
--- a/test/box/update.test.lua
+++ b/test/box/update.test.lua
@@ -269,3 +269,44 @@ t:update({{'=', 3, map}})
s:update(1, {{'=', 3, map}})
s:drop()
+
+--
+-- gh-3051 Lost format while tuple update
+--
+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()
+
+-- test transform integrity
+space = box.schema.space.create('tweedledum')
+tmp = space:create_index('primary', { type = 'hash', parts = {1, 'string'}, unique = true })
+t = space:insert{'1', '2', '3', '4', '5', '6', '7'}
+t:transform(-1, 1)
+n = 2000
+tab = {}; for i=1,n,1 do table.insert(tab, i) end
+t = box.tuple.new(tab)
+t:transform(1, n - 1)
+t = nil
--
2.7.4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.
2018-04-10 10:23 ` Kirill Shcherbatov
@ 2018-04-10 10:44 ` Vladislav Shpilevoy
2018-04-15 10:03 ` Kirill Shcherbatov
0 siblings, 1 reply; 17+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-10 10:44 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches
Hello. See my 19 comments below.
1. Build fails:
/Users/v.shpilevoy/Work/Repositories/tarantool/src/box/lua/tuple.c:405:6:
error: variable 'new_tuple' is used uninitialized whenever 'if'
condition is true [-Werror,-Wsometimes-uninitialized]
if (new_data == NULL) {
^~~~~~~~~~~~~~~~
/Users/v.shpilevoy/Work/Repositories/tarantool/src/box/lua/tuple.c:420:6:
note: uninitialized use occurs here
if (new_tuple == NULL)
^~~~~~~~~
/Users/v.shpilevoy/Work/Repositories/tarantool/src/box/lua/tuple.c:405:2:
note: remove the 'if' if its condition is always false
if (new_data == NULL) {
^~~~~~~~~~~~~~~~~~~~~~~
/Users/v.shpilevoy/Work/Repositories/tarantool/src/box/lua/tuple.c:409:2:
note: variable 'new_tuple' is declared here
struct tuple *new_tuple = tuple_new(box_tuple_format_default(),
^
1 error generated.
On 10/04/2018 13:23, Kirill Shcherbatov wrote:
> From c268dedd32ed124fb25c46e3fb0bf1cd507fda62 Mon Sep 17 00:00:00 2001
> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> Date: Fri, 6 Apr 2018 19:21:32 +0300
> Subject: [PATCH] Fixed lost format on update and upsert operations.
2. Please, describe a problem, which the patch solves.
>
> Fixes #3051
> ---
> src/box/lua/tuple.c | 31 +++++++++++--
> src/box/memtx_tuple.cc | 1 +
> src/box/tuple.c | 31 ++++++++-----
> src/box/tuple.h | 2 +-
> src/box/tuple_format.h | 5 +-
> src/box/vy_stmt.c | 7 +++
> src/box/vy_stmt.h | 9 ++++
> test/box/update.result | 118 +++++++++++++++++++++++++++++++++++++++++++++++
> test/box/update.test.lua | 41 ++++++++++++++++
> 9 files changed, 226 insertions(+), 19 deletions(-)
>
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index 47b33c9..9bb62b0 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>
3. Do not put project headers in <>.
>
> #include "box/tuple.h"
> #include "box/tuple_convert.h"
> @@ -390,15 +391,35 @@ 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);
> + 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);
4. Please, do not put each argument on a separate line when possible.
> + if (new_data == NULL) {
> + region_truncate(region, used);
> + goto error;
5. You can remove goto error, if instead of new_data == NULL with do
if new_data != NULL then new_tuple = tuple_new(...) end
region_truncate(...)
if new_tuple == NULL then lua_error(...) end
> + }
> + struct tuple *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 */
> + goto error;
> + tuple_bless(new_tuple);
6. AFAIK tuple_bless here is not needed, because lua_pushtuple already
does tuple reference count increment.
> +
> luaT_pushtuple(L, new_tuple);
> ibuf_reset(buf);
> return 1;
> +error:
> + if (new_tuple == NULL)
> + luaT_error(L);
> + return 0;
7. Error label will be removed, but please answer yourself why error
label can return non-error code.
> }
>
> /**
> 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
> diff --git a/src/box/tuple.c b/src/box/tuple.c
> index d4760f3..cb57d81 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 *
8. Please, make this function static.
> +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 *
9. Please make this wrapper be in tuple.h, static inline and add a
comment. It is a common practice for our virtual methods wrappers. See
key_def.h for examples.
> +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
> @@ -396,22 +406,20 @@ 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;
> 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);
10. Garbage diff.
> 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(tuple_format(tuple), new_data,
> + new_data + new_size);
11. Same.
> region_truncate(region, used);
> if (ret != NULL)
> return tuple_bless(ret);
> @@ -422,8 +430,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;
12. Same.
> -
> uint32_t new_size = 0, bsize;
> const char *old_data = tuple_data_range(tuple, &bsize);
> struct region *region = &fiber()->gc;
> @@ -437,7 +443,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);
13. Same.
> 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..d8138e2 100644
> --- a/src/box/tuple.h
> +++ b/src/box/tuple.h
> @@ -445,7 +445,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. */
14. Out of 66 symbols.
> + struct tuple*
> + (*tuple_new)(struct tuple_format *format, const char *data, const char *end);
15. Out of 80 symbols.
> };
>
> /** 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 *
16. Static.
> +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);
17. Used in vy_stmt.c only. You can remove it from the header.
> diff --git a/test/box/update.test.lua b/test/box/update.test.lua
> index c455bc1..1dc1751 100644
> --- a/test/box/update.test.lua
> +++ b/test/box/update.test.lua
> @@ -269,3 +269,44 @@ t:update({{'=', 3, map}})
> s:update(1, {{'=', 3, map}})
>
> s:drop()
> +
> +--
> +-- gh-3051 Lost format while tuple update
> +--
> +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()
18. Even if a tuple leaks, this code does not detect it. Either create a
correct check via Lua weak references (see __mode = 'k'/'v' and
net.box.test.lua), or remove this code.
> +aa.VAL
> +s:drop()
> +aa = nil
> +collectgarbage()
> +
> +s = box.schema.space.create('tst_sample', {engine = 'vinyl', format = format})
19. When you need multiple engines for the same test, you must use
engine/ test suite.
> +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()
> +
> +-- test transform integrity
> +space = box.schema.space.create('tweedledum')
> +tmp = space:create_index('primary', { type = 'hash', parts = {1, 'string'}, unique = true })
> +t = space:insert{'1', '2', '3', '4', '5', '6', '7'}
> +t:transform(-1, 1)
> +n = 2000
> +tab = {}; for i=1,n,1 do table.insert(tab, i) end
> +t = box.tuple.new(tab)
> +t:transform(1, n - 1)
> +t = nil
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.
2018-04-10 10:44 ` Vladislav Shpilevoy
@ 2018-04-15 10:03 ` Kirill Shcherbatov
2018-04-15 13:18 ` Vladislav Shpilevoy
0 siblings, 1 reply; 17+ messages in thread
From: Kirill Shcherbatov @ 2018-04-15 10:03 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
From 1aa00690d6c246450e82454f414c89af709905b5 Mon Sep 17 00:00:00 2001
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Date: Sun, 15 Apr 2018 13:02:29 +0300
Subject: [PATCH] Accounted recommendations
---
src/box/lua/tuple.c | 25 +++++++---------------
src/box/tuple.c | 10 ++-------
src/box/tuple.h | 8 ++++---
src/box/tuple_format.h | 9 +++++---
src/box/vy_stmt.c | 5 ++++-
src/box/vy_stmt.h | 9 --------
test/box/update.result | 54 +++++++++---------------------------------------
test/box/update.test.lua | 20 +++++-------------
8 files changed, 40 insertions(+), 100 deletions(-)
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index d3b8dac..c40bdb7 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() */
@@ -36,7 +37,6 @@
#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"
@@ -397,30 +397,21 @@ lbox_tuple_transform(struct lua_State *L)
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,
+ region, buf->buf,
buf->buf + ibuf_used(buf),
- old_data,
- old_data + bsize,
+ old_data, old_data + bsize,
&new_size, 1, NULL);
- if (new_data == NULL) {
- region_truncate(region, used);
- goto error;
- }
- new_tuple = tuple_new(box_tuple_format_default(),
- new_data, new_data + new_size);
+ 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)
- goto error;
- tuple_bless(new_tuple);
+ luaT_error(L);
luaT_pushtuple(L, new_tuple);
ibuf_reset(buf);
return 1;
-error:
- if (new_tuple == NULL)
- luaT_error(L);
- return 0;
}
/**
diff --git a/src/box/tuple.c b/src/box/tuple.c
index cb57d81..34efc12 100644
--- a/src/box/tuple.c
+++ b/src/box/tuple.c
@@ -61,7 +61,7 @@ struct tuple_format *tuple_format_runtime;
static void
runtime_tuple_delete(struct tuple_format *format, struct tuple *tuple);
-struct 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 */
@@ -70,7 +70,7 @@ static struct tuple_format_vtab tuple_format_runtime_vtab = {
runtime_tuple_new,
};
-struct tuple *
+static struct tuple *
runtime_tuple_new(struct tuple_format *format, const char *data, const char *end)
{
assert(format->vtab.tuple_delete == tuple_format_runtime_vtab.tuple_delete);
@@ -151,12 +151,6 @@ 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
diff --git a/src/box/tuple.h b/src/box/tuple.h
index d8138e2..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.
diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h
index cc727ec..155d0b5 100644
--- a/src/box/tuple_format.h
+++ b/src/box/tuple_format.h
@@ -66,12 +66,15 @@ 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
(*tuple_delete)(struct tuple_format *format, struct tuple *tuple);
- /** Allocates a new tuple on the same allocator and with the same format. */
+ /** 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_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 3135c58..d02ffd2 100644
--- a/src/box/vy_stmt.c
+++ b/src/box/vy_stmt.c
@@ -45,6 +45,9 @@
#include "xrow.h"
#include "fiber.h"
+static struct tuple *
+vy_tuple_new(struct tuple_format *format, const char *data, const char *end);
+
struct tuple_format_vtab vy_tuple_format_vtab = {
vy_tuple_delete,
vy_tuple_new,
@@ -70,7 +73,7 @@ vy_tuple_delete(struct tuple_format *format, struct tuple *tuple)
free(tuple);
}
-struct tuple *
+static struct tuple *
vy_tuple_new(struct tuple_format *format, const char *data, const char *end)
{
return vy_stmt_new_insert(format, data, end);
diff --git a/src/box/vy_stmt.h b/src/box/vy_stmt.h
index efc99c8..a33739d 100644
--- a/src/box/vy_stmt.h
+++ b/src/box/vy_stmt.h
@@ -206,15 +206,6 @@ 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 7b9e295..c71cbd8 100644
--- a/test/box/update.result
+++ b/test/box/update.result
@@ -852,54 +852,25 @@ s:drop()
--
-- gh-3051 Lost format while tuple update
--
-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'})
+test_run = require('test_run')
---
-- [1, 'sss']
...
-aa = box.space.tst_sample:get(1)
+inspector = test_run.new()
---
...
-aa.VAL
+engine = inspector:get_cfg('engine')
---
-- sss
...
-aa = aa:update({{'=',2,'ssss'}})
----
-...
-collectgarbage()
----
-- 0
-...
-aa.VAL
----
-- ssss
-...
-s:drop()
+format = {}
---
...
-aa = nil
+format[1] = {name = 'KEY', type = 'unsigned'}
---
...
-collectgarbage()
+format[2] = {name = 'VAL', type = 'string'}
---
-- 0
...
-s = box.schema.space.create('tst_sample', {engine = 'vinyl', format = format})
+s = box.schema.space.create('tst_sample', {engine = engine, format = format})
---
...
pk = s:create_index('pk')
@@ -919,10 +890,6 @@ aa.VAL
aa = aa:update({{'=',2,'ssss'}})
---
...
-collectgarbage()
----
-- 0
-...
aa.VAL
---
- ssss
@@ -933,10 +900,6 @@ s:drop()
aa = nil
---
...
-collectgarbage()
----
-- 0
-...
-- test transform integrity
space = box.schema.space.create('tweedledum')
---
@@ -967,3 +930,6 @@ t:transform(1, n - 1)
t = nil
---
...
+space:drop()
+---
+...
diff --git a/test/box/update.test.lua b/test/box/update.test.lua
index 1dc1751..b6271a6 100644
--- a/test/box/update.test.lua
+++ b/test/box/update.test.lua
@@ -273,32 +273,21 @@ s:drop()
--
-- gh-3051 Lost format while tuple update
--
+test_run = require('test_run')
+inspector = test_run.new()
+engine = inspector:get_cfg('engine')
format = {}
format[1] = {name = 'KEY', type = 'unsigned'}
format[2] = {name = 'VAL', type = 'string'}
-s = box.schema.create_space('tst_sample', {format = format})
+s = box.schema.space.create('tst_sample', {engine = engine, 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()
-- test transform integrity
space = box.schema.space.create('tweedledum')
@@ -310,3 +299,4 @@ tab = {}; for i=1,n,1 do table.insert(tab, i) end
t = box.tuple.new(tab)
t:transform(1, n - 1)
t = nil
+space:drop()
--
2.7.4
On 10.04.2018 13:44, Vladislav Shpilevoy wrote:
> Hello. See my 19 comments below.
>
> 1. Build fails:
>
> /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/lua/tuple.c:405:6:
> error: variable 'new_tuple' is used uninitialized whenever 'if'
> condition is true [-Werror,-Wsometimes-uninitialized]
> if (new_data == NULL) {
> ^~~~~~~~~~~~~~~~
> /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/lua/tuple.c:420:6:
> note: uninitialized use occurs here
> if (new_tuple == NULL)
> ^~~~~~~~~
> /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/lua/tuple.c:405:2:
> note: remove the 'if' if its condition is always false
> if (new_data == NULL) {
> ^~~~~~~~~~~~~~~~~~~~~~~
> /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/lua/tuple.c:409:2:
> note: variable 'new_tuple' is declared here
> struct tuple *new_tuple = tuple_new(box_tuple_format_default(),
> ^
> 1 error generated.
>
>
> On 10/04/2018 13:23, Kirill Shcherbatov wrote:
>> From c268dedd32ed124fb25c46e3fb0bf1cd507fda62 Mon Sep 17 00:00:00 2001
>> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
>> Date: Fri, 6 Apr 2018 19:21:32 +0300
>> Subject: [PATCH] Fixed lost format on update and upsert operations.
>
> 2. Please, describe a problem, which the patch solves.
>
>>
>> Fixes #3051
>> ---
>> src/box/lua/tuple.c | 31 +++++++++++--
>> src/box/memtx_tuple.cc | 1 +
>> src/box/tuple.c | 31 ++++++++-----
>> src/box/tuple.h | 2 +-
>> src/box/tuple_format.h | 5 +-
>> src/box/vy_stmt.c | 7 +++
>> src/box/vy_stmt.h | 9 ++++
>> test/box/update.result | 118 +++++++++++++++++++++++++++++++++++++++++++++++
>> test/box/update.test.lua | 41 ++++++++++++++++
>> 9 files changed, 226 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
>> index 47b33c9..9bb62b0 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>
>
> 3. Do not put project headers in <>.
>
>>
>> #include "box/tuple.h"
>> #include "box/tuple_convert.h"
>> @@ -390,15 +391,35 @@ 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);
>> + 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);
>
> 4. Please, do not put each argument on a separate line when possible.
>
>> + if (new_data == NULL) {
>> + region_truncate(region, used);
>> + goto error;
>
> 5. You can remove goto error, if instead of new_data == NULL with do
>
> if new_data != NULL then new_tuple = tuple_new(...) end
> region_truncate(...)
> if new_tuple == NULL then lua_error(...) end
>
>> + }
>> + struct tuple *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 */
>> + goto error;
>> + tuple_bless(new_tuple);
>
> 6. AFAIK tuple_bless here is not needed, because lua_pushtuple already
> does tuple reference count increment.
>
>> +
>> luaT_pushtuple(L, new_tuple);
>> ibuf_reset(buf);
>> return 1;
>> +error:
>> + if (new_tuple == NULL)
>> + luaT_error(L);
>> + return 0;
>
> 7. Error label will be removed, but please answer yourself why error
> label can return non-error code.
>
>> }
>>
>> /**
>> 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
>> diff --git a/src/box/tuple.c b/src/box/tuple.c
>> index d4760f3..cb57d81 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 *
>
> 8. Please, make this function static.
>
>> +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 *
>
> 9. Please make this wrapper be in tuple.h, static inline and add a
> comment. It is a common practice for our virtual methods wrappers. See
> key_def.h for examples.
>
>> +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
>> @@ -396,22 +406,20 @@ 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;
>> 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);
>
> 10. Garbage diff.
>
>> 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(tuple_format(tuple), new_data,
>> + new_data + new_size);
>
> 11. Same.
>
>> region_truncate(region, used);
>> if (ret != NULL)
>> return tuple_bless(ret);
>> @@ -422,8 +430,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;
>
> 12. Same.
>
>> -
>> uint32_t new_size = 0, bsize;
>> const char *old_data = tuple_data_range(tuple, &bsize);
>> struct region *region = &fiber()->gc;
>> @@ -437,7 +443,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);
>
> 13. Same.
>
>> 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..d8138e2 100644
>> --- a/src/box/tuple.h
>> +++ b/src/box/tuple.h
>> @@ -445,7 +445,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. */
>
> 14. Out of 66 symbols.
>
>> + struct tuple*
>> + (*tuple_new)(struct tuple_format *format, const char *data, const char *end);
>
> 15. Out of 80 symbols.
>
>> };
>>
>> /** 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 *
>
> 16. Static.
>
>> +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);
>
> 17. Used in vy_stmt.c only. You can remove it from the header.
>
>> diff --git a/test/box/update.test.lua b/test/box/update.test.lua
>> index c455bc1..1dc1751 100644
>> --- a/test/box/update.test.lua
>> +++ b/test/box/update.test.lua
>> @@ -269,3 +269,44 @@ t:update({{'=', 3, map}})
>> s:update(1, {{'=', 3, map}})
>>
>> s:drop()
>> +
>> +--
>> +-- gh-3051 Lost format while tuple update
>> +--
>> +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()
>
> 18. Even if a tuple leaks, this code does not detect it. Either create a
> correct check via Lua weak references (see __mode = 'k'/'v' and
> net.box.test.lua), or remove this code.
>
>> +aa.VAL
>> +s:drop()
>> +aa = nil
>> +collectgarbage()
>> +
>> +s = box.schema.space.create('tst_sample', {engine = 'vinyl', format = format})
>
> 19. When you need multiple engines for the same test, you must use
> engine/ test suite.
>
>> +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()
>> +
>> +-- test transform integrity
>> +space = box.schema.space.create('tweedledum')
>> +tmp = space:create_index('primary', { type = 'hash', parts = {1, 'string'}, unique = true })
>> +t = space:insert{'1', '2', '3', '4', '5', '6', '7'}
>> +t:transform(-1, 1)
>> +n = 2000
>> +tab = {}; for i=1,n,1 do table.insert(tab, i) end
>> +t = box.tuple.new(tab)
>> +t:transform(1, n - 1)
>> +t = nil
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.
2018-04-15 10:03 ` Kirill Shcherbatov
@ 2018-04-15 13:18 ` Vladislav Shpilevoy
2018-04-16 7:47 ` Kirill Shcherbatov
0 siblings, 1 reply; 17+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-15 13:18 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov
Hello. See below 6 comments.
1. I fixed some of my comments, and pushed them with --force.
You can see them using diff your local and remote changes. After
this do pull. Please, try to investigate, why your text editor
uses spaces for multiline statements alignment, and fix it.
2. I do not see a test on upsert - please, add.
3. Please, add a test on incorrect update.
4. In the previous letter I have asked you to move the test into
engine/ suite. You can not get different engines on box/ suite:
[001] Test failed! Result content mismatch:
[001] --- box/update.result Sun Apr 15 15:55:23 2018
[001] +++ box/update.reject Sun Apr 15 16:07:40 2018
[001] @@ -861,6 +861,10 @@
[001] engine = inspector:get_cfg('engine')
[001] ---
[001] ...
[001] +engine
[001] +---
[001] +- null
[001] +...
[001] format = {}
[001] ---
[001] ...
[001]
As you can see, I printed engine variable in you test and 1) it is null,
2) the test is not run 2 times - with engine = memtx and engine = vinyl.
For this you must use engine/ suite, not box/.
> +test_run = require('test_run')
> +inspector = test_run.new()
5. It is not needed to create a new test run each time when you need to
get a variable from environment. Here the test run already is created.
> +engine = inspector:get_cfg('engine')
> format = {}
> -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()
>
> -- test transform integrity
> space = box.schema.space.create('tweedledum')
6. Try to do not create a new space to test each tuple. You can create
one space, and test on it transform, correct update, correct upsert,
incorrect update, incorrect upsert.
Too many actions for such simple test is not good, because test count
grows every day, and running all of them is already too long.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.
2018-04-15 13:18 ` Vladislav Shpilevoy
@ 2018-04-16 7:47 ` Kirill Shcherbatov
2018-04-16 10:07 ` Vladislav Shpilevoy
0 siblings, 1 reply; 17+ messages in thread
From: Kirill Shcherbatov @ 2018-04-16 7:47 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
I've fixed tests and wrong code style with tabs.
From f9803f8d0b411abac02e35d5e4120ff0ce7e9341 Mon Sep 17 00:00:00 2001
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Date: Mon, 16 Apr 2018 10:44:10 +0300
Subject: [PATCH] Fixed tests and spaces
---
src/box/lua/tuple.c | 10 +++---
test/box/update.result | 84 ---------------------------------------------
test/box/update.test.lua | 31 -----------------
test/engine/update.result | 61 ++++++++++++++++++++++++++++++++
test/engine/update.test.lua | 24 +++++++++++++
5 files changed, 90 insertions(+), 120 deletions(-)
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index c40bdb7..566cbec 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -397,13 +397,13 @@ lbox_tuple_transform(struct lua_State *L)
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);
+ 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);
+ new_data, new_data + new_size);
region_truncate(region, used);
if (new_tuple == NULL)
diff --git a/test/box/update.result b/test/box/update.result
index c71cbd8..a3f731b 100644
--- a/test/box/update.result
+++ b/test/box/update.result
@@ -849,87 +849,3 @@ s:update(1, {{'=', 3, map}})
s:drop()
---
...
---
--- gh-3051 Lost format while tuple update
---
-test_run = require('test_run')
----
-...
-inspector = test_run.new()
----
-...
-engine = inspector:get_cfg('engine')
----
-...
-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'})
----
-- [1, 'sss']
-...
-aa = box.space.tst_sample:get(1)
----
-...
-aa.VAL
----
-- sss
-...
-aa = aa:update({{'=',2,'ssss'}})
----
-...
-aa.VAL
----
-- ssss
-...
-s:drop()
----
-...
-aa = nil
----
-...
--- test transform integrity
-space = box.schema.space.create('tweedledum')
----
-...
-tmp = space:create_index('primary', { type = 'hash', parts = {1, 'string'}, unique = true })
----
-...
-t = space:insert{'1', '2', '3', '4', '5', '6', '7'}
----
-...
-t:transform(-1, 1)
----
-- ['1', '2', '3', '4', '5', '6']
-...
-n = 2000
----
-...
-tab = {}; for i=1,n,1 do table.insert(tab, i) end
----
-...
-t = box.tuple.new(tab)
----
-...
-t:transform(1, n - 1)
----
-- [2000]
-...
-t = nil
----
-...
-space:drop()
----
-...
diff --git a/test/box/update.test.lua b/test/box/update.test.lua
index b6271a6..c455bc1 100644
--- a/test/box/update.test.lua
+++ b/test/box/update.test.lua
@@ -269,34 +269,3 @@ t:update({{'=', 3, map}})
s:update(1, {{'=', 3, map}})
s:drop()
-
---
--- gh-3051 Lost format while tuple update
---
-test_run = require('test_run')
-inspector = test_run.new()
-engine = inspector:get_cfg('engine')
-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'})
-aa = box.space.tst_sample:get(1)
-aa.VAL
-aa = aa:update({{'=',2,'ssss'}})
-aa.VAL
-s:drop()
-aa = nil
-
--- test transform integrity
-space = box.schema.space.create('tweedledum')
-tmp = space:create_index('primary', { type = 'hash', parts = {1, 'string'}, unique = true })
-t = space:insert{'1', '2', '3', '4', '5', '6', '7'}
-t:transform(-1, 1)
-n = 2000
-tab = {}; for i=1,n,1 do table.insert(tab, i) end
-t = box.tuple.new(tab)
-t:transform(1, n - 1)
-t = nil
-space:drop()
diff --git a/test/engine/update.result b/test/engine/update.result
index 653ebec..48398d2 100644
--- a/test/engine/update.result
+++ b/test/engine/update.result
@@ -683,3 +683,64 @@ 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
+...
+-- 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
+...
+s:drop()
+---
+...
diff --git a/test/engine/update.test.lua b/test/engine/update.test.lua
index 0252b8a..c53e458 100644
--- a/test/engine/update.test.lua
+++ b/test/engine/update.test.lua
@@ -94,3 +94,27 @@ 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
+-- 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
+
+s:drop()
--
2.7.4
On 15.04.2018 16:18, Vladislav Shpilevoy wrote:
> Hello. See below 6 comments.
>
> 1. I fixed some of my comments, and pushed them with --force.
> You can see them using diff your local and remote changes. After
> this do pull. Please, try to investigate, why your text editor
> uses spaces for multiline statements alignment, and fix it.
>
> 2. I do not see a test on upsert - please, add.
>
> 3. Please, add a test on incorrect update.
>
> 4. In the previous letter I have asked you to move the test into
> engine/ suite. You can not get different engines on box/ suite:
>
> [001] Test failed! Result content mismatch:
> [001] --- box/update.result Sun Apr 15 15:55:23 2018
> [001] +++ box/update.reject Sun Apr 15 16:07:40 2018
> [001] @@ -861,6 +861,10 @@
> [001] engine = inspector:get_cfg('engine')
> [001] ---
> [001] ...
> [001] +engine
> [001] +---
> [001] +- null
> [001] +...
> [001] format = {}
> [001] ---
> [001] ...
> [001]
>
>
> As you can see, I printed engine variable in you test and 1) it is null,
> 2) the test is not run 2 times - with engine = memtx and engine = vinyl.
> For this you must use engine/ suite, not box/.
>
>
>
>> +test_run = require('test_run')
>> +inspector = test_run.new()
>
> 5. It is not needed to create a new test run each time when you need to
> get a variable from environment. Here the test run already is created.
>
>> +engine = inspector:get_cfg('engine')
>> format = {}
>
>
>> -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()
>>
>> -- test transform integrity
>> space = box.schema.space.create('tweedledum')
>
> 6. Try to do not create a new space to test each tuple. You can create
> one space, and test on it transform, correct update, correct upsert,
> incorrect update, incorrect upsert.
>
> Too many actions for such simple test is not good, because test count
> grows every day, and running all of them is already too long.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.
2018-04-16 7:47 ` Kirill Shcherbatov
@ 2018-04-16 10:07 ` Vladislav Shpilevoy
2018-04-16 16:51 ` Kirill Shcherbatov
0 siblings, 1 reply; 17+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-16 10:07 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches
Hello. Thanks for fixes. One more thing is left - please,
add tests on incorrect update and incorrect upsert, as I
asked you in two previous reviews.
And in tests you does not need to do insert + get. Insert returns
the tuple, so you can do t = s:insert(), t:update/upsert ...
On 16/04/2018 10:47, Kirill Shcherbatov wrote:
> I've fixed tests and wrong code style with tabs.
>
> From f9803f8d0b411abac02e35d5e4120ff0ce7e9341 Mon Sep 17 00:00:00 2001
> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> Date: Mon, 16 Apr 2018 10:44:10 +0300
> Subject: [PATCH] Fixed tests and spaces
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.
2018-04-16 10:07 ` Vladislav Shpilevoy
@ 2018-04-16 16:51 ` Kirill Shcherbatov
2018-04-16 17:14 ` Vladislav Shpilevoy
0 siblings, 1 reply; 17+ messages in thread
From: Kirill Shcherbatov @ 2018-04-16 16:51 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
From 84f2fc22124c2a2d1450de9b818f61c9c1cf31b0 Mon Sep 17 00:00:00 2001
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Date: Mon, 16 Apr 2018 19:50:04 +0300
Subject: [PATCH] New tests for invalid update and upsert
---
test/engine/update.result | 10 ++++++++++
test/engine/update.test.lua | 5 ++++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/test/engine/update.result b/test/engine/update.result
index 48398d2..0ae22ac 100644
--- a/test/engine/update.result
+++ b/test/engine/update.result
@@ -719,6 +719,11 @@ 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)
---
@@ -741,6 +746,11 @@ 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 c53e458..3ff7e62 100644
--- a/test/engine/update.test.lua
+++ b/test/engine/update.test.lua
@@ -108,6 +108,8 @@ 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
@@ -116,5 +118,6 @@ 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 16.04.2018 13:07, Vladislav Shpilevoy wrote:
> Hello. Thanks for fixes. One more thing is left - please,
> add tests on incorrect update and incorrect upsert, as I
> asked you in two previous reviews.
>
> And in tests you does not need to do insert + get. Insert returns
> the tuple, so you can do t = s:insert(), t:update/upsert ...
>
> On 16/04/2018 10:47, Kirill Shcherbatov wrote:
>> I've fixed tests and wrong code style with tabs.
>>
>> From f9803f8d0b411abac02e35d5e4120ff0ce7e9341 Mon Sep 17 00:00:00 2001
>> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
>> Date: Mon, 16 Apr 2018 10:44:10 +0300
>> Subject: [PATCH] Fixed tests and spaces
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.
2018-04-16 16:51 ` Kirill Shcherbatov
@ 2018-04-16 17:14 ` Vladislav Shpilevoy
2018-04-18 12:28 ` Vladimir Davydov
0 siblings, 1 reply; 17+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-16 17:14 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov, Vladimir Davydov
Now the patch looks good to me, except commit messages, that I fixed on
branch - see the new ones, Kirill. The commit message header must fit
into 50 symbols, contain "<subsystem>:" prefix, be in imperative
mood, and do not contain dot at the end. That is instead of
Fixed lost tuple format.
You must write
tuple: fix lost tuple format
The commit message body must fit into 80 symbols.
Vova, can you please take a look on the patch?
On 16/04/2018 19:51, Kirill Shcherbatov wrote:
> From 84f2fc22124c2a2d1450de9b818f61c9c1cf31b0 Mon Sep 17 00:00:00 2001
> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> Date: Mon, 16 Apr 2018 19:50:04 +0300
> Subject: [PATCH] New tests for invalid update and upsert
>
> ---
> test/engine/update.result | 10 ++++++++++
> test/engine/update.test.lua | 5 ++++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/test/engine/update.result b/test/engine/update.result
> index 48398d2..0ae22ac 100644
> --- a/test/engine/update.result
> +++ b/test/engine/update.result
> @@ -719,6 +719,11 @@ 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)
> ---
> @@ -741,6 +746,11 @@ 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 c53e458..3ff7e62 100644
> --- a/test/engine/update.test.lua
> +++ b/test/engine/update.test.lua
> @@ -108,6 +108,8 @@ 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
> @@ -116,5 +118,6 @@ 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()
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.
2018-04-16 17:14 ` Vladislav Shpilevoy
@ 2018-04-18 12:28 ` Vladimir Davydov
2018-04-18 12:55 ` Kirill Shcherbatov
0 siblings, 1 reply; 17+ messages in thread
From: Vladimir Davydov @ 2018-04-18 12:28 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Kirill Shcherbatov
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.
2018-04-18 12:28 ` Vladimir Davydov
@ 2018-04-18 12:55 ` Kirill Shcherbatov
2018-04-22 15:15 ` Vladislav Shpilevoy
0 siblings, 1 reply; 17+ messages in thread
From: Kirill Shcherbatov @ 2018-04-18 12:55 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladimir Davydov, v.shpilevoy
> 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.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.
2018-04-18 12:55 ` Kirill Shcherbatov
@ 2018-04-22 15:15 ` Vladislav Shpilevoy
2018-04-28 6:56 ` Kirill Shcherbatov
0 siblings, 1 reply; 17+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-22 15:15 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov; +Cc: Vladimir Davydov
On 18/04/2018 15:55, Kirill Shcherbatov wrote:
>> 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.
Please, describe why we need inlined update here. 'Vlad insisted' is not a reason.
>
>> 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.
Vova means, that if tuple:transform would use non-default tuple format,
the tests will pass anyway. Please, add a test on transform, that would not
pass, if tuple uses non-default format.
> 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)
This test will pass even if we would use non-default tuple format.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.
2018-04-22 15:15 ` Vladislav Shpilevoy
@ 2018-04-28 6:56 ` Kirill Shcherbatov
0 siblings, 0 replies; 17+ messages in thread
From: Kirill Shcherbatov @ 2018-04-28 6:56 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
> Please, describe why we need inlined update here. 'Vlad insisted' is not a reason.
Unlike update and upsert, transform creates a new tuple with runtime format;
Moreover, the function inlining allowed to make allocations in the region, what is preferable.
> Vova means, that if tuple:transform would use non-default tuple format,
> the tests will pass anyway. Please, add a test on transform, that would not
> pass, if tuple uses non-default format.
> This test will pass even if we would use non-default tuple format
diff --git a/test/engine/update.result b/test/engine/update.result
index b73037e..6ab4436 100644
--- a/test/engine/update.result
+++ b/test/engine/update.result
@@ -728,11 +728,7 @@ aa:update({{'=',2, 666}})
aa:transform(-1, 1)
---
- [1, 'ssss', '3', '4', '5', '6']
-...
-aa:transform(1, 6)
---
-- ['7']
-...
aa = nil
---
...
diff --git a/test/engine/update.test.lua b/test/engine/update.test.lua
index e2a2265..3ff7e62 100644
--- a/test/engine/update.test.lua
+++ b/test/engine/update.test.lua
@@ -112,7 +112,6 @@ aa.VAL
aa:update({{'=',2, 666}})
-- test transform integrity
aa:transform(-1, 1)
-aa:transform(1, 6)
aa = nil
s:upsert({2, 'wwwww'}, {{'=', 2, 'wwwww'}})
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v2 0/2] Fix lost format on update operation
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-28 9:29 ` Vladislav Shpilevoy
2 siblings, 0 replies; 17+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-28 9:29 UTC (permalink / raw)
To: tarantool-patches, Konstantin Osipov; +Cc: Kirill Shcherbatov
Now the patchset LGTM. Kostja, please, look at this.
On 06/04/2018 19:24, Kirill Shcherbatov wrote:
> Branch: http://github.com/tarantool/tarantool/tree/gh-3051-lost-format-while-tuple-update
> Issue: https://github.com/tarantool/tarantool/issues/3051
>
> Kirill Shcherbatov (2):
> Fixed invalid check in lbox_tuple_transform.
> Fixed lost format on update and upsert operations.
>
> src/box/lua/tuple.c | 6 ++--
> 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, 171 insertions(+), 18 deletions(-)
>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-04-28 9:29 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox