[Tarantool-patches] [PATCH] vinyl: fix assert in vy_tx_write()

[Tarantool-patches] [PATCH] vinyl: fix assert in vy_tx_write()

[Nikita Pettik]

Assert in vy_tx_write() validates that upsert applied on the top of
cached statement is always replace. In fact, it is not always so. If
vy_apply_upsert() fails due to the fact that PK is modified, it returns
old statement (i.e. statement which upsert is applied on). In this
regard, if tuple cache contains insert and invalid upsert is executed,
vy_apply_upsert() will return insert. As a result, assert will fire.
Let's fix it and take into account that vy_apply_upsert() is able to
return inserts as well.

Closes #5005
---
Branch: https://github.com/tarantool/tarantool/tree/np/gh-5005-upsert-changing-pk-crash
Issue: https://github.com/tarantool/tarantool/issues/5005

@ChangeLog:
 * Fix crash in debug build due to invalid upsert applied on
cached INSERT statement.

 src/box/vy_tx.c                               |  7 +++--
 test/vinyl/gh-5005-upsert-affecting-pk.result | 30 +++++++++++++++++++
 .../gh-5005-upsert-affecting-pk.test.lua      | 12 ++++++++
 3 files changed, 47 insertions(+), 2 deletions(-)
 create mode 100644 test/vinyl/gh-5005-upsert-affecting-pk.result
 create mode 100644 test/vinyl/gh-5005-upsert-affecting-pk.test.lua

diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index d092e0cdb..846c63294 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -506,8 +506,11 @@ vy_tx_write(struct vy_lsm *lsm, struct vy_mem *mem,
 							mem->cmp_def, false);
 			tuple_unref(deleted.stmt);
 			if (applied.stmt != NULL) {
-				assert(vy_stmt_type(applied.stmt) ==
-							IPROTO_REPLACE);
+				enum iproto_type applied_type =
+					vy_stmt_type(applied.stmt);
+				assert(applied_type == IPROTO_REPLACE ||
+				       applied_type == IPROTO_INSERT);
+				(void) applied_type;
 				int rc = vy_lsm_set(lsm, mem, applied,
 						    region_stmt);
 				tuple_unref(applied.stmt);
diff --git a/test/vinyl/gh-5005-upsert-affecting-pk.result b/test/vinyl/gh-5005-upsert-affecting-pk.result
new file mode 100644
index 000000000..e2fea3501
--- /dev/null
+++ b/test/vinyl/gh-5005-upsert-affecting-pk.result
@@ -0,0 +1,30 @@
+-- test-run result file version 2
+-- Make sure that applying invalid upsert (which modifies PK)
+-- after read (so that key populates tuple cache) does not result
+-- in crash.
+--
+s = box.schema.create_space('test', {engine = 'vinyl'})
+ | ---
+ | ...
+pk = s:create_index('pk')
+ | ---
+ | ...
+s:insert{1, 1}
+ | ---
+ | - [1, 1]
+ | ...
+s:select()
+ | ---
+ | - - [1, 1]
+ | ...
+s:upsert({1}, {{'+', 1, 1}})
+ | ---
+ | ...
+s:select()
+ | ---
+ | - - [1, 1]
+ | ...
+
+s:drop()
+ | ---
+ | ...
diff --git a/test/vinyl/gh-5005-upsert-affecting-pk.test.lua b/test/vinyl/gh-5005-upsert-affecting-pk.test.lua
new file mode 100644
index 000000000..23dc46ecd
--- /dev/null
+++ b/test/vinyl/gh-5005-upsert-affecting-pk.test.lua
@@ -0,0 +1,12 @@
+-- Make sure that applying invalid upsert (which modifies PK)
+-- after read (so that key populates tuple cache) does not result
+-- in crash.
+--
+s = box.schema.create_space('test', {engine = 'vinyl'})
+pk = s:create_index('pk')
+s:insert{1, 1}
+s:select()
+s:upsert({1}, {{'+', 1, 1}})
+s:select()
+
+s:drop()
-- 
2.17.1

Re: [Tarantool-patches] [PATCH] vinyl: fix assert in vy_tx_write()

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 21/05/2020 03:15, Nikita Pettik wrote:
> Assert in vy_tx_write() validates that upsert applied on the top of
> cached statement is always replace. In fact, it is not always so. If
> vy_apply_upsert() fails due to the fact that PK is modified, it returns
> old statement (i.e. statement which upsert is applied on). In this
> regard, if tuple cache contains insert and invalid upsert is executed,
> vy_apply_upsert() will return insert. As a result, assert will fire.
> Let's fix it and take into account that vy_apply_upsert() is able to
> return inserts as well.

I looked at other places in vinyl, which apply an upsert, and found
vy_lsm_commit_upsert() in vy_lsm.c:1018. The result is also always
assumed to be REPLACE. Is it correct?

Re: [Tarantool-patches] [PATCH] vinyl: fix assert in vy_tx_write()

[Nikita Pettik]

On 21 May 16:12, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 21/05/2020 03:15, Nikita Pettik wrote:
> > Assert in vy_tx_write() validates that upsert applied on the top of
> > cached statement is always replace. In fact, it is not always so. If
> > vy_apply_upsert() fails due to the fact that PK is modified, it returns
> > old statement (i.e. statement which upsert is applied on). In this
> > regard, if tuple cache contains insert and invalid upsert is executed,
> > vy_apply_upsert() will return insert. As a result, assert will fire.
> > Let's fix it and take into account that vy_apply_upsert() is able to
> > return inserts as well.
> 
> I looked at other places in vinyl, which apply an upsert, and found
> vy_lsm_commit_upsert() in vy_lsm.c:1018. The result is also always
> assumed to be REPLACE. Is it correct?

No, it's not a case. If vy_apply_upsert() fails due to PK modification,
it will return old_stmt. In turn old_stmt has different LSN, so flaw
returns before this assert:

  1030			if (upserted_lsn != lsn) {
  1031				/**
  1032				 * This could only happen if the upsert completely
  1033				 * failed and the old tuple was returned.
  1034				 * In this case we shouldn't insert the same replace
  1035				 * again.
  1036				 */
  1037				assert(older.stmt == NULL ||
  1038				       upserted_lsn == vy_stmt_lsn(older.stmt));
  1039				tuple_unref(upserted.stmt);
  1040				return;
  1041			}

Note that they must have different LSN since they can't come in one
tx set. Otherwise, during apply_upsert() in vy_tx_set_entry() upsert
turns into old_stmt (insert) and simply skipped (due to is_overwritten
flag) during statement commit.

Re: [Tarantool-patches] [PATCH] vinyl: fix assert in vy_tx_write()

[Vladislav Shpilevoy]

Thanks for the patch!

LGTM.

Re: [Tarantool-patches] [PATCH] vinyl: fix assert in vy_tx_write()

[Nikita Pettik]

On 26 May 23:44, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> LGTM.

Pushed to master, 2.4, 2.3 and 1.10. Changelogs are updated correspondingly.
Branch is dropped.