[PATCH 0/3] Fixes of a few Vinyl transaction manager issues

[PATCH 0/3] Fixes of a few Vinyl transaction manager issues

[Vladimir Davydov]

For more details see comments to the patches below.

https://github.com/tarantool/tarantool/issues/3693
https://github.com/tarantool/tarantool/issues/4242
https://github.com/tarantool/tarantool/issues/4248
https://github.com/tarantool/tarantool/commits/dv/vy-tx-commit-fixes

Vladimir Davydov (3):
  vinyl: fix secondary index divergence on update
  vinyl: don't produce deferred DELETE on commit if key isn't updated
  vinyl: fix deferred DELETE statement lost on commit

 src/box/vinyl.c                     |   7 +-
 src/box/vy_tx.c                     |  98 ++++++++++++------------
 src/box/vy_tx.h                     |  17 ++---
 test/engine/update.result           |  30 ++++++++
 test/engine/update.test.lua         |  12 +++
 test/vinyl/deferred_delete.result   | 145 ++++++++++++++++++++++++++++++++++++
 test/vinyl/deferred_delete.test.lua |  47 ++++++++++++
 test/vinyl/quota.result             |   2 +-
 test/vinyl/stat.result              |   6 +-
 test/vinyl/stat.test.lua            |   2 +-
 test/vinyl/write_iterator.result    |   4 +-
 test/vinyl/write_iterator.test.lua  |   4 +-
 12 files changed, 301 insertions(+), 73 deletions(-)

-- 
2.11.0

[PATCH 1/3] vinyl: fix secondary index divergence on update

[Vladimir Davydov]

If an UPDATE request doesn't touch key parts of a secondary index, we
don't need to write it to the index memory level or dump it to disk, as
this would only increase IO load. Historically, we use column mask set
by the UPDATE operation to skip secondary indexes that are not affected
by the operation on commit. However, there's a problem here: the column
mask isn't precise - it may have a bit set even if the corresponding
column doesn't really get updated, e.g. consider {'+', 2, 0}. Not taking
this into account may result in appearance of phantom tuples on disk as
the write iterator assumes that statements that have no effect aren't
written to secondary indexes (this is needed to apply INSERT+DELETE
"annihilation" optimization). We fixed that by clearing column mask bits
in vy_tx_set in case we detect that the key isn't changed, for more
details see #3607 and commit e72867cb9169 ("vinyl: fix appearance of
phantom tuple in secondary index after update"). It was rather an ugly
hack, but it worked.

However, it turned out that apart from looking hackish this code has
a nasty bug that may lead to tuples missing from secondary indexes.
Consider the following example:

  s = box.schema.space.create('test', {engine = 'vinyl'})
  s:create_index('pk')
  s:create_index('sk', {parts = {2, 'unsigned'}})
  s:insert{1, 1, 1}

  box.begin()
  s:update(1, {{'=', 2, 2}})
  s:update(1, {{'=', 3, 2}})
  box.commit()

The first update operation writes DELETE{1,1} and REPLACE{2,1} to the
secondary index write set. The second update replaces REPLACE{2,1} with
DELETE{2,1} and then with REPLACE{2,1}. When replacing DELETE{2,1} with
REPLACE{2,1} in the write set, we assume that the update doesn't modify
secondary index key parts and clear the column mask so as not to commit
a pointless request, see vy_tx_set. As a result, we skip the first
update too and get key {2,1} missing from the secondary index.

Actually, it was a dumb idea to use column mask to skip statements in
the first place, as there's a much easier way to filter out statements
that have no effect for secondary indexes. The thing is every DELETE
statement inserted into a secondary index write set acts as a "single
DELETE", i.e. there's exactly one older statement it is supposed to
purge. This is, because in contrast to the primary index we don't write
DELETE statements blindly - we always look up the tuple overwritten in
the primary index first. This means that REPLACE+DELETE for the same key
is basically a no-op and can be safely skip. Moreover, DELETE+REPLACE
can be treated as no-op, too, because secondary indexes don't store full
tuples hence all REPLACE statements for the same key are equivalent.
By marking such pair of statements as no-op in vy_tx_set, we guarantee
that no-op statements don't make it to secondary index memory or disk
levels.

Closes #4242
---
 src/box/vinyl.c                    |  7 ++--
 src/box/vy_tx.c                    | 70 ++++++++++++--------------------------
 src/box/vy_tx.h                    | 17 ++++-----
 test/engine/update.result          | 30 ++++++++++++++++
 test/engine/update.test.lua        | 12 +++++++
 test/vinyl/write_iterator.result   |  2 +-
 test/vinyl/write_iterator.test.lua |  2 +-
 7 files changed, 74 insertions(+), 66 deletions(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 9372e5f7..9e731d13 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1870,7 +1870,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_with_colmask(tx, pk, stmt->new_tuple, column_mask) != 0)
+	if (vy_tx_set(tx, pk, stmt->new_tuple) != 0)
 		return -1;
 	if (space->index_count == 1)
 		return 0;
@@ -1884,10 +1884,9 @@ 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(env, lsm))
 			continue;
-		if (vy_tx_set_with_colmask(tx, lsm, delete, column_mask) != 0)
+		if (vy_tx_set(tx, lsm, delete) != 0)
 			goto error;
-		if (vy_tx_set_with_colmask(tx, lsm, stmt->new_tuple,
-					column_mask) != 0)
+		if (vy_tx_set(tx, lsm, stmt->new_tuple) != 0)
 			goto error;
 	}
 	tuple_unref(delete);
diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index b1dee0fa..aedd0610 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -51,7 +51,6 @@
 #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"
@@ -212,8 +211,7 @@ tx_manager_destroy_read_view(struct tx_manager *xm,
 }
 
 static struct txv *
