* [PATCH 1/8] vinyl: move update optimization from write iterator to tx
2018-10-14 18:16 [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask Vladimir Davydov
@ 2018-10-14 18:16 ` Vladimir Davydov
2018-10-23 7:35 ` [tarantool-patches] " Konstantin Osipov
2018-10-14 18:16 ` [PATCH 2/8] vinyl: factor out common code of UPDATE and UPSERT Vladimir Davydov
` (8 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: Vladimir Davydov @ 2018-10-14 18:16 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
An UPDATE operation is written as DELETE + REPLACE to secondary indexes.
We write those statements to the memory level even if the UPDATE doesn't
actually update columns indexed by a secondary key. We filter them out
in the write iterator when the memory level is dumped. That's what we
use vy_stmt_column_mask for.
Actually, there's no point to keep those statements until dump - we
could as well filter them out when the transaction is committed. This
would even save some memory. This wouldn't hurt read operations, because
point lookup doesn't work for secondary indexes by design and so we have
to read all sources, including disk, on every read from a secondary
index.
That said, let's move update optimization from the write iterator to
vy_tx_commit. This is a step towards removing vy_stmt_column_mask.
---
src/box/vy_tx.c | 11 +++-
src/box/vy_write_iterator.c | 44 +++++-----------
src/box/vy_write_iterator.h | 26 ++-------
test/unit/vy_cache.c | 9 ++--
test/unit/vy_iterators_helper.c | 22 +++-----
test/unit/vy_iterators_helper.h | 19 ++-----
test/unit/vy_point_lookup.c | 2 +-
test/unit/vy_write_iterator.c | 105 ++-----------------------------------
test/unit/vy_write_iterator.result | 54 ++++++++-----------
9 files changed, 64 insertions(+), 228 deletions(-)
diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index 957c87f0..f83f9981 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -50,6 +50,7 @@
#include "trigger.h"
#include "trivia/util.h"
#include "tuple.h"
+#include "column_mask.h"
#include "vy_cache.h"
#include "vy_lsm.h"
#include "vy_mem.h"
@@ -651,6 +652,12 @@ vy_tx_prepare(struct vy_tx *tx)
if (v->is_overwritten)
continue;
+ /* Skip statements which don't change this secondary key. */
+ if (lsm->index_id > 0 &&
+ key_update_can_be_skipped(lsm->key_def->column_mask,
+ vy_stmt_column_mask(v->stmt)))
+ continue;
+
if (lsm->index_id > 0 && repsert == NULL && delete == NULL) {
/*
* This statement is for a secondary index,
@@ -1001,7 +1008,7 @@ vy_tx_set(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt)
if (old != NULL && vy_stmt_type(stmt) != IPROTO_UPSERT) {
/*
* Inherit the column mask of the overwritten statement
- * so as not to skip both statements on dump.
+ * so as not to skip both statements on commit.
*/
uint64_t column_mask = vy_stmt_column_mask(stmt);
if (column_mask != UINT64_MAX)
@@ -1020,7 +1027,7 @@ vy_tx_set(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt)
* second field, but the column mask will say it does.
*
* To discard DELETE statements in the write iterator
- * (see optimization #6), we turn a REPLACE into an
+ * (see optimization #5), we turn a REPLACE into an
* INSERT in case the REPLACE was generated by an
* update that changed secondary key fields. So we
* can't tolerate inaccuracy in a column mask.
diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index 1b54e539..17b80685 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -32,7 +32,6 @@
#include "vy_mem.h"
#include "vy_run.h"
#include "vy_upsert.h"
-#include "column_mask.h"
#include "fiber.h"
#define HEAP_FORWARD_DECLARATION
@@ -632,7 +631,7 @@ vy_write_iterator_deferred_delete(struct vy_write_iterator *stream,
/**
* Build the history of the current key.
- * Apply optimizations 1, 2 and 3 (@sa vy_write_iterator.h).
+ * Apply optimizations 1 and 2 (@sa vy_write_iterator.h).
* When building a history, some statements can be
* skipped (e.g. multiple REPLACE statements on the same key),
* but nothing can be merged yet, since we don't know the first
@@ -690,7 +689,6 @@ vy_write_iterator_build_history(struct vy_write_iterator *stream,
int current_rv_i = 0;
int64_t current_rv_lsn = vy_write_iterator_get_vlsn(stream, 0);
int64_t merge_until_lsn = vy_write_iterator_get_vlsn(stream, 1);
- uint64_t key_mask = stream->cmp_def->column_mask;
while (true) {
*is_first_insert = vy_stmt_type(src->tuple) == IPROTO_INSERT;
@@ -702,9 +700,7 @@ vy_write_iterator_build_history(struct vy_write_iterator *stream,
* generated by an update operation, it can be
* turned into an INSERT.
*/
- uint64_t stmt_mask = vy_stmt_column_mask(src->tuple);
- if (stmt_mask != UINT64_MAX &&
- !key_update_can_be_skipped(stmt_mask, key_mask))
+ if (vy_stmt_column_mask(src->tuple) != UINT64_MAX)
*is_first_insert = true;
}
@@ -758,6 +754,12 @@ vy_write_iterator_build_history(struct vy_write_iterator *stream,
goto next_lsn;
}
+ rc = vy_write_iterator_push_rv(stream, src->tuple,
+ current_rv_i);
+ if (rc != 0)
+ break;
+ ++*count;
+
/*
* Optimization 2: skip statements overwritten
* by a REPLACE or DELETE.
@@ -765,34 +767,12 @@ vy_write_iterator_build_history(struct vy_write_iterator *stream,
if (vy_stmt_type(src->tuple) == IPROTO_REPLACE ||
vy_stmt_type(src->tuple) == IPROTO_INSERT ||
vy_stmt_type(src->tuple) == IPROTO_DELETE) {
- uint64_t stmt_mask = vy_stmt_column_mask(src->tuple);
- /*
- * Optimization 3: skip statements which
- * do not change this secondary key.
- */
- if (!stream->is_primary &&
- key_update_can_be_skipped(key_mask, stmt_mask))
- goto next_lsn;
-
- rc = vy_write_iterator_push_rv(stream, src->tuple,
- current_rv_i);
- if (rc != 0)
- break;
- ++*count;
current_rv_i++;
current_rv_lsn = merge_until_lsn;
merge_until_lsn =
vy_write_iterator_get_vlsn(stream,
current_rv_i + 1);
- goto next_lsn;
}
-
- assert(vy_stmt_type(src->tuple) == IPROTO_UPSERT);
- rc = vy_write_iterator_push_rv(stream, src->tuple,
- current_rv_i);
- if (rc != 0)
- break;
- ++*count;
next_lsn:
rc = vy_write_iterator_merge_step(stream);
if (rc != 0)
@@ -844,7 +824,7 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
assert(rv->history != NULL);
struct vy_write_history *h = rv->history;
/*
- * Optimization 5: discard a DELETE statement referenced
+ * Optimization 4: discard a DELETE statement referenced
* by a read view if it is preceded by another DELETE for
* the same key.
*/
@@ -923,7 +903,7 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
}
if (is_first_insert && vy_stmt_type(rv->tuple) == IPROTO_DELETE) {
/*
- * Optimization 6: discard the first DELETE if
+ * Optimization 5: discard the first DELETE if
* the oldest statement for the current key among
* all sources is an INSERT and hence there's no
* statements for this key in older runs or the
@@ -939,11 +919,11 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
* If the oldest statement among all sources is an
* INSERT, convert the first REPLACE to an INSERT
* so that if the key gets deleted later, we will
- * be able invoke optimization #6 to discard the
+ * be able invoke optimization #5 to discard the
* DELETE statement.
*
* Otherwise convert the first INSERT to a REPLACE
- * so as not to trigger optimization #6 on the next
+ * so as not to trigger optimization #5 on the next
* compaction.
*/
uint32_t size;
diff --git a/src/box/vy_write_iterator.h b/src/box/vy_write_iterator.h
index 3b9b535a..4ae3815d 100644
--- a/src/box/vy_write_iterator.h
+++ b/src/box/vy_write_iterator.h
@@ -117,27 +117,7 @@
* skip keep skip merge
*
* ---------------------------------------------------------------
- * Optimization #3: when compacting runs of a secondary key, skip
- * statements, which do not update this key.
- *
- * --------
- * SAME KEY
- * --------
- * VLSN(i) VLSN(i+1)
- * Masks | |
- * intersection:| not 0 0 0 not 0 not 0 |
- * | ANY DELETE REPLACE ANY ... REPLACE |
- * \______/\_______________/\___________________/
- * merge skip merge
- *
- * Details: when UPDATE is executed by Tarantool, it is
- * transformed into DELETE + REPLACE or a single REPLACE. But it
- * is only necessary to write anything into the secondary key if
- * such UPDATE changes any field, which is part of the key.
- * All other UPDATEs can be simply skipped.
- *
- * ---------------------------------------------------------------
- * Optimization #4: use older REPLACE/DELETE to apply UPSERTs and
+ * Optimization #3: use older REPLACE/DELETE to apply UPSERTs and
* convert them into a single REPLACE. When compaction includes
* the last level, absence of REPLACE or DELETE is equivalent
* to a DELETE, and UPSERT can be converted to REPLACE as well.
@@ -166,7 +146,7 @@
* vy_write_iterator_build_read_views.
*
* ---------------------------------------------------------------
- * Optimization #5: discard a tautological DELETE statement, i.e.
+ * Optimization #4: discard a tautological DELETE statement, i.e.
* a statement that was not removed from the history because it
* is referenced by read view, but that is preceeded by another
* DELETE and hence not needed.
@@ -182,7 +162,7 @@
* skip keep skip discard
*
* ---------------------------------------------------------------
- * Optimization #6: discard the first DELETE if the oldest
+ * Optimization #5: discard the first DELETE if the oldest
* statement for the current key among all sources is an INSERT.
* Rationale: if a key's history starts from an INSERT, there is
* either no statements for this key in older runs or the latest
diff --git a/test/unit/vy_cache.c b/test/unit/vy_cache.c
index 5d296aa6..d46d6c3f 100644
--- a/test/unit/vy_cache.c
+++ b/test/unit/vy_cache.c
@@ -18,8 +18,7 @@ test_basic()
struct tuple_format *format;
create_test_cache(fields, types, lengthof(fields), &cache, &key_def,
&format);
- struct tuple *select_all = vy_new_simple_stmt(format, NULL,
- &key_template);
+ struct tuple *select_all = vy_new_simple_stmt(format, &key_template);
struct mempool history_node_pool;
mempool_create(&history_node_pool, cord_slab_cache(),
@@ -96,7 +95,7 @@ test_basic()
for (int i = 0; i < 4; ++i)
vy_cache_iterator_next(&itr, &history, &unused);
ret = vy_history_last_stmt(&history);
- ok(vy_stmt_are_same(ret, &chain1[3], format, NULL),
+ ok(vy_stmt_are_same(ret, &chain1[3], format),
"next_key * 4");
/*
@@ -115,11 +114,11 @@ test_basic()
* the last_stmt. So restore on chain1[0], but the result
* must be chain1[1].
*/
- struct tuple *last_stmt = vy_new_simple_stmt(format, NULL, &chain1[0]);
+ struct tuple *last_stmt = vy_new_simple_stmt(format, &chain1[0]);
ok(vy_cache_iterator_restore(&itr, last_stmt, &history, &unused) >= 0,
"restore");
ret = vy_history_last_stmt(&history);
- ok(vy_stmt_are_same(ret, &chain1[1], format, NULL),
+ ok(vy_stmt_are_same(ret, &chain1[1], format),
"restore on position after last");
tuple_unref(last_stmt);
diff --git a/test/unit/vy_iterators_helper.c b/test/unit/vy_iterators_helper.c
index 40d8d6a1..58eaa220 100644
--- a/test/unit/vy_iterators_helper.c
+++ b/test/unit/vy_iterators_helper.c
@@ -42,7 +42,6 @@ vy_iterator_C_test_finish()
struct tuple *
vy_new_simple_stmt(struct tuple_format *format,
- struct tuple_format *format_with_colmask,
const struct vy_stmt_template *templ)
{
if (templ == NULL)
@@ -59,9 +58,6 @@ vy_new_simple_stmt(struct tuple_format *format,
++i;
}
size += mp_sizeof_array(i);
- fail_if(templ->optimize_update && templ->type == IPROTO_UPSERT);
- if (templ->optimize_update)
- format = format_with_colmask;
/* Encode the statement. */
char *buf = (char *) malloc(size);
@@ -119,7 +115,6 @@ vy_new_simple_stmt(struct tuple_format *format,
ops = mp_encode_int(ops, templ->upsert_value);
operations[0].iov_base = tmp;
operations[0].iov_len = ops - tmp;
- fail_if(templ->optimize_update);
ret = vy_stmt_new_upsert(format, buf, pos, operations, 1);
fail_if(ret == NULL);
break;
@@ -137,16 +132,13 @@ vy_new_simple_stmt(struct tuple_format *format,
free(buf);
vy_stmt_set_lsn(ret, templ->lsn);
vy_stmt_set_flags(ret, templ->flags);
- if (templ->optimize_update)
- vy_stmt_set_column_mask(ret, 0);
return ret;
}
const struct tuple *
vy_mem_insert_template(struct vy_mem *mem, const struct vy_stmt_template *templ)
{
- struct tuple *stmt = vy_new_simple_stmt(mem->format,
- mem->format_with_colmask, templ);
+ struct tuple *stmt = vy_new_simple_stmt(mem->format, templ);
struct tuple *region_stmt = vy_stmt_dup_lsregion(stmt,
&mem->env->allocator, mem->generation);
assert(region_stmt != NULL);
@@ -166,12 +158,12 @@ vy_cache_insert_templates_chain(struct vy_cache *cache,
const struct vy_stmt_template *key_templ,
enum iterator_type order)
{
- struct tuple *key = vy_new_simple_stmt(format, NULL, key_templ);
+ struct tuple *key = vy_new_simple_stmt(format, key_templ);
struct tuple *prev_stmt = NULL;
struct tuple *stmt = NULL;
for (uint i = 0; i < length; ++i) {
- stmt = vy_new_simple_stmt(format, NULL, &chain[i]);
+ stmt = vy_new_simple_stmt(format, &chain[i]);
vy_cache_add(cache, stmt, prev_stmt, key, order);
if (i != 0)
tuple_unref(prev_stmt);
@@ -187,7 +179,7 @@ void
vy_cache_on_write_template(struct vy_cache *cache, struct tuple_format *format,
const struct vy_stmt_template *templ)
{
- struct tuple *written = vy_new_simple_stmt(format, NULL, templ);
+ struct tuple *written = vy_new_simple_stmt(format, templ);
vy_cache_on_write(cache, written, NULL);
tuple_unref(written);
}
@@ -250,13 +242,11 @@ destroy_test_cache(struct vy_cache *cache, struct key_def *def,
bool
vy_stmt_are_same(const struct tuple *actual,
const struct vy_stmt_template *expected,
- struct tuple_format *format,
- struct tuple_format *format_with_colmask)
+ struct tuple_format *format)
{
if (vy_stmt_type(actual) != expected->type)
return false;
- struct tuple *tmp = vy_new_simple_stmt(format, format_with_colmask,
- expected);
+ struct tuple *tmp = vy_new_simple_stmt(format, expected);
fail_if(tmp == NULL);
uint32_t a_len, b_len;
const char *a, *b;
diff --git a/test/unit/vy_iterators_helper.h b/test/unit/vy_iterators_helper.h
index 24641df3..9690f684 100644
--- a/test/unit/vy_iterators_helper.h
+++ b/test/unit/vy_iterators_helper.h
@@ -43,13 +43,10 @@
#define vyend 99999999
#define MAX_FIELDS_COUNT 100
#define STMT_TEMPLATE(lsn, type, ...) \
-{ { __VA_ARGS__, vyend }, IPROTO_##type, lsn, false, 0, 0, 0 }
-
-#define STMT_TEMPLATE_OPTIMIZED(lsn, type, ...) \
-{ { __VA_ARGS__, vyend }, IPROTO_##type, lsn, true, 0, 0, 0 }
+{ { __VA_ARGS__, vyend }, IPROTO_##type, lsn, 0, 0, 0 }
#define STMT_TEMPLATE_FLAGS(lsn, type, flags, ...) \
-{ { __VA_ARGS__, vyend }, IPROTO_##type, lsn, false, flags, 0, 0 }
+{ { __VA_ARGS__, vyend }, IPROTO_##type, lsn, flags, 0, 0 }
#define STMT_TEMPLATE_DEFERRED_DELETE(lsn, type, ...) \
STMT_TEMPLATE_FLAGS(lsn, type, VY_STMT_DEFERRED_DELETE, __VA_ARGS__)
@@ -83,11 +80,6 @@ struct vy_stmt_template {
enum iproto_type type;
/** Statement lsn. */
int64_t lsn;
- /*
- * True, if statement must have column mask, that allows
- * to skip it in the write_iterator.
- */
- bool optimize_update;
/** Statement flags. */
uint8_t flags;
/*
@@ -103,15 +95,12 @@ struct vy_stmt_template {
* Create a new vinyl statement using the specified template.
*
* @param format
- * @param format_with_colmask Format for statements with a
- * colmask.
* @param templ Statement template.
*
* @return Created statement.
*/
struct tuple *
vy_new_simple_stmt(struct tuple_format *format,
- struct tuple_format *format_with_colmask,
const struct vy_stmt_template *templ);
/**
@@ -210,15 +199,13 @@ destroy_test_cache(struct vy_cache *cache, struct key_def *def,
* @param stmt Actual value.
* @param templ Expected value.
* @param format Template statement format.
- * @param format_with_colmask Template statement format with colmask.
*
* @retval stmt === template.
*/
bool
vy_stmt_are_same(const struct tuple *actual,
const struct vy_stmt_template *expected,
- struct tuple_format *format,
- struct tuple_format *format_with_colmask);
+ struct tuple_format *format);
#if defined(__cplusplus)
}
diff --git a/test/unit/vy_point_lookup.c b/test/unit/vy_point_lookup.c
index dd33bbec..642cb4ce 100644
--- a/test/unit/vy_point_lookup.c
+++ b/test/unit/vy_point_lookup.c
@@ -274,7 +274,7 @@ test_basic()
struct vy_stmt_template tmpl_key =
STMT_TEMPLATE(0, SELECT, i);
struct tuple *key = vy_new_simple_stmt(format,
- pk->mem_format_with_colmask, &tmpl_key);
+ &tmpl_key);
struct tuple *res;
rc = vy_point_lookup(pk, NULL, &prv, key, &res);
tuple_unref(key);
diff --git a/test/unit/vy_write_iterator.c b/test/unit/vy_write_iterator.c
index 337e27ac..bb0eb8d3 100644
--- a/test/unit/vy_write_iterator.c
+++ b/test/unit/vy_write_iterator.c
@@ -119,8 +119,7 @@ compare_write_iterator_results(const struct vy_stmt_template *content,
if (ret == NULL)
break;
fail_if(i >= expected_count);
- ok(vy_stmt_are_same(ret, &expected[i], mem->format,
- mem->format_with_colmask),
+ ok(vy_stmt_are_same(ret, &expected[i], mem->format),
"stmt %d is correct", i);
++i;
} while (ret != NULL);
@@ -129,7 +128,7 @@ compare_write_iterator_results(const struct vy_stmt_template *content,
for (i = 0; i < handler.count; i++) {
fail_if(i >= deferred_count);
ok(vy_stmt_are_same(handler.stmt[i], &deferred[i],
- handler.format, NULL),
+ handler.format),
"deferred stmt %d is correct", i);
}
if (deferred != NULL) {
@@ -149,7 +148,7 @@ void
test_basic(void)
{
header();
- plan(66);
+ plan(58);
{
/*
* STATEMENT: REPL REPL REPL DEL REPL REPL REPL REPL REPL REPL
@@ -312,54 +311,6 @@ test_basic(void)
}
{
/*
- * STATEMENT: REPL DEL REPL REPL
- * LSN: 5 6 6 7
- * READ VIEW: *
- * \_______________/\_______/
- * \_____/\______/
- * merge skip as
- * optimized
- * update
- * DEL and REPL with lsn 6 can be skipped for read view 6 for
- * secondary index, because they do not change secondary key.
- */
- const struct vy_stmt_template content[] = {
- STMT_TEMPLATE(5, REPLACE, 1, 1),
- STMT_TEMPLATE_OPTIMIZED(6, DELETE, 1),
- STMT_TEMPLATE_OPTIMIZED(6, REPLACE, 1, 2),
- STMT_TEMPLATE(7, REPLACE, 1, 3)
- };
- const struct vy_stmt_template expected[] = { content[3], content[0] };
- const int vlsns[] = {6};
- int content_count = sizeof(content) / sizeof(content[0]);
- int expected_count = sizeof(expected) / sizeof(expected[0]);
- int vlsns_count = sizeof(vlsns) / sizeof(vlsns[0]);
- compare_write_iterator_results(content, content_count,
- expected, expected_count, NULL, 0,
- vlsns, vlsns_count, false, true);
-}
-{
-/*
- * STATEMENT: DEL REPL
- * LSN: 6 6
- * \______/
- * skip both as optimized update
- */
- const struct vy_stmt_template content[] = {
- STMT_TEMPLATE_OPTIMIZED(6, DELETE, 1),
- STMT_TEMPLATE_OPTIMIZED(6, REPLACE, 1, 2),
- };
- const struct vy_stmt_template expected[] = {};
- const int vlsns[] = {};
- int content_count = sizeof(content) / sizeof(content[0]);
- int expected_count = sizeof(expected) / sizeof(expected[0]);
- int vlsns_count = sizeof(vlsns) / sizeof(vlsns[0]);
- compare_write_iterator_results(content, content_count,
- expected, expected_count, NULL, 0,
- vlsns, vlsns_count, false, false);
-}
-{
-/*
* STATEMENT: UPS UPS UPS REPL
* LSN: 6 7 8 9
* READ VIEW: *
@@ -415,56 +366,6 @@ test_basic(void)
}
{
/*
- * STATEMENT: REPL DEL REPL
- * LSN: 6 7 7
- * \___/\__________/
- * merge skip as optimized update
- *
- * last_level = false.
- * Check if the key is not fully skipped in a case of optimized
- * update as the newest version.
- */
- const struct vy_stmt_template content[] = {
- STMT_TEMPLATE(6, REPLACE, 1, 1),
- STMT_TEMPLATE_OPTIMIZED(7, DELETE, 1),
- STMT_TEMPLATE_OPTIMIZED(7, REPLACE, 1, 2),
- };
- const struct vy_stmt_template expected[] = { content[0] };
- const int vlsns[] = {};
- int content_count = sizeof(content) / sizeof(content[0]);
- int expected_count = sizeof(expected) / sizeof(expected[0]);
- int vlsns_count = sizeof(vlsns) / sizeof(vlsns[0]);
- compare_write_iterator_results(content, content_count,
- expected, expected_count, NULL, 0,
- vlsns, vlsns_count, false, false);
-}
-{
-/*
- * STATEMENT: REPL DEL REPL
- * LSN: 6 7 7
- * \_________/|\___/
- * skip last level | skip as optimized
- * delete. | update.
- *
- * last_level = true. First apply 'last level DELETE' optimization
- * and only then the 'optimized UPDATE'.
- */
- const struct vy_stmt_template content[] = {
- STMT_TEMPLATE(6, REPLACE, 1, 1),
- STMT_TEMPLATE_OPTIMIZED(7, DELETE, 1),
- STMT_TEMPLATE_OPTIMIZED(7, REPLACE, 1, 2),
- };
- const struct vy_stmt_template expected[] = { content[2] };
- const int vlsns[] = {};
- int content_count = sizeof(content) / sizeof(content[0]);
- int expected_count = sizeof(expected) / sizeof(expected[0]);
- int vlsns_count = sizeof(vlsns) / sizeof(vlsns[0]);
- compare_write_iterator_results(content, content_count,
- expected, expected_count, NULL, 0,
- vlsns, vlsns_count, true, false);
-}
-{
-/*
* STATEMENT: REPL DEL REPL DEL REPL DEL
* LSN: 4 5 6 7 8 9
* READ VIEW: * * *
diff --git a/test/unit/vy_write_iterator.result b/test/unit/vy_write_iterator.result
index 4f95aeb9..a8011685 100644
--- a/test/unit/vy_write_iterator.result
+++ b/test/unit/vy_write_iterator.result
@@ -1,5 +1,5 @@
*** test_basic ***
-1..66
+1..58
ok 1 - stmt 0 is correct
ok 2 - stmt 1 is correct
ok 3 - stmt 2 is correct
@@ -24,46 +24,38 @@ ok 21 - correct results count
ok 22 - stmt 0 is correct
ok 23 - stmt 1 is correct
ok 24 - correct results count
-ok 25 - correct results count
-ok 26 - stmt 0 is correct
-ok 27 - stmt 1 is correct
+ok 25 - stmt 0 is correct
+ok 26 - stmt 1 is correct
+ok 27 - stmt 2 is correct
ok 28 - correct results count
ok 29 - stmt 0 is correct
-ok 30 - stmt 1 is correct
-ok 31 - stmt 2 is correct
-ok 32 - correct results count
-ok 33 - stmt 0 is correct
+ok 30 - correct results count
+ok 31 - stmt 0 is correct
+ok 32 - stmt 1 is correct
+ok 33 - stmt 2 is correct
ok 34 - correct results count
ok 35 - stmt 0 is correct
-ok 36 - correct results count
-ok 37 - stmt 0 is correct
+ok 36 - stmt 1 is correct
+ok 37 - stmt 2 is correct
ok 38 - correct results count
ok 39 - stmt 0 is correct
ok 40 - stmt 1 is correct
ok 41 - stmt 2 is correct
ok 42 - correct results count
-ok 43 - stmt 0 is correct
-ok 44 - stmt 1 is correct
-ok 45 - stmt 2 is correct
-ok 46 - correct results count
-ok 47 - stmt 0 is correct
-ok 48 - stmt 1 is correct
-ok 49 - stmt 2 is correct
+ok 43 - deferred stmt 0 is correct
+ok 44 - deferred stmt 1 is correct
+ok 45 - deferred stmt 2 is correct
+ok 46 - deferred stmt 3 is correct
+ok 47 - correct deferred stmt count
+ok 48 - stmt 0 is correct
+ok 49 - stmt 1 is correct
ok 50 - correct results count
-ok 51 - deferred stmt 0 is correct
-ok 52 - deferred stmt 1 is correct
-ok 53 - deferred stmt 2 is correct
-ok 54 - deferred stmt 3 is correct
+ok 51 - correct deferred stmt count
+ok 52 - stmt 0 is correct
+ok 53 - stmt 1 is correct
+ok 54 - correct results count
ok 55 - correct deferred stmt count
ok 56 - stmt 0 is correct
-ok 57 - stmt 1 is correct
-ok 58 - correct results count
-ok 59 - correct deferred stmt count
-ok 60 - stmt 0 is correct
-ok 61 - stmt 1 is correct
-ok 62 - correct results count
-ok 63 - correct deferred stmt count
-ok 64 - stmt 0 is correct
-ok 65 - correct results count
-ok 66 - correct deferred stmt count
+ok 57 - correct results count
+ok 58 - correct deferred stmt count
*** test_basic: done ***
--
2.11.0
^ permalink raw reply [flat|nested] 17+ messages in thread* [tarantool-patches] Re: [PATCH 1/8] vinyl: move update optimization from write iterator to tx
2018-10-14 18:16 ` [PATCH 1/8] vinyl: move update optimization from write iterator to tx Vladimir Davydov
@ 2018-10-23 7:35 ` Konstantin Osipov
0 siblings, 0 replies; 17+ messages in thread
From: Konstantin Osipov @ 2018-10-23 7:35 UTC (permalink / raw)
To: tarantool-patches
* Vladimir Davydov <vdavydov.dev@gmail.com> [18/10/15 10:15]:
> An UPDATE operation is written as DELETE + REPLACE to secondary indexes.
> We write those statements to the memory level even if the UPDATE doesn't
> actually update columns indexed by a secondary key. We filter them out
> in the write iterator when the memory level is dumped. That's what we
> use vy_stmt_column_mask for.
>
> Actually, there's no point to keep those statements until dump - we
> could as well filter them out when the transaction is committed. This
> would even save some memory. This wouldn't hurt read operations, because
> point lookup doesn't work for secondary indexes by design and so we have
> to read all sources, including disk, on every read from a secondary
> index.
>
> That said, let's move update optimization from the write iterator to
> vy_tx_commit. This is a step towards removing vy_stmt_column_mask.
OK to push.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/8] vinyl: factor out common code of UPDATE and UPSERT
2018-10-14 18:16 [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask Vladimir Davydov
2018-10-14 18:16 ` [PATCH 1/8] vinyl: move update optimization from write iterator to tx Vladimir Davydov
@ 2018-10-14 18:16 ` Vladimir Davydov
2018-10-23 7:36 ` [tarantool-patches] " Konstantin Osipov
2018-10-14 18:16 ` [PATCH 3/8] vinyl: do not use column mask as trigger for turning REPLACE into INSERT Vladimir Davydov
` (7 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: Vladimir Davydov @ 2018-10-14 18:16 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
This patch introduces a helper function vy_perform_update() that
performs operations common for UPDATE and UPSERT, namely replaces
a tuple in a transaction write set.
---
src/box/vinyl.c | 115 ++++++++++++++++++++++++--------------------------------
1 file changed, 50 insertions(+), 65 deletions(-)
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 33621813..869c4140 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1708,6 +1708,50 @@ vy_check_update(struct space *space, const struct vy_lsm *pk,
}
/**
+ * An UPDATE operation turns into a REPLACE statement in the
+ * primary index and into DELETE + INSERT in secondary indexes.
+ * This function performs an UPDATE operation in the given
+ * transaction write set after stmt->old_tuple and new_tuple
+ * have been initialized and checked.
+ */
+static int
+vy_perform_update(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
+ struct space *space, struct vy_lsm *pk, int64_t column_mask)
+{
+ assert(stmt->old_tuple != NULL);
+ assert(stmt->new_tuple != NULL);
+
+ if (vy_check_is_unique(env, tx, space, stmt->new_tuple) != 0)
+ return -1;
+ if (vy_tx_set(tx, pk, stmt->new_tuple) != 0)
+ return -1;
+ if (space->index_count == 1)
+ return 0;
+
+ struct tuple *delete;
+ delete = vy_stmt_new_surrogate_delete(pk->mem_format_with_colmask,
+ stmt->old_tuple);
+ if (delete == NULL)
+ return -1;
+ vy_stmt_set_column_mask(delete, column_mask);
+
+ for (uint32_t i = 1; i < space->index_count; ++i) {
+ struct vy_lsm *lsm = vy_lsm(space->index[i]);
+ if (vy_is_committed_one(env, lsm))
+ continue;
+ if (vy_tx_set(tx, lsm, delete) != 0)
+ goto error;
+ if (vy_tx_set(tx, lsm, stmt->new_tuple) != 0)
+ goto error;
+ }
+ tuple_unref(delete);
+ return 0;
+error:
+ tuple_unref(delete);
+ return -1;
+}
+
+/**
* Execute UPDATE in a vinyl space.
* @param env Vinyl environment.
* @param tx Current transaction.
@@ -1767,15 +1811,14 @@ vy_update(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
if (tuple_validate_raw(pk->mem_format, new_tuple))
return -1;
- struct tuple_format *mask_format = pk->mem_format_with_colmask;
if (space->index_count == 1) {
stmt->new_tuple = vy_stmt_new_replace(pk->mem_format, new_tuple,
new_tuple_end);
if (stmt->new_tuple == NULL)
return -1;
} else {
- stmt->new_tuple = vy_stmt_new_replace(mask_format, new_tuple,
- new_tuple_end);
+ stmt->new_tuple = vy_stmt_new_replace(pk->mem_format_with_colmask,
+ new_tuple, new_tuple_end);
if (stmt->new_tuple == NULL)
return -1;
vy_stmt_set_column_mask(stmt->new_tuple, column_mask);
@@ -1783,38 +1826,8 @@ vy_update(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
if (vy_check_update(space, pk, stmt->old_tuple, stmt->new_tuple,
column_mask) != 0)
return -1;
- if (vy_check_is_unique(env, tx, space, stmt->new_tuple) != 0)
- return -1;
-
- /*
- * In the primary index the tuple can be replaced without
- * the old tuple deletion.
- */
- if (vy_tx_set(tx, pk, stmt->new_tuple) != 0)
- return -1;
- if (space->index_count == 1)
- return 0;
- struct tuple *delete = vy_stmt_new_surrogate_delete(mask_format,
- stmt->old_tuple);
- if (delete == NULL)
- return -1;
- vy_stmt_set_column_mask(delete, column_mask);
-
- for (uint32_t i = 1; i < space->index_count; ++i) {
- lsm = vy_lsm(space->index[i]);
- if (vy_is_committed_one(env, lsm))
- continue;
- if (vy_tx_set(tx, lsm, delete) != 0)
- goto error;
- if (vy_tx_set(tx, lsm, stmt->new_tuple) != 0)
- goto error;
- }
- tuple_unref(delete);
- return 0;
-error:
- tuple_unref(delete);
- return -1;
+ return vy_perform_update(env, tx, stmt, space, pk, column_mask);
}
/**
@@ -2046,15 +2059,14 @@ vy_upsert(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
if (tuple_validate_raw(pk->mem_format, new_tuple))
return -1;
new_tuple_end = new_tuple + new_size;
- struct tuple_format *mask_format = pk->mem_format_with_colmask;
if (space->index_count == 1) {
stmt->new_tuple = vy_stmt_new_replace(pk->mem_format, new_tuple,
new_tuple_end);
if (stmt->new_tuple == NULL)
return -1;
} else {
- stmt->new_tuple = vy_stmt_new_replace(mask_format, new_tuple,
- new_tuple_end);
+ stmt->new_tuple = vy_stmt_new_replace(pk->mem_format_with_colmask,
+ new_tuple, new_tuple_end);
if (stmt->new_tuple == NULL)
return -1;
vy_stmt_set_column_mask(stmt->new_tuple, column_mask);
@@ -2068,34 +2080,7 @@ vy_upsert(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
*/
return 0;
}
- if (vy_check_is_unique(env, tx, space, stmt->new_tuple) != 0)
- return -1;
- if (vy_tx_set(tx, pk, stmt->new_tuple))
- return -1;
- if (space->index_count == 1)
- return 0;
-
- /* Replace in secondary indexes works as delete insert. */
- struct tuple *delete = vy_stmt_new_surrogate_delete(mask_format,
- stmt->old_tuple);
- if (delete == NULL)
- return -1;
- vy_stmt_set_column_mask(delete, column_mask);
-
- for (uint32_t i = 1; i < space->index_count; ++i) {
- struct vy_lsm *lsm = vy_lsm(space->index[i]);
- if (vy_is_committed_one(env, lsm))
- continue;
- if (vy_tx_set(tx, lsm, delete) != 0)
- goto error;
- if (vy_tx_set(tx, lsm, stmt->new_tuple) != 0)
- goto error;
- }
- tuple_unref(delete);
- return 0;
-error:
- tuple_unref(delete);
- return -1;
+ return vy_perform_update(env, tx, stmt, space, pk, column_mask);
}
/**
--
2.11.0
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 3/8] vinyl: do not use column mask as trigger for turning REPLACE into INSERT
2018-10-14 18:16 [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask Vladimir Davydov
2018-10-14 18:16 ` [PATCH 1/8] vinyl: move update optimization from write iterator to tx Vladimir Davydov
2018-10-14 18:16 ` [PATCH 2/8] vinyl: factor out common code of UPDATE and UPSERT Vladimir Davydov
@ 2018-10-14 18:16 ` Vladimir Davydov
2018-10-23 7:37 ` [tarantool-patches] " Konstantin Osipov
2018-10-14 18:16 ` [PATCH 4/8] vinyl: explicitly pass column mask to vy_tx_set Vladimir Davydov
` (6 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: Vladimir Davydov @ 2018-10-14 18:16 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
If a REPLACE statement was generated by an UPDATE operation that updated
a column indexed by a secondary key, we can turn it into INSERT when the
secondary index is dumped, because there can't be an older statement
with the same key other than DELETE. Currently, we use the statement
column mask to detect such REPLACEs in the write iterator, but I'm
planning to get rid of vy_stmt_column_mask so let's instead introduce
a new statement flag to mark such REPLACEs.
---
src/box/vinyl.c | 3 +++
src/box/vy_stmt.c | 8 ++++++++
src/box/vy_stmt.h | 11 ++++++++++-
src/box/vy_write_iterator.c | 5 ++---
4 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 869c4140..be21d9d9 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1723,6 +1723,9 @@ vy_perform_update(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
if (vy_check_is_unique(env, tx, space, stmt->new_tuple) != 0)
return -1;
+
+ vy_stmt_set_flags(stmt->new_tuple, VY_STMT_UPDATE);
+
if (vy_tx_set(tx, pk, stmt->new_tuple) != 0)
return -1;
if (space->index_count == 1)
diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
index ebe64300..bb9e1edb 100644
--- a/src/box/vy_stmt.c
+++ b/src/box/vy_stmt.c
@@ -61,6 +61,14 @@ static inline uint8_t
vy_stmt_persistent_flags(const struct tuple *stmt, bool is_primary)
{
uint8_t mask = VY_STMT_FLAGS_ALL;
+
+ /*
+ * This flag is only used by the write iterator to turn
+ * in-memory REPLACEs into INSERTs on dump so no need to
+ * persist it.
+ */
+ mask &= ~VY_STMT_UPDATE;
+
if (!is_primary) {
/*
* Do not store VY_STMT_DEFERRED_DELETE flag in
diff --git a/src/box/vy_stmt.h b/src/box/vy_stmt.h
index 73e18ca7..4a67b3c7 100644
--- a/src/box/vy_stmt.h
+++ b/src/box/vy_stmt.h
@@ -98,9 +98,18 @@ enum {
*/
VY_STMT_SKIP_READ = 1 << 1,
/**
+ * This flag is set for those REPLACE statements that were
+ * generated by UPDATE operations. It is used by the write
+ * iterator to turn such REPLACEs into INSERTs in secondary
+ * indexes so that they can get annihilated with DELETEs on
+ * compaction. It is never written to disk.
+ */
+ VY_STMT_UPDATE = 1 << 2,
+ /**
* Bit mask of all statement flags.
*/
- VY_STMT_FLAGS_ALL = VY_STMT_DEFERRED_DELETE | VY_STMT_SKIP_READ,
+ VY_STMT_FLAGS_ALL = (VY_STMT_DEFERRED_DELETE | VY_STMT_SKIP_READ |
+ VY_STMT_UPDATE),
};
/**
diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index 17b80685..0b6ab0a8 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -694,14 +694,13 @@ vy_write_iterator_build_history(struct vy_write_iterator *stream,
*is_first_insert = vy_stmt_type(src->tuple) == IPROTO_INSERT;
if (!stream->is_primary &&
- vy_stmt_type(src->tuple) == IPROTO_REPLACE) {
+ (vy_stmt_flags(src->tuple) & VY_STMT_UPDATE) != 0) {
/*
* If a REPLACE stored in a secondary index was
* generated by an update operation, it can be
* turned into an INSERT.
*/
- if (vy_stmt_column_mask(src->tuple) != UINT64_MAX)
- *is_first_insert = true;
+ *is_first_insert = true;
}
/*
--
2.11.0
^ permalink raw reply [flat|nested] 17+ messages in thread* [tarantool-patches] Re: [PATCH 3/8] vinyl: do not use column mask as trigger for turning REPLACE into INSERT
2018-10-14 18:16 ` [PATCH 3/8] vinyl: do not use column mask as trigger for turning REPLACE into INSERT Vladimir Davydov
@ 2018-10-23 7:37 ` Konstantin Osipov
0 siblings, 0 replies; 17+ messages in thread
From: Konstantin Osipov @ 2018-10-23 7:37 UTC (permalink / raw)
To: tarantool-patches
* Vladimir Davydov <vdavydov.dev@gmail.com> [18/10/15 10:15]:
> If a REPLACE statement was generated by an UPDATE operation that updated
> a column indexed by a secondary key, we can turn it into INSERT when the
> secondary index is dumped, because there can't be an older statement
> with the same key other than DELETE. Currently, we use the statement
> column mask to detect such REPLACEs in the write iterator, but I'm
> planning to get rid of vy_stmt_column_mask so let's instead introduce
> a new statement flag to mark such REPLACEs.
OK to push.
> ---
> src/box/vinyl.c | 3 +++
> src/box/vy_stmt.c | 8 ++++++++
> src/box/vy_stmt.h | 11 ++++++++++-
> src/box/vy_write_iterator.c | 5 ++---
> 4 files changed, 23 insertions(+), 4 deletions(-)
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/8] vinyl: explicitly pass column mask to vy_tx_set
2018-10-14 18:16 [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask Vladimir Davydov
` (2 preceding siblings ...)
2018-10-14 18:16 ` [PATCH 3/8] vinyl: do not use column mask as trigger for turning REPLACE into INSERT Vladimir Davydov
@ 2018-10-14 18:16 ` Vladimir Davydov
2018-10-14 18:16 ` [PATCH 5/8] vinyl: explicitly pass column mask to vy_check_is_unique Vladimir Davydov
` (5 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Vladimir Davydov @ 2018-10-14 18:16 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
This patch is a preparation for removing vy_stmt_column_mask.
---
src/box/vinyl.c | 8 +++++---
src/box/vy_tx.c | 27 +++++++++++++--------------
src/box/vy_tx.h | 11 ++++++++++-
3 files changed, 28 insertions(+), 18 deletions(-)
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index be21d9d9..bd007645 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1726,7 +1726,7 @@ vy_perform_update(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
vy_stmt_set_flags(stmt->new_tuple, VY_STMT_UPDATE);
- if (vy_tx_set(tx, pk, stmt->new_tuple) != 0)
+ if (vy_tx_set_with_colmask(tx, pk, stmt->new_tuple, column_mask) != 0)
return -1;
if (space->index_count == 1)
return 0;
@@ -1742,9 +1742,11 @@ vy_perform_update(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
struct vy_lsm *lsm = vy_lsm(space->index[i]);
if (vy_is_committed_one(env, lsm))
continue;
- if (vy_tx_set(tx, lsm, delete) != 0)
+ if (vy_tx_set_with_colmask(tx, lsm, delete,
+ column_mask) != 0)
goto error;
- if (vy_tx_set(tx, lsm, stmt->new_tuple) != 0)
+ if (vy_tx_set_with_colmask(tx, lsm, stmt->new_tuple,
+ column_mask) != 0)
goto error;
}
tuple_unref(delete);
diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index f83f9981..d1027425 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -213,7 +213,8 @@ tx_manager_destroy_read_view(struct tx_manager *xm,
}
static struct txv *
-txv_new(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt)
+txv_new(struct vy_tx *tx, struct vy_lsm *lsm,
+ struct tuple *stmt, uint64_t column_mask)
{
struct tx_manager *xm = tx->xm;
struct txv *v = mempool_alloc(&xm->txv_mempool);
@@ -227,6 +228,7 @@ txv_new(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt)
v->stmt = stmt;
tuple_ref(stmt);
v->region_stmt = NULL;
+ v->column_mask = column_mask;
v->tx = tx;
v->is_first_insert = false;
v->is_overwritten = false;
@@ -587,7 +589,8 @@ vy_tx_handle_deferred_delete(struct vy_tx *tx, struct txv *v)
int rc = 0;
for (uint32_t i = 1; i < space->index_count; i++) {
struct vy_lsm *lsm = vy_lsm(space->index[i]);
- struct txv *delete_txv = txv_new(tx, lsm, delete_stmt);
+ struct txv *delete_txv = txv_new(tx, lsm, delete_stmt,
+ UINT64_MAX);
if (delete_txv == NULL) {
rc = -1;
break;
@@ -655,7 +658,7 @@ vy_tx_prepare(struct vy_tx *tx)
/* Skip statements which don't change this secondary key. */
if (lsm->index_id > 0 &&
key_update_can_be_skipped(lsm->key_def->column_mask,
- vy_stmt_column_mask(v->stmt)))
+ v->column_mask))
continue;
if (lsm->index_id > 0 && repsert == NULL && delete == NULL) {
@@ -955,7 +958,8 @@ vy_tx_track_point(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt)
}
int
-vy_tx_set(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt)
+vy_tx_set_with_colmask(struct vy_tx *tx, struct vy_lsm *lsm,
+ struct tuple *stmt, uint64_t column_mask)
{
assert(vy_stmt_type(stmt) != 0);
/**
@@ -987,7 +991,7 @@ vy_tx_set(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt)
}
/* Allocate a MVCC container. */
- struct txv *v = txv_new(tx, lsm, stmt);
+ struct txv *v = txv_new(tx, lsm, stmt, column_mask);
if (applied != NULL)
tuple_unref(applied);
if (v == NULL)
@@ -1005,15 +1009,12 @@ vy_tx_set(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt)
if (old == NULL && vy_stmt_type(stmt) == IPROTO_INSERT)
v->is_first_insert = true;
- if (old != NULL && vy_stmt_type(stmt) != IPROTO_UPSERT) {
+ if (old != NULL) {
/*
* Inherit the column mask of the overwritten statement
* so as not to skip both statements on commit.
*/
- uint64_t column_mask = vy_stmt_column_mask(stmt);
- if (column_mask != UINT64_MAX)
- vy_stmt_set_column_mask(stmt, column_mask |
- vy_stmt_column_mask(old->stmt));
+ v->column_mask |= old->column_mask;
}
if (lsm->index_id > 0 && vy_stmt_type(stmt) == IPROTO_REPLACE &&
@@ -1038,10 +1039,8 @@ vy_tx_set(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt)
* key bits in the column mask to ensure that no REPLACE
* statement will be written for this secondary key.
*/
- uint64_t column_mask = vy_stmt_column_mask(stmt);
- if (column_mask != UINT64_MAX)
- vy_stmt_set_column_mask(stmt, column_mask &
- ~lsm->cmp_def->column_mask);
+ if (v->column_mask != UINT64_MAX)
+ v->column_mask &= ~lsm->cmp_def->column_mask;
}
v->overwritten = old;
diff --git a/src/box/vy_tx.h b/src/box/vy_tx.h
index b201abd7..87066091 100644
--- a/src/box/vy_tx.h
+++ b/src/box/vy_tx.h
@@ -85,6 +85,8 @@ struct txv {
struct tuple *stmt;
/** Statement allocated on vy_mem->allocator. */
const struct tuple *region_stmt;
+ /** Mask of columns modified by this operation. */
+ uint64_t column_mask;
/** Next in the transaction log. */
struct stailq_entry next_in_log;
/** Member the transaction write set. */
@@ -371,7 +373,14 @@ vy_tx_track_point(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt);
/** Add a statement to a transaction. */
int
-vy_tx_set(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt);
+vy_tx_set_with_colmask(struct vy_tx *tx, struct vy_lsm *lsm,
+ struct tuple *stmt, uint64_t column_mask);
+
+static inline int
+vy_tx_set(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt)
+{
+ return vy_tx_set_with_colmask(tx, lsm, stmt, UINT64_MAX);
+}
/**
* Iterator over the write set of a transaction.
--
2.11.0
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 5/8] vinyl: explicitly pass column mask to vy_check_is_unique
2018-10-14 18:16 [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask Vladimir Davydov
` (3 preceding siblings ...)
2018-10-14 18:16 ` [PATCH 4/8] vinyl: explicitly pass column mask to vy_tx_set Vladimir Davydov
@ 2018-10-14 18:16 ` Vladimir Davydov
2018-10-14 18:16 ` [PATCH 6/8] vinyl: zap vy_stmt_column_mask and mem_format_with_colmask Vladimir Davydov
` (4 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Vladimir Davydov @ 2018-10-14 18:16 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
This patch is a preparation for removing vy_stmt_column_mask.
---
src/box/vinyl.c | 32 ++++++++++++++++++++------------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index bd007645..4e6c5d56 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1475,9 +1475,6 @@ vy_check_is_unique_secondary(struct vy_tx *tx, const struct vy_read_view **rv,
if (!lsm->check_is_unique)
return 0;
- if (key_update_can_be_skipped(lsm->key_def->column_mask,
- vy_stmt_column_mask(stmt)))
- return 0;
if (lsm->key_def->is_nullable &&
vy_tuple_key_contains_null(stmt, lsm->key_def))
return 0;
@@ -1517,17 +1514,22 @@ vy_check_is_unique_secondary(struct vy_tx *tx, const struct vy_read_view **rv,
/**
* Check if insertion of a new tuple violates unique constraint
* of any index of the space.
- * @param env Vinyl environment.
- * @param tx Current transaction.
- * @param space Space to check.
- * @param stmt New tuple.
+ * @param env Vinyl environment.
+ * @param tx Current transaction.
+ * @param space Space to check.
+ * @param stmt New tuple.
+ * @param column_mask Mask of columns changed by the operation.
+ * Used to optimize out uniqueness check in
+ * secondary indexes when an inserted tuple
+ * is a result of an UPDATE operation.
*
* @retval 0 Success, unique constraint is satisfied.
* @retval -1 Duplicate is found or read error occurred.
*/
static int
vy_check_is_unique(struct vy_env *env, struct vy_tx *tx,
- struct space *space, struct tuple *stmt)
+ struct space *space, struct tuple *stmt,
+ uint64_t column_mask)
{
assert(space->index_count > 0);
assert(vy_stmt_type(stmt) == IPROTO_INSERT ||
@@ -1560,6 +1562,9 @@ vy_check_is_unique(struct vy_env *env, struct vy_tx *tx,
*/
for (uint32_t i = 1; i < space->index_count; i++) {
struct vy_lsm *lsm = vy_lsm(space->index[i]);
+ if (key_update_can_be_skipped(lsm->key_def->column_mask,
+ column_mask))
+ continue;
if (vy_check_is_unique_secondary(tx, rv, space_name(space),
index_name_by_id(space, i),
lsm, stmt) != 0)
@@ -1721,7 +1726,8 @@ vy_perform_update(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
assert(stmt->old_tuple != NULL);
assert(stmt->new_tuple != NULL);
- if (vy_check_is_unique(env, tx, space, stmt->new_tuple) != 0)
+ if (vy_check_is_unique(env, tx, space, stmt->new_tuple,
+ column_mask) != 0)
return -1;
vy_stmt_set_flags(stmt->new_tuple, VY_STMT_UPDATE);
@@ -1853,7 +1859,7 @@ vy_insert_first_upsert(struct vy_env *env, struct vy_tx *tx,
assert(tx != NULL && tx->state == VINYL_TX_READY);
assert(space->index_count > 0);
assert(vy_stmt_type(stmt) == IPROTO_INSERT);
- if (vy_check_is_unique(env, tx, space, stmt) != 0)
+ if (vy_check_is_unique(env, tx, space, stmt, COLUMN_MASK_FULL) != 0)
return -1;
struct vy_lsm *pk = vy_lsm(space->index[0]);
assert(pk->index_id == 0);
@@ -2121,7 +2127,8 @@ vy_insert(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
request->tuple_end);
if (stmt->new_tuple == NULL)
return -1;
- if (vy_check_is_unique(env, tx, space, stmt->new_tuple) != 0)
+ if (vy_check_is_unique(env, tx, space, stmt->new_tuple,
+ COLUMN_MASK_FULL) != 0)
return -1;
if (vy_tx_set(tx, pk, stmt->new_tuple) != 0)
return -1;
@@ -2173,7 +2180,8 @@ vy_replace(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
request->tuple_end);
if (stmt->new_tuple == NULL)
return -1;
- if (vy_check_is_unique(env, tx, space, stmt->new_tuple) != 0)
+ if (vy_check_is_unique(env, tx, space, stmt->new_tuple,
+ COLUMN_MASK_FULL) != 0)
return -1;
/*
* Get the overwritten tuple from the primary index if
--
2.11.0
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 6/8] vinyl: zap vy_stmt_column_mask and mem_format_with_colmask
2018-10-14 18:16 [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask Vladimir Davydov
` (4 preceding siblings ...)
2018-10-14 18:16 ` [PATCH 5/8] vinyl: explicitly pass column mask to vy_check_is_unique Vladimir Davydov
@ 2018-10-14 18:16 ` Vladimir Davydov
2018-10-14 18:16 ` [PATCH 7/8] tuple: zap tuple_format_dup Vladimir Davydov
` (3 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Vladimir Davydov @ 2018-10-14 18:16 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
Finally, these atrocities are not used anywhere and can be removed.
---
src/box/vinyl.c | 41 ++++++++++----------------------------
src/box/vy_lsm.c | 25 +++++------------------
src/box/vy_lsm.h | 8 --------
src/box/vy_mem.c | 14 ++++---------
src/box/vy_mem.h | 17 +++++++---------
src/box/vy_stmt.c | 12 -----------
src/box/vy_stmt.h | 44 -----------------------------------------
test/unit/vy_iterators_helper.c | 8 +-------
test/unit/vy_point_lookup.c | 11 ++++-------
9 files changed, 31 insertions(+), 149 deletions(-)
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 4e6c5d56..c1e70cb2 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1138,8 +1138,6 @@ vinyl_space_swap_index(struct space *old_space, struct space *new_space,
SWAP(old_lsm, new_lsm);
SWAP(old_lsm->check_is_unique, new_lsm->check_is_unique);
SWAP(old_lsm->mem_format, new_lsm->mem_format);
- SWAP(old_lsm->mem_format_with_colmask,
- new_lsm->mem_format_with_colmask);
SWAP(old_lsm->disk_format, new_lsm->disk_format);
SWAP(old_lsm->opts, new_lsm->opts);
key_def_swap(old_lsm->key_def, new_lsm->key_def);
@@ -1737,12 +1735,10 @@ vy_perform_update(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
if (space->index_count == 1)
return 0;
- struct tuple *delete;
- delete = vy_stmt_new_surrogate_delete(pk->mem_format_with_colmask,
- stmt->old_tuple);
+ struct tuple *delete = vy_stmt_new_surrogate_delete(pk->mem_format,
+ stmt->old_tuple);
if (delete == NULL)
return -1;
- vy_stmt_set_column_mask(delete, column_mask);
for (uint32_t i = 1; i < space->index_count; ++i) {
struct vy_lsm *lsm = vy_lsm(space->index[i]);
@@ -1821,19 +1817,10 @@ vy_update(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
*/
if (tuple_validate_raw(pk->mem_format, new_tuple))
return -1;
-
- if (space->index_count == 1) {
- stmt->new_tuple = vy_stmt_new_replace(pk->mem_format, new_tuple,
- new_tuple_end);
- if (stmt->new_tuple == NULL)
- return -1;
- } else {
- stmt->new_tuple = vy_stmt_new_replace(pk->mem_format_with_colmask,
- new_tuple, new_tuple_end);
- if (stmt->new_tuple == NULL)
- return -1;
- vy_stmt_set_column_mask(stmt->new_tuple, column_mask);
- }
+ stmt->new_tuple = vy_stmt_new_replace(pk->mem_format, new_tuple,
+ new_tuple_end);
+ if (stmt->new_tuple == NULL)
+ return -1;
if (vy_check_update(space, pk, stmt->old_tuple, stmt->new_tuple,
column_mask) != 0)
return -1;
@@ -2070,18 +2057,10 @@ vy_upsert(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
if (tuple_validate_raw(pk->mem_format, new_tuple))
return -1;
new_tuple_end = new_tuple + new_size;
- if (space->index_count == 1) {
- stmt->new_tuple = vy_stmt_new_replace(pk->mem_format, new_tuple,
- new_tuple_end);
- if (stmt->new_tuple == NULL)
- return -1;
- } else {
- stmt->new_tuple = vy_stmt_new_replace(pk->mem_format_with_colmask,
- new_tuple, new_tuple_end);
- if (stmt->new_tuple == NULL)
- return -1;
- vy_stmt_set_column_mask(stmt->new_tuple, column_mask);
- }
+ stmt->new_tuple = vy_stmt_new_replace(pk->mem_format, new_tuple,
+ new_tuple_end);
+ if (stmt->new_tuple == NULL)
+ return -1;
if (vy_check_update(space, pk, stmt->old_tuple, stmt->new_tuple,
column_mask) != 0) {
diag_log();
diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
index de5ad314..80341267 100644
--- a/src/box/vy_lsm.c
+++ b/src/box/vy_lsm.c
@@ -161,16 +161,6 @@ vy_lsm_new(struct vy_lsm_env *lsm_env, struct vy_cache_env *cache_env,
}
tuple_format_ref(lsm->disk_format);
- if (index_def->iid == 0) {
- lsm->mem_format_with_colmask =
- vy_tuple_format_new_with_colmask(format);
- if (lsm->mem_format_with_colmask == NULL)
- goto fail_mem_format_with_colmask;
- } else {
- lsm->mem_format_with_colmask = pk->mem_format_with_colmask;
- }
- tuple_format_ref(lsm->mem_format_with_colmask);
-
if (vy_lsm_stat_create(&lsm->stat) != 0)
goto fail_stat;
@@ -178,8 +168,8 @@ vy_lsm_new(struct vy_lsm_env *lsm_env, struct vy_cache_env *cache_env,
if (lsm->run_hist == NULL)
goto fail_run_hist;
- lsm->mem = vy_mem_new(mem_env, *lsm->env->p_generation,
- cmp_def, format, lsm->mem_format_with_colmask,
+ lsm->mem = vy_mem_new(mem_env, cmp_def, format,
+ *lsm->env->p_generation,
space_cache_version);
if (lsm->mem == NULL)
goto fail_mem;
@@ -215,8 +205,6 @@ fail_mem:
fail_run_hist:
vy_lsm_stat_destroy(&lsm->stat);
fail_stat:
- tuple_format_unref(lsm->mem_format_with_colmask);
-fail_mem_format_with_colmask:
tuple_format_unref(lsm->disk_format);
fail_format:
key_def_delete(cmp_def);
@@ -270,7 +258,6 @@ vy_lsm_delete(struct vy_lsm *lsm)
vy_range_tree_iter(lsm->tree, NULL, vy_range_tree_free_cb, NULL);
vy_range_heap_destroy(&lsm->range_heap);
tuple_format_unref(lsm->disk_format);
- tuple_format_unref(lsm->mem_format_with_colmask);
key_def_delete(lsm->cmp_def);
key_def_delete(lsm->key_def);
histogram_delete(lsm->run_hist);
@@ -807,9 +794,8 @@ vy_lsm_rotate_mem(struct vy_lsm *lsm)
struct vy_mem *mem;
assert(lsm->mem != NULL);
- mem = vy_mem_new(lsm->mem->env, *lsm->env->p_generation,
- lsm->cmp_def, lsm->mem_format,
- lsm->mem_format_with_colmask, space_cache_version);
+ mem = vy_mem_new(lsm->mem->env, lsm->cmp_def, lsm->mem_format,
+ *lsm->env->p_generation, space_cache_version);
if (mem == NULL)
return -1;
@@ -857,8 +843,7 @@ vy_lsm_set(struct vy_lsm *lsm, struct vy_mem *mem,
lsm->stat.memory.count.bytes += tuple_size(stmt);
/* Abort transaction if format was changed by DDL */
- if (format_id != tuple_format_id(mem->format_with_colmask) &&
- format_id != tuple_format_id(mem->format)) {
+ if (format_id != tuple_format_id(mem->format)) {
diag_set(ClientError, ER_TRANSACTION_CONFLICT);
return -1;
}
diff --git a/src/box/vy_lsm.h b/src/box/vy_lsm.h
index 2e026ced..d8d9f9d0 100644
--- a/src/box/vy_lsm.h
+++ b/src/box/vy_lsm.h
@@ -193,14 +193,6 @@ struct vy_lsm {
/** Tuple format of the space this LSM tree belongs to. */
struct tuple_format *mem_format;
/**
- * Format for tuples of type REPLACE or DELETE which
- * are a result of an UPDATE operation. Such tuples
- * contain a column mask which preserves the list
- * of actually changed columns. Used when creating
- * tuples for vy_mem, and used only by primary key.
- */
- struct tuple_format *mem_format_with_colmask;
- /**
* If this LSM tree is for a secondary index, the following
* variable points to the LSM tree of the primary index of
* the same space, otherwise it is set to NULL. Referenced
diff --git a/src/box/vy_mem.c b/src/box/vy_mem.c
index ccd079f0..f8b89d4e 100644
--- a/src/box/vy_mem.c
+++ b/src/box/vy_mem.c
@@ -96,9 +96,8 @@ vy_mem_tree_extent_free(void *ctx, void *p)
}
struct vy_mem *
-vy_mem_new(struct vy_mem_env *env, int64_t generation,
- struct key_def *cmp_def, struct tuple_format *format,
- struct tuple_format *format_with_colmask,
+vy_mem_new(struct vy_mem_env *env, struct key_def *cmp_def,
+ struct tuple_format *format, int64_t generation,
uint32_t space_cache_version)
{
struct vy_mem *index = calloc(1, sizeof(*index));
@@ -114,8 +113,6 @@ vy_mem_new(struct vy_mem_env *env, int64_t generation,
index->space_cache_version = space_cache_version;
index->format = format;
tuple_format_ref(format);
- index->format_with_colmask = format_with_colmask;
- tuple_format_ref(format_with_colmask);
vy_mem_tree_create(&index->tree, cmp_def,
vy_mem_tree_extent_alloc,
vy_mem_tree_extent_free, index);
@@ -129,7 +126,6 @@ vy_mem_delete(struct vy_mem *index)
{
index->env->tree_extent_size -= index->tree_extent_size;
tuple_format_unref(index->format);
- tuple_format_unref(index->format_with_colmask);
fiber_cond_destroy(&index->pin_cond);
TRASH(index);
free(index);
@@ -160,8 +156,7 @@ vy_mem_insert_upsert(struct vy_mem *mem, const struct tuple *stmt)
{
assert(vy_stmt_type(stmt) == IPROTO_UPSERT);
/* Check if the statement can be inserted in the vy_mem. */
- assert(stmt->format_id == tuple_format_id(mem->format_with_colmask) ||
- stmt->format_id == tuple_format_id(mem->format));
+ assert(stmt->format_id == tuple_format_id(mem->format));
/* The statement must be from a lsregion. */
assert(!vy_stmt_is_refable(stmt));
size_t size = tuple_size(stmt);
@@ -220,8 +215,7 @@ vy_mem_insert(struct vy_mem *mem, const struct tuple *stmt)
{
assert(vy_stmt_type(stmt) != IPROTO_UPSERT);
/* Check if the statement can be inserted in the vy_mem. */
- assert(stmt->format_id == tuple_format_id(mem->format_with_colmask) ||
- stmt->format_id == tuple_format_id(mem->format));
+ assert(stmt->format_id == tuple_format_id(mem->format));
/* The statement must be from a lsregion. */
assert(!vy_stmt_is_refable(stmt));
size_t size = tuple_size(stmt);
diff --git a/src/box/vy_mem.h b/src/box/vy_mem.h
index d6afeed1..b41c3c0b 100644
--- a/src/box/vy_mem.h
+++ b/src/box/vy_mem.h
@@ -194,12 +194,12 @@ struct vy_mem {
*/
int64_t generation;
/**
- * Format of vy_mem REPLACE and DELETE tuples without
- * column mask.
+ * Format of statements stored in this in-memory index.
+ * Note, the statements don't reference the format by
+ * themselves, instead it is referenced once by vy_mem.
+ * This allows us to drop vy_mem in O(1).
*/
struct tuple_format *format;
- /** Format of vy_mem tuples with column mask. */
- struct tuple_format *format_with_colmask;
/**
* Number of active writers to this index.
*
@@ -254,19 +254,16 @@ vy_mem_wait_pinned(struct vy_mem *mem)
* Instantiate a new in-memory level.
*
* @param env Vinyl memory environment.
- * @param generation Generation of statements stored in the tree.
* @param key_def key definition.
* @param format Format for REPLACE and DELETE tuples.
- * @param format_with_colmask Format for tuples, which have
- * column mask.
+ * @param generation Generation of statements stored in the tree.
* @param space_cache_version Data dictionary cache version
* @retval new vy_mem instance on success.
* @retval NULL on error, check diag.
*/
struct vy_mem *
-vy_mem_new(struct vy_mem_env *env, int64_t generation,
- struct key_def *cmp_def, struct tuple_format *format,
- struct tuple_format *format_with_colmask,
+vy_mem_new(struct vy_mem_env *env, struct key_def *cmp_def,
+ struct tuple_format *format, int64_t generation,
uint32_t space_cache_version);
/**
diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
index bb9e1edb..7082952b 100644
--- a/src/box/vy_stmt.c
+++ b/src/box/vy_stmt.c
@@ -748,15 +748,3 @@ vy_stmt_str(const struct tuple *stmt)
return "<failed to format statement>";
return buf;
}
-
-struct tuple_format *
-vy_tuple_format_new_with_colmask(struct tuple_format *mem_format)
-{
- struct tuple_format *format = tuple_format_dup(mem_format);
- if (format == NULL)
- return NULL;
- /* + size of column mask. */
- assert(format->extra_size == 0);
- format->extra_size = sizeof(uint64_t);
- return format;
-}
diff --git a/src/box/vy_stmt.h b/src/box/vy_stmt.h
index 4a67b3c7..c3381402 100644
--- a/src/box/vy_stmt.h
+++ b/src/box/vy_stmt.h
@@ -219,39 +219,6 @@ vy_stmt_set_n_upserts(struct tuple *stmt, uint8_t n)
*((uint8_t *)stmt - 1) = n;
}
-/** Get the column mask of the specified tuple. */
-static inline uint64_t
-vy_stmt_column_mask(const struct tuple *tuple)
-{
- enum iproto_type type = vy_stmt_type(tuple);
- assert(type == IPROTO_INSERT || type == IPROTO_REPLACE ||
- type == IPROTO_DELETE);
- (void) type;
- if (tuple_format(tuple)->extra_size == sizeof(uint64_t)) {
- /* Tuple has column mask */
- const char *extra = tuple_extra(tuple);
- return load_u64(extra);
- }
- return UINT64_MAX; /* return default value */
-}
-
-/**
- * Set the column mask in the tuple.
- * @param tuple Tuple to set column mask.
- * @param column_mask Bitmask of the updated columns.
- */
-static inline void
-vy_stmt_set_column_mask(struct tuple *tuple, uint64_t column_mask)
-{
- enum iproto_type type = vy_stmt_type(tuple);
- assert(type == IPROTO_INSERT || type == IPROTO_REPLACE ||
- type == IPROTO_DELETE);
- assert(tuple_format(tuple)->extra_size == sizeof(uint64_t));
- (void) type;
- char *extra = (char *) tuple_extra(tuple);
- store_u64(extra, column_mask);
-}
-
/**
* Free the tuple of a vinyl space.
* @pre tuple->refs == 0
@@ -710,17 +677,6 @@ const char *
vy_stmt_str(const struct tuple *stmt);
/**
- * Create a tuple format with column mask of an update operation.
- * @sa vy_index.column_mask, vy_can_skip_update().
- * @param mem_format A base tuple format.
- *
- * @retval not NULL Success.
- * @retval NULL Memory or format register error.
- */
-struct tuple_format *
-vy_tuple_format_new_with_colmask(struct tuple_format *mem_format);
-
-/**
* Check if a key of @a tuple contains NULL.
* @param tuple Tuple to check.
* @param def Key def to check by.
diff --git a/test/unit/vy_iterators_helper.c b/test/unit/vy_iterators_helper.c
index 58eaa220..dcd76e4c 100644
--- a/test/unit/vy_iterators_helper.c
+++ b/test/unit/vy_iterators_helper.c
@@ -205,14 +205,8 @@ create_test_mem(struct key_def *def)
0, NULL, 0, NULL);
fail_if(format == NULL);
- /* Create format with column mask */
- struct tuple_format *format_with_colmask =
- vy_tuple_format_new_with_colmask(format);
- assert(format_with_colmask != NULL);
-
/* Create mem */
- struct vy_mem *mem = vy_mem_new(&mem_env, 1, def, format,
- format_with_colmask, 0);
+ struct vy_mem *mem = vy_mem_new(&mem_env, def, format, 1, 0);
fail_if(mem == NULL);
return mem;
}
diff --git a/test/unit/vy_point_lookup.c b/test/unit/vy_point_lookup.c
index 642cb4ce..8cf0fb72 100644
--- a/test/unit/vy_point_lookup.c
+++ b/test/unit/vy_point_lookup.c
@@ -179,9 +179,8 @@ test_basic()
/* create second run */
struct vy_mem *run_mem =
- vy_mem_new(pk->mem->env, *pk->env->p_generation,
- pk->cmp_def, pk->mem_format,
- pk->mem_format_with_colmask, 0);
+ vy_mem_new(pk->mem->env, pk->cmp_def, pk->mem_format,
+ *pk->env->p_generation, 0);
for (size_t i = 0; i < num_of_keys; i++) {
if (!in_run2[i])
@@ -211,10 +210,8 @@ test_basic()
vy_run_unref(run);
/* create first run */
- run_mem =
- vy_mem_new(pk->mem->env, *pk->env->p_generation,
- pk->cmp_def, pk->mem_format,
- pk->mem_format_with_colmask, 0);
+ run_mem = vy_mem_new(pk->mem->env, pk->cmp_def, pk->mem_format,
+ *pk->env->p_generation, 0);
for (size_t i = 0; i < num_of_keys; i++) {
if (!in_run1[i])
--
2.11.0
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 7/8] tuple: zap tuple_format_dup
2018-10-14 18:16 [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask Vladimir Davydov
` (5 preceding siblings ...)
2018-10-14 18:16 ` [PATCH 6/8] vinyl: zap vy_stmt_column_mask and mem_format_with_colmask Vladimir Davydov
@ 2018-10-14 18:16 ` Vladimir Davydov
2018-10-14 18:16 ` [PATCH 8/8] tuple: zap tuple_extra Vladimir Davydov
` (2 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Vladimir Davydov @ 2018-10-14 18:16 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
This function was only used for creating a format for tuples with column
mask in vinyl. Not needed anymore and can be removed.
Anyway, it doesn't make much sense to duplciate a tuple format, because
it can be referenced instead. Besides, once JSON indexes are introcued,
duplicating a tuple format will be really painful. One more reason to
drop it now.
---
src/box/tuple_format.c | 22 ----------------------
src/box/tuple_format.h | 10 ----------
2 files changed, 32 deletions(-)
diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index 5f4899d9..c510ffd9 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -324,28 +324,6 @@ tuple_format1_can_store_format2_tuples(const struct tuple_format *format1,
return true;
}
-struct tuple_format *
-tuple_format_dup(struct tuple_format *src)
-{
- uint32_t total = sizeof(struct tuple_format) +
- src->field_count * sizeof(struct tuple_field);
- struct tuple_format *format = (struct tuple_format *) malloc(total);
- if (format == NULL) {
- diag_set(OutOfMemory, total, "malloc", "tuple format");
- return NULL;
- }
- memcpy(format, src, total);
- tuple_dictionary_ref(format->dict);
- format->id = FORMAT_ID_NIL;
- format->refs = 0;
- if (tuple_format_register(format) != 0) {
- tuple_format_destroy(format);
- free(format);
- return NULL;
- }
- return format;
-}
-
/** @sa declaration for details. */
int
tuple_init_field_map(const struct tuple_format *format, uint32_t *field_map,
diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h
index 344aada7..cbd4e4b4 100644
--- a/src/box/tuple_format.h
+++ b/src/box/tuple_format.h
@@ -233,16 +233,6 @@ tuple_format1_can_store_format2_tuples(const struct tuple_format *format1,
const struct tuple_format *format2);
/**
- * Register the duplicate of the specified format.
- * @param src Original format.
- *
- * @retval not NULL Success.
- * @retval NULL Memory or format register error.
- */
-struct tuple_format *
-tuple_format_dup(struct tuple_format *src);
-
-/**
* Returns the total size of tuple metadata of this format.
* See @link struct tuple @endlink for explanation of tuple layout.
*
--
2.11.0
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 8/8] tuple: zap tuple_extra
2018-10-14 18:16 [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask Vladimir Davydov
` (6 preceding siblings ...)
2018-10-14 18:16 ` [PATCH 7/8] tuple: zap tuple_format_dup Vladimir Davydov
@ 2018-10-14 18:16 ` Vladimir Davydov
2018-10-23 7:42 ` [tarantool-patches] " Konstantin Osipov
2018-10-23 7:32 ` [tarantool-patches] Re: [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask Konstantin Osipov
2018-10-24 11:13 ` Vladimir Davydov
9 siblings, 1 reply; 17+ messages in thread
From: Vladimir Davydov @ 2018-10-14 18:16 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
tuple_extra() allows to store arbitrary metadata inside tuples.
To use it, one should set extra_size when creating a tuple_format.
It was introduced for storing UPSERT counter or column mask inside
vinyl statements. Turned out that it wasn't really needed as UPSERT
counter can be stored on lsregion while column mask doesn't need to
be stored at all.
Actually, the whole idea of tuple_extra() is rather crooked: why
would we need it if we can inherit struct tuple instead, as we do
in case of memtx_tuple and vy_stmt? Accessing an inherited struct
is much more convenient than using tuple_extra().
So this patch gets rid of tuple_extra(). To do that, it partially
reverts the following commits:
6c0842e0302c vinyl: refactor vy_stmt_alloc()
74ff46d8e93d vinyl: add special format for tuples with column mask
11eb7816a30e Add extra size to tuple_format->field_map_size
---
src/box/blackhole.c | 2 +-
src/box/memtx_engine.c | 10 +++++-----
src/box/memtx_space.c | 2 +-
src/box/tuple.c | 12 ++++++------
src/box/tuple.h | 30 ++++++------------------------
src/box/tuple_format.c | 6 ++----
src/box/tuple_format.h | 23 +----------------------
src/box/vinyl.c | 4 ++--
src/box/vy_lsm.c | 5 ++---
src/box/vy_stmt.c | 8 ++++----
test/unit/vy_iterators_helper.c | 6 +++---
test/unit/vy_mem.c | 2 +-
test/unit/vy_point_lookup.c | 2 +-
13 files changed, 35 insertions(+), 77 deletions(-)
diff --git a/src/box/blackhole.c b/src/box/blackhole.c
index f979304e..08cbe77f 100644
--- a/src/box/blackhole.c
+++ b/src/box/blackhole.c
@@ -151,7 +151,7 @@ blackhole_engine_create_space(struct engine *engine, struct space_def *def,
/* Allocate tuples on runtime arena, but check space format. */
struct tuple_format *format;
- format = tuple_format_new(&tuple_format_runtime->vtab, NULL, 0, 0,
+ format = tuple_format_new(&tuple_format_runtime->vtab, NULL, 0,
def->fields, def->field_count, def->dict);
if (format == NULL) {
free(space);
diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index 5a5e87e6..cda578e1 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -1130,8 +1130,8 @@ memtx_tuple_new(struct tuple_format *format, const char *data, const char *end)
struct memtx_engine *memtx = (struct memtx_engine *)format->engine;
assert(mp_typeof(*data) == MP_ARRAY);
size_t tuple_len = end - data;
- size_t meta_size = tuple_format_meta_size(format);
- size_t total = sizeof(struct memtx_tuple) + meta_size + tuple_len;
+ size_t total = sizeof(struct memtx_tuple) + format->field_map_size +
+ tuple_len;
ERROR_INJECT(ERRINJ_TUPLE_ALLOC, {
diag_set(OutOfMemory, total, "slab allocator", "memtx_tuple");
@@ -1166,7 +1166,7 @@ memtx_tuple_new(struct tuple_format *format, const char *data, const char *end)
* tuple base, not from memtx_tuple, because the struct
* tuple is not the first field of the memtx_tuple.
*/
- tuple->data_offset = sizeof(struct tuple) + meta_size;
+ tuple->data_offset = sizeof(struct tuple) + format->field_map_size;
char *raw = (char *) tuple + tuple->data_offset;
uint32_t *field_map = (uint32_t *) raw;
memcpy(raw, data, tuple_len);
@@ -1184,8 +1184,8 @@ memtx_tuple_delete(struct tuple_format *format, struct tuple *tuple)
struct memtx_engine *memtx = (struct memtx_engine *)format->engine;
say_debug("%s(%p)", __func__, tuple);
assert(tuple->refs == 0);
- size_t total = sizeof(struct memtx_tuple) +
- tuple_format_meta_size(format) + tuple->bsize;
+ size_t total = sizeof(struct memtx_tuple) + format->field_map_size +
+ tuple->bsize;
tuple_format_unref(format);
struct memtx_tuple *memtx_tuple =
container_of(tuple, struct memtx_tuple, base);
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 08ae0daa..0ff89d7e 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -909,7 +909,7 @@ memtx_space_new(struct memtx_engine *memtx,
keys[key_count++] = index_def->key_def;
struct tuple_format *format =
- tuple_format_new(&memtx_tuple_format_vtab, keys, key_count, 0,
+ tuple_format_new(&memtx_tuple_format_vtab, keys, key_count,
def->fields, def->field_count, def->dict);
if (format == NULL) {
free(memtx_space);
diff --git a/src/box/tuple.c b/src/box/tuple.c
index c975e539..0bf4c61e 100644
--- a/src/box/tuple.c
+++ b/src/box/tuple.c
@@ -97,8 +97,8 @@ runtime_tuple_new(struct tuple_format *format, const char *data, const char *end
mp_tuple_assert(data, end);
size_t data_len = end - data;
- size_t meta_size = tuple_format_meta_size(format);
- size_t total = sizeof(struct tuple) + meta_size + data_len;
+ size_t total = sizeof(struct tuple) + format->field_map_size +
+ data_len;
struct tuple *tuple = (struct tuple *) smalloc(&runtime_alloc, total);
if (tuple == NULL) {
@@ -111,7 +111,7 @@ runtime_tuple_new(struct tuple_format *format, const char *data, const char *end
tuple->bsize = data_len;
tuple->format_id = tuple_format_id(format);
tuple_format_ref(format);
- tuple->data_offset = sizeof(struct tuple) + meta_size;
+ tuple->data_offset = sizeof(struct tuple) + format->field_map_size;
char *raw = (char *) tuple + tuple->data_offset;
uint32_t *field_map = (uint32_t *) raw;
memcpy(raw, data, data_len);
@@ -129,7 +129,7 @@ runtime_tuple_delete(struct tuple_format *format, struct tuple *tuple)
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) +
+ size_t total = sizeof(struct tuple) + format->field_map_size +
tuple->bsize;
tuple_format_unref(format);
smfree(&runtime_alloc, tuple, total);
@@ -223,7 +223,7 @@ tuple_init(field_name_hash_f hash)
* Create a format for runtime tuples
*/
tuple_format_runtime = tuple_format_new(&tuple_format_runtime_vtab,
- NULL, 0, 0, NULL, 0, NULL);
+ NULL, 0, NULL, 0, NULL);
if (tuple_format_runtime == NULL)
return -1;
@@ -395,7 +395,7 @@ box_tuple_format_new(struct key_def **keys, uint16_t key_count)
{
box_tuple_format_t *format =
tuple_format_new(&tuple_format_runtime_vtab,
- keys, key_count, 0, NULL, 0, NULL);
+ keys, key_count, NULL, 0, NULL);
if (format != NULL)
tuple_format_ref(format);
return format;
diff --git a/src/box/tuple.h b/src/box/tuple.h
index b638f508..5c6dc6b8 100644
--- a/src/box/tuple.h
+++ b/src/box/tuple.h
@@ -289,18 +289,12 @@ box_tuple_upsert(const box_tuple_t *tuple, const char *expr, const
/**
* An atom of Tarantool storage. Represents MsgPack Array.
* Tuple has the following structure:
- * format->tuple_meta_size bsize
- * +------------------------+-------------+
- * tuple_begin, ..., raw = | tuple_meta | MessagePack |
- * | +------------------------+-------------+
- * | ^
- * +---------------------------------------------data_offset
- *
- * tuple_meta structure:
- * +----------------------+-----------------------+
- * | extra_size | offset N ... offset 1 |
- * +----------------------+-----------------------+
- * @sa tuple_format_new() uint32 ... uint32
+ * uint32 uint32 bsize
+ * +-------------------+-------------+
+ * tuple_begin, ..., raw = | offN | ... | off1 | MessagePack |
+ * | +-------------------+-------------+
+ * | ^
+ * +---------------------------------------data_offset
*
* Each 'off_i' is the offset to the i-th indexed field.
*/
@@ -416,18 +410,6 @@ tuple_format(const struct tuple *tuple)
}
/**
- * Return extra data saved in tuple metadata.
- * @param tuple tuple
- * @return a pointer to extra data saved in tuple metadata.
- */
-static inline const char *
-tuple_extra(const struct tuple *tuple)
-{
- struct tuple_format *format = tuple_format(tuple);
- return tuple_data(tuple) - tuple_format_meta_size(format);
-}
-
-/**
* Instantiate a new engine-independent tuple from raw MsgPack Array data
* using runtime arena. Use this function to create a standalone tuple
* from Lua or C procedures.
diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index c510ffd9..1c4b2e6a 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -140,7 +140,7 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys,
assert(format->fields[0].offset_slot == TUPLE_OFFSET_SLOT_NIL);
size_t field_map_size = -current_slot * sizeof(uint32_t);
- if (field_map_size + format->extra_size > UINT16_MAX) {
+ if (field_map_size > UINT16_MAX) {
/** tuple->data_offset is 16 bits */
diag_set(ClientError, ER_INDEX_FIELD_COUNT_LIMIT,
-current_slot);
@@ -258,8 +258,7 @@ tuple_format_delete(struct tuple_format *format)
struct tuple_format *
tuple_format_new(struct tuple_format_vtab *vtab, struct key_def * const *keys,
- uint16_t key_count, uint16_t extra_size,
- const struct field_def *space_fields,
+ uint16_t key_count, const struct field_def *space_fields,
uint32_t space_field_count, struct tuple_dictionary *dict)
{
struct tuple_format *format =
@@ -268,7 +267,6 @@ tuple_format_new(struct tuple_format_vtab *vtab, struct key_def * const *keys,
return NULL;
format->vtab = *vtab;
format->engine = NULL;
- format->extra_size = extra_size;
format->is_temporary = false;
if (tuple_format_register(format) < 0) {
tuple_format_destroy(format);
diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h
index cbd4e4b4..345e41b0 100644
--- a/src/box/tuple_format.h
+++ b/src/box/tuple_format.h
@@ -130,12 +130,6 @@ struct tuple_format {
*/
bool is_temporary;
/**
- * The number of extra bytes to reserve in tuples before
- * field map.
- * \sa struct tuple
- */
- uint16_t extra_size;
- /**
* Size of field map of tuple in bytes.
* \sa struct tuple
*/
@@ -204,7 +198,6 @@ tuple_format_unref(struct tuple_format *format)
* @param vtab Virtual function table for specific engines.
* @param keys Array of key_defs of a space.
* @param key_count The number of keys in @a keys array.
- * @param extra_size Extra bytes to reserve in tuples metadata.
* @param space_fields Array of fields, defined in a space format.
* @param space_field_count Length of @a space_fields.
*
@@ -213,8 +206,7 @@ tuple_format_unref(struct tuple_format *format)
*/
struct tuple_format *
tuple_format_new(struct tuple_format_vtab *vtab, struct key_def * const *keys,
- uint16_t key_count, uint16_t extra_size,
- const struct field_def *space_fields,
+ uint16_t key_count, const struct field_def *space_fields,
uint32_t space_field_count, struct tuple_dictionary *dict);
/**
@@ -233,19 +225,6 @@ tuple_format1_can_store_format2_tuples(const struct tuple_format *format1,
const struct tuple_format *format2);
/**
- * Returns the total size of tuple metadata of this format.
- * See @link struct tuple @endlink for explanation of tuple layout.
- *
- * @param format Tuple Format.
- * @returns the total size of tuple metadata
- */
-static inline uint16_t
-tuple_format_meta_size(const struct tuple_format *format)
-{
- return format->extra_size + format->field_map_size;
-}
-
-/**
* Calculate minimal field count of tuples with specified keys and
* space format.
* @param keys Array of key definitions of indexes.
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index c1e70cb2..e4d97c9a 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -603,7 +603,7 @@ vinyl_engine_create_space(struct engine *engine, struct space_def *def,
keys[key_count++] = index_def->key_def;
struct tuple_format *format =
- tuple_format_new(&vy_tuple_format_vtab, keys, key_count, 0,
+ tuple_format_new(&vy_tuple_format_vtab, keys, key_count,
def->fields, def->field_count, def->dict);
if (format == NULL) {
free(space);
@@ -3007,7 +3007,7 @@ vy_send_lsm(struct vy_join_ctx *ctx, struct vy_lsm_recovery_info *lsm_info)
if (ctx->key_def == NULL)
goto out;
ctx->format = tuple_format_new(&vy_tuple_format_vtab, &ctx->key_def,
- 1, 0, NULL, 0, NULL);
+ 1, NULL, 0, NULL);
if (ctx->format == NULL)
goto out_free_key_def;
tuple_format_ref(ctx->format);
diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
index 80341267..e024e508 100644
--- a/src/box/vy_lsm.c
+++ b/src/box/vy_lsm.c
@@ -61,7 +61,7 @@ vy_lsm_env_create(struct vy_lsm_env *env, const char *path,
void *upsert_thresh_arg)
{
env->key_format = tuple_format_new(&vy_tuple_format_vtab,
- NULL, 0, 0, NULL, 0, NULL);
+ NULL, 0, NULL, 0, NULL);
if (env->key_format == NULL)
return -1;
tuple_format_ref(env->key_format);
@@ -154,8 +154,7 @@ vy_lsm_new(struct vy_lsm_env *lsm_env, struct vy_cache_env *cache_env,
lsm->disk_format = format;
} else {
lsm->disk_format = tuple_format_new(&vy_tuple_format_vtab,
- &cmp_def, 1, 0, NULL, 0,
- NULL);
+ &cmp_def, 1, NULL, 0, NULL);
if (lsm->disk_format == NULL)
goto fail_format;
}
diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
index 7082952b..d8384040 100644
--- a/src/box/vy_stmt.c
+++ b/src/box/vy_stmt.c
@@ -127,8 +127,8 @@ vy_tuple_delete(struct tuple_format *format, struct tuple *tuple)
static struct tuple *
vy_stmt_alloc(struct tuple_format *format, uint32_t bsize)
{
- uint32_t meta_size = tuple_format_meta_size(format);
- uint32_t total_size = sizeof(struct vy_stmt) + meta_size + bsize;
+ uint32_t total_size = sizeof(struct vy_stmt) + format->field_map_size +
+ bsize;
if (unlikely(total_size > vy_max_tuple_size)) {
diag_set(ClientError, ER_VINYL_MAX_TUPLE_SIZE,
(unsigned) total_size);
@@ -141,13 +141,13 @@ vy_stmt_alloc(struct tuple_format *format, uint32_t bsize)
return NULL;
}
say_debug("vy_stmt_alloc(format = %d %u, bsize = %zu) = %p",
- format->id, tuple_format_meta_size(format), bsize, tuple);
+ format->id, format->field_map_size, bsize, tuple);
tuple->refs = 1;
tuple->format_id = tuple_format_id(format);
if (cord_is_main())
tuple_format_ref(format);
tuple->bsize = bsize;
- tuple->data_offset = sizeof(struct vy_stmt) + meta_size;;
+ tuple->data_offset = sizeof(struct vy_stmt) + format->field_map_size;
vy_stmt_set_lsn(tuple, 0);
vy_stmt_set_type(tuple, 0);
vy_stmt_set_flags(tuple, 0);
diff --git a/test/unit/vy_iterators_helper.c b/test/unit/vy_iterators_helper.c
index dcd76e4c..7fad5600 100644
--- a/test/unit/vy_iterators_helper.c
+++ b/test/unit/vy_iterators_helper.c
@@ -21,7 +21,7 @@ vy_iterator_C_test_init(size_t cache_size)
tuple_init(NULL);
vy_cache_env_create(&cache_env, cord_slab_cache());
vy_cache_env_set_quota(&cache_env, cache_size);
- vy_key_format = tuple_format_new(&vy_tuple_format_vtab, NULL, 0, 0,
+ vy_key_format = tuple_format_new(&vy_tuple_format_vtab, NULL, 0,
NULL, 0, NULL);
tuple_format_ref(vy_key_format);
@@ -202,7 +202,7 @@ create_test_mem(struct key_def *def)
struct key_def * const defs[] = { def };
struct tuple_format *format =
tuple_format_new(&vy_tuple_format_vtab, defs, def->part_count,
- 0, NULL, 0, NULL);
+ NULL, 0, NULL);
fail_if(format == NULL);
/* Create mem */
@@ -219,7 +219,7 @@ create_test_cache(uint32_t *fields, uint32_t *types,
*def = box_key_def_new(fields, types, key_cnt);
assert(*def != NULL);
vy_cache_create(cache, &cache_env, *def, true);
- *format = tuple_format_new(&vy_tuple_format_vtab, def, 1, 0, NULL, 0,
+ *format = tuple_format_new(&vy_tuple_format_vtab, def, 1, NULL, 0,
NULL);
tuple_format_ref(*format);
}
diff --git a/test/unit/vy_mem.c b/test/unit/vy_mem.c
index 967aabe8..ebf3fbc7 100644
--- a/test/unit/vy_mem.c
+++ b/test/unit/vy_mem.c
@@ -77,7 +77,7 @@ test_iterator_restore_after_insertion()
/* Create format */
struct tuple_format *format = tuple_format_new(&vy_tuple_format_vtab,
- &key_def, 1, 0, NULL, 0,
+ &key_def, 1, NULL, 0,
NULL);
assert(format != NULL);
tuple_format_ref(format);
diff --git a/test/unit/vy_point_lookup.c b/test/unit/vy_point_lookup.c
index 8cf0fb72..65dafcb2 100644
--- a/test/unit/vy_point_lookup.c
+++ b/test/unit/vy_point_lookup.c
@@ -84,7 +84,7 @@ test_basic()
vy_cache_create(&cache, &cache_env, key_def, true);
struct tuple_format *format = tuple_format_new(&vy_tuple_format_vtab,
- &key_def, 1, 0, NULL, 0,
+ &key_def, 1, NULL, 0,
NULL);
isnt(format, NULL, "tuple_format_new is not NULL");
tuple_format_ref(format);
--
2.11.0
^ permalink raw reply [flat|nested] 17+ messages in thread* [tarantool-patches] Re: [PATCH 8/8] tuple: zap tuple_extra
2018-10-14 18:16 ` [PATCH 8/8] tuple: zap tuple_extra Vladimir Davydov
@ 2018-10-23 7:42 ` Konstantin Osipov
2018-10-23 8:32 ` Vladimir Davydov
0 siblings, 1 reply; 17+ messages in thread
From: Konstantin Osipov @ 2018-10-23 7:42 UTC (permalink / raw)
To: tarantool-patches
* Vladimir Davydov <vdavydov.dev@gmail.com> [18/10/15 10:15]:
> Actually, the whole idea of tuple_extra() is rather crooked: why
> would we need it if we can inherit struct tuple instead, as we do
> in case of memtx_tuple and vy_stmt? Accessing an inherited struct
> is much more convenient than using tuple_extra().
Because you don't want to inherit a tuple to store last access
time, or some information specific to functional index or row id.
That was the idea at least.
This patch and the entire patch set is OK to push, when we get to
implementing the above mentioned features we can consider putting
it back in again.
>
> So this patch gets rid of tuple_extra(). To do that, it partially
> reverts the following commits:
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [tarantool-patches] Re: [PATCH 8/8] tuple: zap tuple_extra
2018-10-23 7:42 ` [tarantool-patches] " Konstantin Osipov
@ 2018-10-23 8:32 ` Vladimir Davydov
0 siblings, 0 replies; 17+ messages in thread
From: Vladimir Davydov @ 2018-10-23 8:32 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches
On Tue, Oct 23, 2018 at 10:42:11AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [18/10/15 10:15]:
> > Actually, the whole idea of tuple_extra() is rather crooked: why
> > would we need it if we can inherit struct tuple instead, as we do
> > in case of memtx_tuple and vy_stmt? Accessing an inherited struct
> > is much more convenient than using tuple_extra().
>
> Because you don't want to inherit a tuple to store last access
> time, or some information specific to functional index or row id.
> That was the idea at least.
But I do want to inherit memtx_tuple for storing atime. It would look
much cleaner IMO.
Regarding functional indexes, in my understanding we don't want to store
any extra information in struct tuple, because that wouldn't work in
case of vinyl. Extra info should be stored right in tuple data IMHO or
may be we could implement functional indexes as shadow spaces, dunno...
This remains to be decided though.
Regarding row id. Again, wouldn't work in case of vinyl. Should be
stored in a hidden field rather than in tuple extra.
I agree that there may be problems with inheritance if we ever want to
store atime and something else (say ctime) for some spaces and only
atime or only ctime for others. But current implementation of tuple
extra wouldn't suit for this either and would need a redesign anyway.
>
> This patch and the entire patch set is OK to push, when we get to
> implementing the above mentioned features we can consider putting
> it back in again.
> >
> > So this patch gets rid of tuple_extra(). To do that, it partially
> > reverts the following commits:
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask
2018-10-14 18:16 [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask Vladimir Davydov
` (7 preceding siblings ...)
2018-10-14 18:16 ` [PATCH 8/8] tuple: zap tuple_extra Vladimir Davydov
@ 2018-10-23 7:32 ` Konstantin Osipov
2018-10-23 8:22 ` Vladimir Davydov
2018-10-24 11:13 ` Vladimir Davydov
9 siblings, 1 reply; 17+ messages in thread
From: Konstantin Osipov @ 2018-10-23 7:32 UTC (permalink / raw)
To: tarantool-patches
* Vladimir Davydov <vdavydov.dev@gmail.com> [18/10/15 10:15]:
> There are two formats for memory statements in Vinyl, vy_lsm::mem_format
> and vy_lsm::mem_format_with_colmask. The only difference between them is
> that the latter allows to store column mask inside tuples. It is used to
> optimize out tautological updates in the write iterator. Turns out that
> we can easily do without it as this patch set demonstrates.
>
> I've always wanted to drop this format because I hate the very idea of
> tuple_extra (that's why I dropped upsert format a while ago as well).
> However, the reason why I'm doing this now is that it is the only thing
> why we need tuple_format_dup. Currently, tuple_format_dup is basically a
> memcpy, but once JSON indexes are introduced it's going to become much
> more complex than just that so we'd better drop it now.
First, I disagree with both premises:
- we will need tuple_extra anyway, for capped collections
- you can't guarantee that a pretty basic method of an object such as creating
a deep copy will never be used in the future, so there is no
much point in attacking it now.
Yet I am happy to look at the patch trimming the code on the edges so a review
follows.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [tarantool-patches] Re: [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask
2018-10-23 7:32 ` [tarantool-patches] Re: [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask Konstantin Osipov
@ 2018-10-23 8:22 ` Vladimir Davydov
0 siblings, 0 replies; 17+ messages in thread
From: Vladimir Davydov @ 2018-10-23 8:22 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches
On Tue, Oct 23, 2018 at 10:32:16AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [18/10/15 10:15]:
> > There are two formats for memory statements in Vinyl, vy_lsm::mem_format
> > and vy_lsm::mem_format_with_colmask. The only difference between them is
> > that the latter allows to store column mask inside tuples. It is used to
> > optimize out tautological updates in the write iterator. Turns out that
> > we can easily do without it as this patch set demonstrates.
> >
> > I've always wanted to drop this format because I hate the very idea of
> > tuple_extra (that's why I dropped upsert format a while ago as well).
> > However, the reason why I'm doing this now is that it is the only thing
> > why we need tuple_format_dup. Currently, tuple_format_dup is basically a
> > memcpy, but once JSON indexes are introduced it's going to become much
> > more complex than just that so we'd better drop it now.
>
> First, I disagree with both premises:
>
> - we will need tuple_extra anyway, for capped collections
I won't, because tuple_extra is extremely inconvenient to use. I will
inherit memtx_tuple instead and access extra fields after casting tuple
to the child class. This will look much easier for understanding.
> - you can't guarantee that a pretty basic method of an object such as creating
> a deep copy will never be used in the future, so there is no
> much point in attacking it now.
Why would we need to make a deep copy of a tuple_format, even
theoretically, when we can simply reference it? Note, tuple_format
is immutable by design.
>
> Yet I am happy to look at the patch trimming the code on the edges so a review
> follows.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask
2018-10-14 18:16 [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask Vladimir Davydov
` (8 preceding siblings ...)
2018-10-23 7:32 ` [tarantool-patches] Re: [PATCH 0/8] Get rid of Vinyl's mem_format_with_colmask Konstantin Osipov
@ 2018-10-24 11:13 ` Vladimir Davydov
9 siblings, 0 replies; 17+ messages in thread
From: Vladimir Davydov @ 2018-10-24 11:13 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
On Sun, Oct 14, 2018 at 09:16:44PM +0300, Vladimir Davydov wrote:
> vinyl: move update optimization from write iterator to tx
> vinyl: factor out common code of UPDATE and UPSERT
> vinyl: do not use column mask as trigger for turning REPLACE into
> INSERT
> vinyl: explicitly pass column mask to vy_tx_set
> vinyl: explicitly pass column mask to vy_check_is_unique
> vinyl: zap vy_stmt_column_mask and mem_format_with_colmask
> tuple: zap tuple_format_dup
> tuple: zap tuple_extra
Pushed to 1.10-features
^ permalink raw reply [flat|nested] 17+ messages in thread