[PATCH] vinyl: fix assertion failure in vy_tx_handle_deferred_delete

[PATCH] vinyl: fix assertion failure in vy_tx_handle_deferred_delete

[Vladimir Davydov]

vy_tx_handle_deferred_delete() expects (righteously) that in case a
deferred DELETE overwrites a statement in a secondary index write set
and the overwritten statement wasn't skipped on commit (i.e. doesn't
have txv->is_overwritten flag set), both the old and the new statement
must be REPLACEs (see the comment to the corresponding assertions for
more details).

The problem is we don't set is_overwritten flag in case a statement
doesn't have any effect (txv->is_nop is set), even if it was, in fact,
overwritten in the primary index write set (see vy_tx_prepare). As
a result, we get an assertion failure when we delete such statement
in the same transaction, e.g.

  s = box.schema.space.create('test', {engine = 'vinyl'})
  s:create_index('pk', {parts = {1, 'unsigned'}})
  s:create_index('sk', {parts = {2, 'unsigned'}})
  s:replace{1, 1, 1}
  box.begin()
  s:update(1, {{'+', 3, 1}}) -- adds no-op REPLACE to sk write set
  s:delete(1)                -- produces deferred DELETE for sk
  box.commit()

results in

  vy_tx_handle_deferred_delete: Assertion `vy_stmt_type(stmt) == IPROTO_REPLACE' failed.

Fix this by making sure we set is_overwritten for all overwritten
statements in a secondary index write set.

Closes #4294
---
https://github.com/tarantool/tarantool/issues/4294
https://github.com/tarantool/tarantool/commits/dv/gh-4294-vy-assert-failure-fix

 src/box/vy_tx.c        |  9 ++++-----
 test/vinyl/gh.result   | 37 +++++++++++++++++++++++++++++++++++++
 test/vinyl/gh.test.lua | 15 +++++++++++++++
 3 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index 76d184fb..56b9f292 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -717,10 +717,6 @@ 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 || v->is_nop)
-			continue;
-
 		if (lsm->index_id > 0 && repsert == NULL && delete == NULL) {
 			/*
 			 * This statement is for a secondary index,
@@ -735,9 +731,12 @@ vy_tx_prepare(struct vy_tx *tx)
 			 * skip it for secondary indexes as well.
 			 */
 			v->is_overwritten = true;
-			continue;
 		}
 
+		/* Do not save statements that was overwritten by the same tx */
+		if (v->is_overwritten || v->is_nop)
+			continue;
+
 		enum iproto_type type = vy_stmt_type(v->entry.stmt);
 
 		/* Optimize out INSERT + DELETE for the same key. */
diff --git a/test/vinyl/gh.result b/test/vinyl/gh.result
index 76beab09..78cb2a28 100644
--- a/test/vinyl/gh.result
+++ b/test/vinyl/gh.result
@@ -718,3 +718,40 @@ while finished ~= 2 do fiber.sleep(0.01) end
 s:drop()
 ---
 ...
+--
+-- gh-4294: assertion failure when deleting a no-op statement from
+-- a secondary index write set.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+---
+...
+_ = s:create_index('pk')
+---
+...
+_ = s:create_index('sk', {parts = {2, 'unsigned'}})
+---
+...
+s:replace{1, 1, 1}
+---
+- [1, 1, 1]
+...
+box.begin()
+---
+...
+s:update(1, {{'+', 3, 1}})
+---
+- [1, 1, 2]
+...
+s:delete(1)
+---
+...
+box.commit()
+---
+...
+s:select()
+---
+- []
+...
+s:drop()
+---
+...
diff --git a/test/vinyl/gh.test.lua b/test/vinyl/gh.test.lua
index 2044f6cc..9ad85798 100644
--- a/test/vinyl/gh.test.lua
+++ b/test/vinyl/gh.test.lua
@@ -312,3 +312,18 @@ cont = false
 while finished ~= 2 do fiber.sleep(0.01) end
 
 s:drop()
+
+--
+-- gh-4294: assertion failure when deleting a no-op statement from
+-- a secondary index write set.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('pk')
+_ = s:create_index('sk', {parts = {2, 'unsigned'}})
+s:replace{1, 1, 1}
+box.begin()
+s:update(1, {{'+', 3, 1}})
+s:delete(1)
+box.commit()
+s:select()
+s:drop()
-- 
2.11.0

[tarantool-patches] Re: [PATCH] vinyl: fix assertion failure in vy_tx_handle_deferred_delete

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/06/17 19:08]:
> vy_tx_handle_deferred_delete() expects (righteously) that in case a
> deferred DELETE overwrites a statement in a secondary index write set
> and the overwritten statement wasn't skipped on commit (i.e. doesn't
> have txv->is_overwritten flag set), both the old and the new statement
> must be REPLACEs (see the comment to the corresponding assertions for
> more details).
> 
> The problem is we don't set is_overwritten flag in case a statement
> doesn't have any effect (txv->is_nop is set), even if it was, in fact,
> overwritten in the primary index write set (see vy_tx_prepare). As
> a result, we get an assertion failure when we delete such statement
> in the same transaction, e.g.

lgtm


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH] vinyl: fix assertion failure in vy_tx_handle_deferred_delete

[Kirill Yukhin]

Hello,

On 17 Jun 18:30, Vladimir Davydov wrote:
> vy_tx_handle_deferred_delete() expects (righteously) that in case a
> deferred DELETE overwrites a statement in a secondary index write set
> and the overwritten statement wasn't skipped on commit (i.e. doesn't
> have txv->is_overwritten flag set), both the old and the new statement
> must be REPLACEs (see the comment to the corresponding assertions for
> more details).
> 
> The problem is we don't set is_overwritten flag in case a statement
> doesn't have any effect (txv->is_nop is set), even if it was, in fact,
> overwritten in the primary index write set (see vy_tx_prepare). As
> a result, we get an assertion failure when we delete such statement
> in the same transaction, e.g.
> 
>   s = box.schema.space.create('test', {engine = 'vinyl'})
>   s:create_index('pk', {parts = {1, 'unsigned'}})
>   s:create_index('sk', {parts = {2, 'unsigned'}})
>   s:replace{1, 1, 1}
>   box.begin()
>   s:update(1, {{'+', 3, 1}}) -- adds no-op REPLACE to sk write set
>   s:delete(1)                -- produces deferred DELETE for sk
>   box.commit()
> 
> results in
> 
>   vy_tx_handle_deferred_delete: Assertion `vy_stmt_type(stmt) == IPROTO_REPLACE' failed.
> 
> Fix this by making sure we set is_overwritten for all overwritten
> statements in a secondary index write set.
> 
> Closes #4294
> ---
> https://github.com/tarantool/tarantool/issues/4294
> https://github.com/tarantool/tarantool/commits/dv/gh-4294-vy-assert-failure-fix

I've checked yout patch into 1.10, 2.1 and master branches.

--
Regards, Kirill Yukhin