-txv_new(struct vy_tx *tx, struct vy_lsm *lsm,
-	struct vy_entry entry, uint64_t column_mask)
+txv_new(struct vy_tx *tx, struct vy_lsm *lsm, struct vy_entry entry)
 {
 	struct tx_manager *xm = tx->xm;
 	struct txv *v = mempool_alloc(&xm->txv_mempool);
@@ -227,9 +225,9 @@ txv_new(struct vy_tx *tx, struct vy_lsm *lsm,
 	v->entry = entry;
 	tuple_ref(entry.stmt);
 	v->region_stmt = NULL;
-	v->column_mask = column_mask;
 	v->tx = tx;
 	v->is_first_insert = false;
+	v->is_nop = false;
 	v->is_overwritten = false;
 	v->overwritten = NULL;
 	xm->write_set_size += tuple_size(entry.stmt);
@@ -631,8 +629,7 @@ vy_tx_handle_deferred_delete(struct vy_tx *tx, struct txv *v)
 		struct vy_lsm *lsm = vy_lsm(space->index[i]);
 		struct vy_entry entry;
 		vy_stmt_foreach_entry(entry, delete_stmt, lsm->cmp_def) {
-			struct txv *delete_txv = txv_new(tx, lsm, entry,
-							 UINT64_MAX);
+			struct txv *delete_txv = txv_new(tx, lsm, entry);
 			if (delete_txv == NULL) {
 				rc = -1;
 				break;
@@ -704,13 +701,7 @@ vy_tx_prepare(struct vy_tx *tx)
 		assert(lsm->space_id == current_space_id);
 
 		/* Do not save statements that was overwritten by the same 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,
-					      v->column_mask))
+		if (v->is_overwritten || v->is_nop)
 			continue;
 
 		if (lsm->index_id > 0 && repsert == NULL && delete == NULL) {
@@ -1038,8 +1029,7 @@ vy_tx_track_point(struct vy_tx *tx, struct vy_lsm *lsm, struct vy_entry entry)
  * are multiple entries of a single statement in a single index.
  */
 static int
-vy_tx_set_entry(struct vy_tx *tx, struct vy_lsm *lsm,
-		struct vy_entry entry, uint64_t column_mask)
+vy_tx_set_entry(struct vy_tx *tx, struct vy_lsm *lsm, struct vy_entry entry)
 {
 	assert(vy_stmt_type(entry.stmt) != 0);
 	/**
@@ -1071,7 +1061,7 @@ vy_tx_set_entry(struct vy_tx *tx, struct vy_lsm *lsm,
 	}
 
 	/* Allocate a MVCC container. */
-	struct txv *v = txv_new(tx, lsm, entry, column_mask);
+	struct txv *v = txv_new(tx, lsm, entry);
 	if (applied.stmt != NULL)
 		tuple_unref(applied.stmt);
 	if (v == NULL)
@@ -1089,38 +1079,21 @@ vy_tx_set_entry(struct vy_tx *tx, struct vy_lsm *lsm,
 	if (old == NULL && vy_stmt_type(entry.stmt) == IPROTO_INSERT)
 		v->is_first_insert = true;
 
-	if (old != NULL) {
+	if (lsm->index_id > 0 && old != NULL && !old->is_nop) {
 		/*
-		 * Inherit the column mask of the overwritten statement
-		 * so as not to skip both statements on commit.
+		 * In a secondary index write set, DELETE statement purges
+		 * exactly one older statement so REPLACE + DELETE is no-op.
+		 * Moreover, DELETE + REPLACE can be treated as no-op, too,
+		 * because secondary indexes don't store full tuples hence
+		 * all REPLACE statements for the same key are equivalent.
+		 * Therefore we can zap DELETE + REPLACE as there must be
+		 * an older REPLACE for the same key stored somewhere in the
+		 * index data.
 		 */
-		v->column_mask |= old->column_mask;
-	}
-
-	if (lsm->index_id > 0 && vy_stmt_type(entry.stmt) == IPROTO_REPLACE &&
-	    old != NULL && vy_stmt_type(old->entry.stmt) == IPROTO_DELETE) {
-		/*
-		 * The column mask of an update operation may have a bit
-		 * set even if the corresponding field doesn't actually
-		 * get updated, because a column mask is generated
-		 * only from update operations, before applying them.
-		 * E.g. update(1, {{'+', 2, 0}}) doesn't modify the
-		 * second field, but the column mask will say it does.
-		 *
-		 * To discard DELETE statements in the write iterator
-		 * (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.
-		 *
-		 * So if the update didn't actually modify secondary
-		 * key fields, i.e. DELETE and REPLACE generated by the
-		 * update have the same key fields, we forcefully clear
-		 * key bits in the column mask to ensure that no REPLACE
-		 * statement will be written for this secondary key.
-		 */
-		if (v->column_mask != UINT64_MAX)
-			v->column_mask &= ~lsm->cmp_def->column_mask;
+		enum iproto_type type = vy_stmt_type(entry.stmt);
+		enum iproto_type old_type = vy_stmt_type(old->entry.stmt);
+		if ((type == IPROTO_DELETE) != (old_type == IPROTO_DELETE))
+			v->is_nop = true;
 	}
 
 	v->overwritten = old;
@@ -1133,12 +1106,11 @@ vy_tx_set_entry(struct vy_tx *tx, struct vy_lsm *lsm,
 }
 
 int
-vy_tx_set_with_colmask(struct vy_tx *tx, struct vy_lsm *lsm,
-		       struct tuple *stmt, uint64_t column_mask)
+vy_tx_set(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt)
 {
 	struct vy_entry entry;
 	vy_stmt_foreach_entry(entry, stmt, lsm->cmp_def) {
-		if (vy_tx_set_entry(tx, lsm, entry, column_mask) != 0)
+		if (vy_tx_set_entry(tx, lsm, entry) != 0)
 			return -1;
 	}
 	return 0;
diff --git a/src/box/vy_tx.h b/src/box/vy_tx.h
index 2712263b..9ee10755 100644
--- a/src/box/vy_tx.h
+++ b/src/box/vy_tx.h
@@ -87,8 +87,6 @@ struct txv {
 	struct vy_entry entry;
 	/** Statement allocated on vy_mem->allocator. */
 	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. */
@@ -101,6 +99,11 @@ struct txv {
 	 */
 	bool is_first_insert;
 	/**
+	 * True if the statement has no effect and can be safely
+	 * skipped on commit.
+	 */
+	bool is_nop;
+	/**
 	 * True if the txv was overwritten by another txv of
 	 * the same transaction.
 	 */
@@ -397,20 +400,12 @@ vy_tx_track_point(struct vy_tx *tx, struct vy_lsm *lsm, struct vy_entry entry);
  * @param tx           Transaction.
  * @param lsm          LSM tree the statement is for.
  * @param stmt         Statement.
- * @param column_mask  Mask of fields modified by the statement.
  *
  * @retval  0 Success
  * @retval -1 Memory allocation error.
  */
 int
-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);
-}
+vy_tx_set(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt);
 
 /**
  * Iterator over the write set of a transaction.
diff --git a/test/engine/update.result b/test/engine/update.result
index b73037eb..69293e46 100644
--- a/test/engine/update.result
+++ b/test/engine/update.result
@@ -758,3 +758,33 @@ s:upsert({2, 666}, {{'=', 2, 666}})
 s:drop()
 ---
 ...
+--
+-- gh-4242 Tuple is missing from secondary index after update.
+--
+s = box.schema.space.create('test', {engine = engine})
+---
+...
+pk = s:create_index('pk')
+---
+...
+sk = s:create_index('sk', {parts = {2, 'unsigned'}})
+---
+...
+s:insert{1, 1, 1}
+---
+- [1, 1, 1]
+...
+box.begin() s:update(1, {{'=', 2, 2}}) s:update(1, {{'=', 3, 2}}) box.commit()
+---
+...
+pk:select()
+---
+- - [1, 2, 2]
+...
+sk:select()
+---
+- - [1, 2, 2]
+...
+s:drop()
+---
+...
diff --git a/test/engine/update.test.lua b/test/engine/update.test.lua
index e2a2265f..51263f57 100644
--- a/test/engine/update.test.lua
+++ b/test/engine/update.test.lua
@@ -122,3 +122,15 @@ box.space.tst_sample:get(2).VAL
 -- invalid upsert
 s:upsert({2, 666}, {{'=', 2, 666}})
 s:drop()
+
+--
+-- gh-4242 Tuple is missing from secondary index after update.
+--
+s = box.schema.space.create('test', {engine = engine})
+pk = s:create_index('pk')
+sk = s:create_index('sk', {parts = {2, 'unsigned'}})
+s:insert{1, 1, 1}
+box.begin() s:update(1, {{'=', 2, 2}}) s:update(1, {{'=', 3, 2}}) box.commit()
+pk:select()
+sk:select()
+s:drop()
diff --git a/test/vinyl/write_iterator.result b/test/vinyl/write_iterator.result
index 39212572..5c5200b8 100644
--- a/test/vinyl/write_iterator.result
+++ b/test/vinyl/write_iterator.result
@@ -767,7 +767,7 @@ box.snapshot()
 - ok
 ...
 -- Some padding to trigger minor compaction.
-for i = 1001, 1000 + PAD2 do s:replace{i, i} end
+for i = 1001, 1000 + PAD2 do s:delete{i} end
 ---
 ...
 box.snapshot()
diff --git a/test/vinyl/write_iterator.test.lua b/test/vinyl/write_iterator.test.lua
index d7871ffc..fa44961a 100644
--- a/test/vinyl/write_iterator.test.lua
+++ b/test/vinyl/write_iterator.test.lua
@@ -328,7 +328,7 @@ PAD2 = 15
 for i = 1001, 1000 + PAD1 do s:replace{i, i} end
 box.snapshot()
 -- Some padding to trigger minor compaction.
-for i = 1001, 1000 + PAD2 do s:replace{i, i} end
+for i = 1001, 1000 + PAD2 do s:delete{i} end
 box.snapshot()
 -- Generate some INSERT statements and dump them to disk.
 _ = s:insert{1, 1} -- insert
-- 
2.11.0

[PATCH 2/3] vinyl: don't produce deferred DELETE on commit if key isn't updated

[Vladimir Davydov]

Consider the following example:

  s = box.schema.space.create('test', {engine = 'vinyl'})
  s:create_index('primary')
  s:create_index('secondary', {parts = {2, 'unsigned'}})

  s:insert{1, 1, 1}
  s:replace{1, 1, 2}

When REPLACE{1,1} is committed to the secondary index, the overwritten
tuple, i.e. INSERT{1,1}, is found in the primary index memory, and so
deferred DELETE{1,1} is generated right away and committed along with
REPLACE{1,1}. However, there's no need to commit anything to the
secondary index in this case, because its key isn't updated. Apart from
eating memory and loading disk, this also breaks index stats, as vy_tx
implementation doesn't expect two statements committed for the same key
in a single transaction.

Fix this by checking if there's a statement in the log for the deleted
key and if there's skipping them both as we do in the regular case, see
the comment in vy_tx_set.

Closes #3693
---
 src/box/vy_tx.c                     | 18 +++++++++
 test/vinyl/deferred_delete.result   | 78 +++++++++++++++++++++++++++++++++++++
 test/vinyl/deferred_delete.test.lua | 26 +++++++++++++
 test/vinyl/quota.result             |  2 +-
 test/vinyl/stat.result              |  6 +--
 test/vinyl/stat.test.lua            |  2 +-
 test/vinyl/write_iterator.result    |  2 +-
 test/vinyl/write_iterator.test.lua  |  2 +-
 8 files changed, 129 insertions(+), 7 deletions(-)

diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index aedd0610..119d975b 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -629,6 +629,23 @@ vy_tx_handle_deferred_delete(struct vy_tx *tx, struct txv *v)
 		struct vy_lsm *lsm = vy_lsm(space->index[i]);
 		struct vy_entry entry;
 		vy_stmt_foreach_entry(entry, delete_stmt, lsm->cmp_def) {
+			struct txv *other = write_set_search_key(&tx->write_set,
+								 lsm, entry);
+			if (other != NULL && !other->is_overwritten) {
+				/*
+				 * The write set contains a statement
+				 * for the key to be deleted. This can
+				 * only occur if it's a REPLACE that
+				 * happens not to update the secondary
+				 * index key parts. It's safe to skip it,
+				 * see vy_tx_set_entry().
+				 */
+				assert(vy_stmt_type(stmt) == IPROTO_REPLACE);
+				assert(vy_stmt_type(other->entry.stmt) ==
+								IPROTO_REPLACE);
+				other->is_nop = true;
+				continue;
+			}
 			struct txv *delete_txv = txv_new(tx, lsm, entry);
 			if (delete_txv == NULL) {
 				rc = -1;
@@ -717,6 +734,7 @@ vy_tx_prepare(struct vy_tx *tx)
 			 * to the primary index LSM tree. So we must
 			 * skip it for secondary indexes as well.
 			 */
+			v->is_overwritten = true;
 			continue;
 		}
 
diff --git a/test/vinyl/deferred_delete.result b/test/vinyl/deferred_delete.result
index 422f2fe0..3f187a5b 100644
--- a/test/vinyl/deferred_delete.result
+++ b/test/vinyl/deferred_delete.result
@@ -726,6 +726,84 @@ sk:select() -- ditto
 s:drop()
 ---
 ...
+--
+-- gh-3693 If a REPLACE doesn't update secondary index key parts,
+-- we must not generate a deferred DELETE for it on commit.
+-- Moreover, we must skip it as it has no effect.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+---
+...
+pk = s:create_index('pk')
+---
+...
+sk = s:create_index('sk', {parts = {2, 'unsigned'}})
+---
+...
+s:insert{1, 1, 1}
+---
+- [1, 1, 1]
+...
+s:insert{2, 2, 2}
+---
+- [2, 2, 2]
+...
+s:replace{2, 2, 3}
+---
+- [2, 2, 3]
+...
+pk:stat().rows -- 3: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3}
+---
+- 3
+...
+sk:stat().rows -- 2: INSERT{1,1} + INSERT{2,2}
+---
+- 2
+...
+box.begin()
+---
+...
+s:replace{1, 1, 2}
+---
+- [1, 1, 2]
+...
+s:delete{1}
+---
+...
+box.commit()
+---
+...
+pk:stat().rows -- 4: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3} + DELETE{1}
+---
+- 4
+...
+sk:stat().rows -- 3: INSERT{1,1} + INSERT{2,2} + DELETE{1,1}
+---
+- 3
+...
+box.snapshot()
+---
+- ok
+...
+pk:stat().rows -- 1: INSERT{2,2,3}
+---
+- 1
+...
+sk:stat().rows -- 1: INSERT{2,2}
+---
+- 1
+...
+pk:select()
+---
+- - [2, 2, 3]
+...
+sk:select()
+---
+- - [2, 2, 3]
+...
+s:drop()
+---
+...
 box.cfg{vinyl_cache = vinyl_cache}
 ---
 ...
diff --git a/test/vinyl/deferred_delete.test.lua b/test/vinyl/deferred_delete.test.lua
index 6dfcbde5..689c8f93 100644
--- a/test/vinyl/deferred_delete.test.lua
+++ b/test/vinyl/deferred_delete.test.lua
@@ -265,6 +265,32 @@ box.snapshot()
 sk:select() -- ditto
 s:drop()
 
+--
+-- gh-3693 If a REPLACE doesn't update secondary index key parts,
+-- we must not generate a deferred DELETE for it on commit.
+-- Moreover, we must skip it as it has no effect.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+pk = s:create_index('pk')
+sk = s:create_index('sk', {parts = {2, 'unsigned'}})
+s:insert{1, 1, 1}
+s:insert{2, 2, 2}
+s:replace{2, 2, 3}
+pk:stat().rows -- 3: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3}
+sk:stat().rows -- 2: INSERT{1,1} + INSERT{2,2}
+box.begin()
+s:replace{1, 1, 2}
+s:delete{1}
+box.commit()
+pk:stat().rows -- 4: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3} + DELETE{1}
+sk:stat().rows -- 3: INSERT{1,1} + INSERT{2,2} + DELETE{1,1}
+box.snapshot()
+pk:stat().rows -- 1: INSERT{2,2,3}
+sk:stat().rows -- 1: INSERT{2,2}
+pk:select()
+sk:select()
+s:drop()
+
 box.cfg{vinyl_cache = vinyl_cache}
 
 --
diff --git a/test/vinyl/quota.result b/test/vinyl/quota.result
index 1a0842f9..d1b28ee5 100644
--- a/test/vinyl/quota.result
+++ b/test/vinyl/quota.result
@@ -89,7 +89,7 @@ _ = space:replace{1, 1, string.rep('a', 1024 * 1024 * 5)}
 ...
 box.stat.vinyl().memory.level0
 ---
-- 5341267
+- 5292076
 ...
 space:drop()
 ---
diff --git a/test/vinyl/stat.result b/test/vinyl/stat.result
index aa47a7be..e2991d53 100644
--- a/test/vinyl/stat.result
+++ b/test/vinyl/stat.result
@@ -1158,7 +1158,7 @@ s = box.schema.space.create('test', {engine = 'vinyl'})
 i1 = s:create_index('i1', {parts = {1, 'unsigned'}})
 ---
 ...
-i2 = s:create_index('i2', {parts = {2, 'unsigned'}})
+i2 = s:create_index('i2', {parts = {2, 'unsigned'}, unique = false})
 ---
 ...
 for i = 1, 100 do s:replace{i, i, string.rep('x', 1000)} end
@@ -1191,7 +1191,7 @@ box.snapshot()
 stat_diff(gstat(), st, 'scheduler')
 ---
 - dump_input: 10420
-  dump_output: 10371
+  dump_output: 10411
   tasks_completed: 2
   dump_count: 1
 ...
@@ -1206,7 +1206,7 @@ while i1:stat().disk.compaction.count == 0 do fiber.sleep(0.01) end
 ...
 stat_diff(gstat(), st, 'scheduler')
 ---
-- compaction_input: 112188
+- compaction_input: 112228
   tasks_completed: 1
   compaction_output: 101984
 ...
diff --git a/test/vinyl/stat.test.lua b/test/vinyl/stat.test.lua
index 4a360f33..4c751aad 100644
--- a/test/vinyl/stat.test.lua
+++ b/test/vinyl/stat.test.lua
@@ -333,7 +333,7 @@ s:drop()
 -- sched stats
 s = box.schema.space.create('test', {engine = 'vinyl'})
 i1 = s:create_index('i1', {parts = {1, 'unsigned'}})
-i2 = s:create_index('i2', {parts = {2, 'unsigned'}})
+i2 = s:create_index('i2', {parts = {2, 'unsigned'}, unique = false})
 
 for i = 1, 100 do s:replace{i, i, string.rep('x', 1000)} end
 st = gstat()
diff --git a/test/vinyl/write_iterator.result b/test/vinyl/write_iterator.result
index 5c5200b8..f0244113 100644
--- a/test/vinyl/write_iterator.result
+++ b/test/vinyl/write_iterator.result
@@ -931,7 +931,7 @@ box.snapshot()
 - ok
 ...
 -- Some padding to trigger minor compaction.
-for i = 1001, 1000 + PAD2 do s:replace{i, i} end
+for i = 1001, 1000 + PAD2 do s:delete{i} end
 ---
 ...
 box.snapshot()
diff --git a/test/vinyl/write_iterator.test.lua b/test/vinyl/write_iterator.test.lua
index fa44961a..0de21f21 100644
--- a/test/vinyl/write_iterator.test.lua
+++ b/test/vinyl/write_iterator.test.lua
@@ -389,7 +389,7 @@ _ = s:insert{7, 7}
 _ = s:insert{8, 8}
 box.snapshot()
 -- Some padding to trigger minor compaction.
-for i = 1001, 1000 + PAD2 do s:replace{i, i} end
+for i = 1001, 1000 + PAD2 do s:delete{i} end
 box.snapshot()
 -- Generate DELETE+INSERT statements and write them to disk.
 s:delete{1} s:insert{1, 100}
-- 
2.11.0

[PATCH 3/3] vinyl: fix deferred DELETE statement lost on commit

[Vladimir Davydov]

Even if a statement isn't marked as VY_STMT_DEFERRED_DELETE, e.g. it's
a REPLACE produced by an UPDATE request, it may overwrite a statement in
the transaction write set that is marked so, for instance:

  s = box.schema.space.create('test', {engine = 'vinyl'})
  pk = s:create_index('pk')
  sk = s:create_index('sk', {parts = {2, 'unsigned'}})

  s:insert{1, 1}

  box.begin()
  s:replace{1, 2}
  s:update(1, {{'=', 2, 3}})
  box.commit()

If we don't mark REPLACE{3,1} produced by the update operatoin with
VY_STMT_DEFERRED_DELETE flag, we will never generate a DELETE statement
for INSERT{1,1}. That is, we must inherit the flag from the overwritten
statement when we insert a new one into a write set.

Closes #4248
---
 src/box/vy_tx.c                     | 10 ++++++
 test/vinyl/deferred_delete.result   | 67 +++++++++++++++++++++++++++++++++++++
 test/vinyl/deferred_delete.test.lua | 21 ++++++++++++
 3 files changed, 98 insertions(+)

diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index 119d975b..76d184fb 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -1092,6 +1092,16 @@ vy_tx_set_entry(struct vy_tx *tx, struct vy_lsm *lsm, struct vy_entry entry)
 		write_set_remove(&tx->write_set, old);
 		old->is_overwritten = true;
 		v->is_first_insert = old->is_first_insert;
+		/*
+		 * Inherit VY_STMT_DEFERRED_DELETE flag from the older
+		 * statement so as to generate a DELETE for the tuple
+		 * overwritten by this transaction.
+		 */
+		if (vy_stmt_flags(old->entry.stmt) & VY_STMT_DEFERRED_DELETE) {
+			uint8_t flags = vy_stmt_flags(entry.stmt);
+			vy_stmt_set_flags(entry.stmt, flags |
+					  VY_STMT_DEFERRED_DELETE);
+		}
 	}
 
 	if (old == NULL && vy_stmt_type(entry.stmt) == IPROTO_INSERT)
diff --git a/test/vinyl/deferred_delete.result b/test/vinyl/deferred_delete.result
index 3f187a5b..23c93f0f 100644
--- a/test/vinyl/deferred_delete.result
+++ b/test/vinyl/deferred_delete.result
@@ -804,6 +804,73 @@ sk:select()
 s:drop()
 ---
 ...
+--
+-- gh-4248 Deferred DELETE isn't produced on transaction commit.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+---
+...
+pk = s:create_index('pk')
+---
+...
+sk = s:create_index('sk', {parts = {2, 'unsigned'}})
+---
+...
+s:insert{1, 10}
+---
+- [1, 10]
+...
+s:insert{2, 20}
+---
+- [2, 20]
+...
+box.begin()
+---
+...
+s:replace{1, 11}
+---
+- [1, 11]
+...
+s:update(1, {{'=', 2, 12}})
+---
+- [1, 12]
+...
+s:update(2, {{'=', 2, 21}})
+---
+- [2, 21]
+...
+s:replace{2, 22}
+---
+- [2, 22]
+...
+box.commit()
+---
+...
+box.snapshot()
+---
+- ok
+...
+pk:stat().rows -- 2: REPLACE{1, 12} + REPLACE{2, 22}
+---
+- 2
+...
+sk:stat().rows -- ditto
+---
+- 2
+...
+pk:select()
+---
+- - [1, 12]
+  - [2, 22]
+...
+sk:select()
+---
+- - [1, 12]
+  - [2, 22]
+...
+s:drop()
+---
+...
 box.cfg{vinyl_cache = vinyl_cache}
 ---
 ...
diff --git a/test/vinyl/deferred_delete.test.lua b/test/vinyl/deferred_delete.test.lua
index 689c8f93..1bce954c 100644
--- a/test/vinyl/deferred_delete.test.lua
+++ b/test/vinyl/deferred_delete.test.lua
@@ -291,6 +291,27 @@ pk:select()
 sk:select()
 s:drop()
 
+--
+-- gh-4248 Deferred DELETE isn't produced on transaction commit.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+pk = s:create_index('pk')
+sk = s:create_index('sk', {parts = {2, 'unsigned'}})
+s:insert{1, 10}
+s:insert{2, 20}
+box.begin()
+s:replace{1, 11}
+s:update(1, {{'=', 2, 12}})
+s:update(2, {{'=', 2, 21}})
+s:replace{2, 22}
+box.commit()
+box.snapshot()
+pk:stat().rows -- 2: REPLACE{1, 12} + REPLACE{2, 22}
+sk:stat().rows -- ditto
+pk:select()
+sk:select()
+s:drop()
+
 box.cfg{vinyl_cache = vinyl_cache}
 
 --
-- 
2.11.0

[tarantool-patches] Re: [PATCH 1/3] vinyl: fix secondary index divergence on update

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/05/25 06:41]:

Vladimir, 

could you clarify your comments a bit?

> If an UPDATE request doesn't touch key parts of a secondary index, we
> don't need to write it to the index memory level or dump it to disk, as

We don't have a separate memtable for secondary keys. Better say
"we don't need to re-index it in the in-memory secondary index".

> this would only increase IO load. Historically, we use column mask set
> by the UPDATE operation to skip secondary indexes that are not affected
> by the operation on commit. However, there's a problem here: the column
> mask isn't precise - it may have a bit set even if the corresponding
> column doesn't really get updated, e.g. consider {'+', 2, 0}.

The column does get updated, but the update doesn't change its
value. Now I am making the ends of it.


> Not taking
> this into account may result in appearance of phantom tuples on disk as
> the write iterator assumes that statements that have no effect aren't
> written to secondary indexes (this is needed to apply INSERT+DELETE
> "annihilation" optimization). We fixed that by clearing column mask bits
> in vy_tx_set in case we detect that the key isn't changed, for more
> details see #3607 and commit e72867cb9169 ("vinyl: fix appearance of
> phantom tuple in secondary index after update"). It was rather an ugly
> hack, but it worked.
> 
> However, it turned out that apart from looking hackish this code has
> a nasty bug that may lead to tuples missing from secondary indexes.
> Consider the following example:
> 
>   s = box.schema.space.create('test', {engine = 'vinyl'})
>   s:create_index('pk')
>   s:create_index('sk', {parts = {2, 'unsigned'}})
>   s:insert{1, 1, 1}
> 
>   box.begin()
>   s:update(1, {{'=', 2, 2}})
>   s:update(1, {{'=', 3, 2}})
>   box.commit()
> 
> The first update operation writes DELETE{1,1} and REPLACE{2,1} to the
> secondary index write set. The second update replaces REPLACE{2,1} with
> DELETE{2,1} and then with REPLACE{2,1}. When replacing DELETE{2,1} with
> REPLACE{2,1} in the write set, we assume that the update doesn't modify
> secondary index key parts and clear the column mask so as not to commit
> a pointless request, see vy_tx_set. As a result, we skip the first
> update too and get key {2,1} missing from the secondary index.
> 
> Actually, it was a dumb idea to use column mask to skip statements in
> the first place, as there's a much easier way to filter out statements
> that have no effect for secondary indexes. The thing is every DELETE
> statement inserted into a secondary index write set acts as a "single
> DELETE", i.e. there's exactly one older statement it is supposed to
> purge. This is, because in contrast to the primary index we don't write
> DELETE statements blindly - we always look up the tuple overwritten in
> the primary index first. This means that REPLACE+DELETE for the same key
> is basically a no-op and can be safely skip. Moreover, DELETE+REPLACE
> can be treated as no-op, too, because secondary indexes don't store full
> tuples hence all REPLACE statements for the same key are equivalent.
> By marking such pair of statements as no-op in vy_tx_set, we guarantee
> that no-op statements don't make it to secondary index memory or disk
> levels.

Better say "mark both statements", not a pair, since they are not
present in the tx write list as a pair.

Could you also please explain why you decided to introduce a new
flag, and not use is_overwritten?


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 2/3] vinyl: don't produce deferred DELETE on commit if key isn't updated

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/05/25 06:41]:

this one is basically lgtm

> Consider the following example:
> 
>   s = box.schema.space.create('test', {engine = 'vinyl'})
>   s:create_index('primary')
>   s:create_index('secondary', {parts = {2, 'unsigned'}})
> 
>   s:insert{1, 1, 1}
>   s:replace{1, 1, 2}
> 
> When REPLACE{1,1} is committed to the secondary index, the overwritten
> tuple, i.e. INSERT{1,1}, is found in the primary index memory, and so
> deferred DELETE{1,1} is generated right away and committed along with
> REPLACE{1,1}. However, there's no need to commit anything to the
> secondary index in this case, because its key isn't updated. Apart from
> eating memory and loading disk, this also breaks index stats, as vy_tx
> implementation doesn't expect two statements committed for the same key
> in a single transaction.
> 
> Fix this by checking if there's a statement in the log for the deleted
> key and if there's skipping them both as we do in the regular case, see
> the comment in vy_tx_set.
> 
> Closes #3693
> ---
>  src/box/vy_tx.c                     | 18 +++++++++
>  test/vinyl/deferred_delete.result   | 78 +++++++++++++++++++++++++++++++++++++
>  test/vinyl/deferred_delete.test.lua | 26 +++++++++++++
>  test/vinyl/quota.result             |  2 +-
>  test/vinyl/stat.result              |  6 +--
>  test/vinyl/stat.test.lua            |  2 +-
>  test/vinyl/write_iterator.result    |  2 +-
>  test/vinyl/write_iterator.test.lua  |  2 +-
>  8 files changed, 129 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
> index aedd0610..119d975b 100644
> --- a/src/box/vy_tx.c
> +++ b/src/box/vy_tx.c
> @@ -629,6 +629,23 @@ vy_tx_handle_deferred_delete(struct vy_tx *tx, struct txv *v)
>  		struct vy_lsm *lsm = vy_lsm(space->index[i]);
>  		struct vy_entry entry;
>  		vy_stmt_foreach_entry(entry, delete_stmt, lsm->cmp_def) {
> +			struct txv *other = write_set_search_key(&tx->write_set,
> +								 lsm, entry);
> +			if (other != NULL && !other->is_overwritten) {
> +				/*
> +				 * The write set contains a statement
> +				 * for the key to be deleted. This can
> +				 * only occur if it's a REPLACE that
> +				 * happens not to update the secondary
> +				 * index key parts. It's safe to skip it,
> +				 * see vy_tx_set_entry().
> +				 */
> +				assert(vy_stmt_type(stmt) == IPROTO_REPLACE);
> +				assert(vy_stmt_type(other->entry.stmt) ==
> +								IPROTO_REPLACE);
> +				other->is_nop = true;
> +				continue;
> +			}
>  			struct txv *delete_txv = txv_new(tx, lsm, entry);
>  			if (delete_txv == NULL) {
>  				rc = -1;
> @@ -717,6 +734,7 @@ vy_tx_prepare(struct vy_tx *tx)
>  			 * to the primary index LSM tree. So we must
>  			 * skip it for secondary indexes as well.
>  			 */
> +			v->is_overwritten = true;
>  			continue;
>  		}
>  
> diff --git a/test/vinyl/deferred_delete.result b/test/vinyl/deferred_delete.result
> index 422f2fe0..3f187a5b 100644
> --- a/test/vinyl/deferred_delete.result
> +++ b/test/vinyl/deferred_delete.result
> @@ -726,6 +726,84 @@ sk:select() -- ditto
>  s:drop()
>  ---
>  ...
> +--
> +-- gh-3693 If a REPLACE doesn't update secondary index key parts,
> +-- we must not generate a deferred DELETE for it on commit.
> +-- Moreover, we must skip it as it has no effect.
> +--
> +s = box.schema.space.create('test', {engine = 'vinyl'})
> +---
> +...
> +pk = s:create_index('pk')
> +---
> +...
> +sk = s:create_index('sk', {parts = {2, 'unsigned'}})
> +---
> +...
> +s:insert{1, 1, 1}
> +---
> +- [1, 1, 1]
> +...
> +s:insert{2, 2, 2}
> +---
> +- [2, 2, 2]
> +...
> +s:replace{2, 2, 3}
> +---
> +- [2, 2, 3]
> +...
> +pk:stat().rows -- 3: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3}
> +---
> +- 3
> +...
> +sk:stat().rows -- 2: INSERT{1,1} + INSERT{2,2}
> +---
> +- 2
> +...
> +box.begin()
> +---
> +...
> +s:replace{1, 1, 2}
> +---
> +- [1, 1, 2]
> +...
> +s:delete{1}
> +---
> +...
> +box.commit()
> +---
> +...
> +pk:stat().rows -- 4: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3} + DELETE{1}
> +---
> +- 4
> +...
> +sk:stat().rows -- 3: INSERT{1,1} + INSERT{2,2} + DELETE{1,1}
> +---
> +- 3
> +...
> +box.snapshot()
> +---
> +- ok
> +...
> +pk:stat().rows -- 1: INSERT{2,2,3}
> +---
> +- 1
> +...
> +sk:stat().rows -- 1: INSERT{2,2}
> +---
> +- 1
> +...
> +pk:select()
> +---
> +- - [2, 2, 3]
> +...
> +sk:select()
> +---
> +- - [2, 2, 3]
> +...
> +s:drop()
> +---
> +...
>  box.cfg{vinyl_cache = vinyl_cache}
>  ---
>  ...
> diff --git a/test/vinyl/deferred_delete.test.lua b/test/vinyl/deferred_delete.test.lua
> index 6dfcbde5..689c8f93 100644
> --- a/test/vinyl/deferred_delete.test.lua
> +++ b/test/vinyl/deferred_delete.test.lua
> @@ -265,6 +265,32 @@ box.snapshot()
>  sk:select() -- ditto
>  s:drop()
>  
> +--
> +-- gh-3693 If a REPLACE doesn't update secondary index key parts,
> +-- we must not generate a deferred DELETE for it on commit.
> +-- Moreover, we must skip it as it has no effect.
> +--
> +s = box.schema.space.create('test', {engine = 'vinyl'})
> +pk = s:create_index('pk')
> +sk = s:create_index('sk', {parts = {2, 'unsigned'}})
> +s:insert{1, 1, 1}
> +s:insert{2, 2, 2}
> +s:replace{2, 2, 3}
> +pk:stat().rows -- 3: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3}
> +sk:stat().rows -- 2: INSERT{1,1} + INSERT{2,2}
> +box.begin()
> +s:replace{1, 1, 2}
> +s:delete{1}
> +box.commit()
> +pk:stat().rows -- 4: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3} + DELETE{1}
> +sk:stat().rows -- 3: INSERT{1,1} + INSERT{2,2} + DELETE{1,1}
> +box.snapshot()
> +pk:stat().rows -- 1: INSERT{2,2,3}
> +sk:stat().rows -- 1: INSERT{2,2}
> +pk:select()
> +sk:select()
> +s:drop()
> +
>  box.cfg{vinyl_cache = vinyl_cache}
>  
>  --
> diff --git a/test/vinyl/quota.result b/test/vinyl/quota.result
> index 1a0842f9..d1b28ee5 100644
> --- a/test/vinyl/quota.result
> +++ b/test/vinyl/quota.result
> @@ -89,7 +89,7 @@ _ = space:replace{1, 1, string.rep('a', 1024 * 1024 * 5)}
>  ...
>  box.stat.vinyl().memory.level0
>  ---
> -- 5341267
> +- 5292076
>  ...
>  space:drop()
>  ---
> diff --git a/test/vinyl/stat.result b/test/vinyl/stat.result
> index aa47a7be..e2991d53 100644
> --- a/test/vinyl/stat.result
> +++ b/test/vinyl/stat.result
> @@ -1158,7 +1158,7 @@ s = box.schema.space.create('test', {engine = 'vinyl'})
>  i1 = s:create_index('i1', {parts = {1, 'unsigned'}})
>  ---
>  ...
> -i2 = s:create_index('i2', {parts = {2, 'unsigned'}})
> +i2 = s:create_index('i2', {parts = {2, 'unsigned'}, unique = false})
>  ---
>  ...
>  for i = 1, 100 do s:replace{i, i, string.rep('x', 1000)} end
> @@ -1191,7 +1191,7 @@ box.snapshot()
>  stat_diff(gstat(), st, 'scheduler')
>  ---
>  - dump_input: 10420
> -  dump_output: 10371
> +  dump_output: 10411
>    tasks_completed: 2
>    dump_count: 1
>  ...
> @@ -1206,7 +1206,7 @@ while i1:stat().disk.compaction.count == 0 do fiber.sleep(0.01) end
>  ...
>  stat_diff(gstat(), st, 'scheduler')
>  ---
> -- compaction_input: 112188
> +- compaction_input: 112228
>    tasks_completed: 1
>    compaction_output: 101984
>  ...
> diff --git a/test/vinyl/stat.test.lua b/test/vinyl/stat.test.lua
> index 4a360f33..4c751aad 100644
> --- a/test/vinyl/stat.test.lua
> +++ b/test/vinyl/stat.test.lua
> @@ -333,7 +333,7 @@ s:drop()
>  -- sched stats
>  s = box.schema.space.create('test', {engine = 'vinyl'})
>  i1 = s:create_index('i1', {parts = {1, 'unsigned'}})
> -i2 = s:create_index('i2', {parts = {2, 'unsigned'}})
> +i2 = s:create_index('i2', {parts = {2, 'unsigned'}, unique = false})
>  
>  for i = 1, 100 do s:replace{i, i, string.rep('x', 1000)} end
>  st = gstat()
> diff --git a/test/vinyl/write_iterator.result b/test/vinyl/write_iterator.result
> index 5c5200b8..f0244113 100644
> --- a/test/vinyl/write_iterator.result
> +++ b/test/vinyl/write_iterator.result
> @@ -931,7 +931,7 @@ box.snapshot()
>  - ok
>  ...
>  -- Some padding to trigger minor compaction.
> -for i = 1001, 1000 + PAD2 do s:replace{i, i} end
> +for i = 1001, 1000 + PAD2 do s:delete{i} end
>  ---
>  ...
>  box.snapshot()
> diff --git a/test/vinyl/write_iterator.test.lua b/test/vinyl/write_iterator.test.lua
> index fa44961a..0de21f21 100644
> --- a/test/vinyl/write_iterator.test.lua
> +++ b/test/vinyl/write_iterator.test.lua
> @@ -389,7 +389,7 @@ _ = s:insert{7, 7}
>  _ = s:insert{8, 8}
>  box.snapshot()
>  -- Some padding to trigger minor compaction.
> -for i = 1001, 1000 + PAD2 do s:replace{i, i} end
> +for i = 1001, 1000 + PAD2 do s:delete{i} end
>  box.snapshot()
>  -- Generate DELETE+INSERT statements and write them to disk.
>  s:delete{1} s:insert{1, 100}
> -- 
> 2.11.0
> 

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 3/3] vinyl: fix deferred DELETE statement lost on commit

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/05/25 06:41]:
> Even if a statement isn't marked as VY_STMT_DEFERRED_DELETE, e.g. it's
> a REPLACE produced by an UPDATE request, it may overwrite a statement in
> the transaction write set that is marked so, for instance:
> 
>   s = box.schema.space.create('test', {engine = 'vinyl'})
>   pk = s:create_index('pk')
>   sk = s:create_index('sk', {parts = {2, 'unsigned'}})
> 
>   s:insert{1, 1}
> 
>   box.begin()
>   s:replace{1, 2}
>   s:update(1, {{'=', 2, 3}})
>   box.commit()
> 
> If we don't mark REPLACE{3,1} produced by the update operatoin with
> VY_STMT_DEFERRED_DELETE flag, we will never generate a DELETE statement
> for INSERT{1,1}. That is, we must inherit the flag from the overwritten
> statement when we insert a new one into a write set.
> 
> Closes #4248

this one is also lgtm.

apparently when looking at deferred delete we completely missed
the multi-statement-transactions complications. 

Maybe produce a randomized test which would create long/diverse
trnasactions with deferred delete and check correctness of the
outcome?


-- 
Konstantin Osipov, Moscow, Russia

Re: [tarantool-patches] Re: [PATCH 1/3] vinyl: fix secondary index divergence on update

[Vladimir Davydov]

On Sat, May 25, 2019 at 09:11:57AM +0300, Konstantin Osipov wrote:
> Could you also please explain why you decided to introduce a new
> flag, and not use is_overwritten?

is_overwritten is cleared on rollback while is_nop stays unchanged.

Re: [tarantool-patches] Re: [PATCH 1/3] vinyl: fix secondary index divergence on update

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/05/25 22:55]:
> On Sat, May 25, 2019 at 09:11:57AM +0300, Konstantin Osipov wrote:
> > Could you also please explain why you decided to introduce a new
> > flag, and not use is_overwritten?
> 
> is_overwritten is cleared on rollback while is_nop stays unchanged.

Then shouldn't better name be is_overwriting, as every
is_overwritten txv is overwritten by some is_overwriting txv
(is_nop)? Is there any symmetry between the two or there are cases
when they are unrelated?

-- 
Konstantin Osipov, Moscow, Russia

Re: [tarantool-patches] Re: [PATCH 1/3] vinyl: fix secondary index divergence on update

[Vladimir Davydov]

On Sat, May 25, 2019 at 11:28:51PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/05/25 22:55]:
> > On Sat, May 25, 2019 at 09:11:57AM +0300, Konstantin Osipov wrote:
> > > Could you also please explain why you decided to introduce a new
> > > flag, and not use is_overwritten?
> > 
> > is_overwritten is cleared on rollback while is_nop stays unchanged.
> 
> Then shouldn't better name be is_overwriting, as every
> is_overwritten txv is overwritten by some is_overwriting txv
> (is_nop)? Is there any symmetry between the two or there are cases
> when they are unrelated?

No.

is_nop is set if a statement has no effect and can be skipped on commit,
e.g. DELETE following REPLACE in case of a secondary index.

Not every statement that overwrites some other statement has no effect,
e.g. REPLACE following other REPLACE can't be skipped.

Re: [tarantool-patches] Re: [PATCH 1/3] vinyl: fix secondary index divergence on update

[Vladimir Davydov]

On Sun, May 26, 2019 at 05:52:05PM +0300, Konstantin Osipov wrote:
> Thanks. Lgtm then. Please use my questions as input for improving the
> comments.

Done. Pushed to master. Backporting to 2.1 and 1.10.

Re: [tarantool-patches] Re: [PATCH 3/3] vinyl: fix deferred DELETE statement lost on commit

[Vladimir Davydov]

On Sat, May 25, 2019 at 09:15:36AM +0300, Konstantin Osipov wrote:
> apparently when looking at deferred delete we completely missed
> the multi-statement-transactions complications. 
> 
> Maybe produce a randomized test which would create long/diverse
> trnasactions with deferred delete and check correctness of the
> outcome?

Opened a ticket: https://github.com/tarantool/tarantool/issues/4251