[PATCH v3 0/3] Fix DML vs DDL race

[PATCH v3 0/3] Fix DML vs DDL race

[Vladimir Davydov]

In Vinyl a DML request may yield on disk read while still having a
pointer to the target space saved on stack. If the space is altered
during this time window, the DML request will most certainly crash
while trying to dereference the stale pointer when it resumes. The
solution is simple: abort all affected transactions whenever we delete
a space from the cache.

https://github.com/tarantool/tarantool/issues/3420
https://github.com/tarantool/tarantool/commits/dv/gh-3420-vy-abort-tx-in-ddl

Changes in v3:
 - Keep track of the *space* affected by the last prepared statement
   rather than the LSM tree.

v2: https://www.freelists.org/post/tarantool-patches/PATCH-v2-03-Fix-DML-vs-DDL-race

Changes in v2:
 - Instead of introducing a per transaction space hash, simply remember
   the pointer to the last prepared index. Thanks to Kostja for the tip.
 - Drop the patch allowing transactions affecting only replica local
   spaces outlast a switch to read-only mode, as we can't easily
   implement it without a per transaction space hash. Anyway, it was out
   of the scope of this issue.

v1: https://www.freelists.org/post/tarantool-patches/PATCH-04-Fix-DML-vs-DDL-race

Vladimir Davydov (3):
  vinyl: make tx_manager_abort_writers_for_ddl more thorough
  vinyl: abort affected transactions when space is removed from cache
  Revert "test: skip ddl test for vinyl on travis"

 rpm/tarantool.spec             |   2 -
 src/box/blackhole.c            |   1 +
 src/box/memtx_space.c          |   1 +
 src/box/schema.cc              |   3 ++
 src/box/space.c                |   6 +++
 src/box/space.h                |  15 ++++++
 src/box/sysview.c              |   1 +
 src/box/vinyl.c                |  33 ++++++++++---
 src/box/vy_point_lookup.c      |  14 ++++++
 src/box/vy_read_iterator.c     |  13 +++++
 src/box/vy_tx.c                |  14 ++++--
 src/box/vy_tx.h                |  15 ++++--
 test/vinyl/ddl.skipcond        |   6 ---
 test/vinyl/errinj_ddl.result   | 110 +++++++++++++++++++++++++++++++++++++++++
 test/vinyl/errinj_ddl.test.lua |  51 +++++++++++++++++++
 15 files changed, 265 insertions(+), 20 deletions(-)
 delete mode 100644 test/vinyl/ddl.skipcond

-- 
2.11.0

[PATCH v3 1/3] vinyl: make tx_manager_abort_writers_for_ddl more thorough

[Vladimir Davydov]

We need to abort all transactions writing to an altered space when
a new index is built. Currently, we use the write set to look up such
transactions, but it isn't quite correct, because a transaction could
yield on disk read before inserting a statement into the write set.
To address this problem, this patch adds vy_tx->last_stmt_space, which
points to the space affected by the last prepared transaction. Now,
tx_manager_abort_writers_for_ddl will look not only at the write set,
but also at this variable to check if it needs to abort a transaction.

Needed for #3420
---
 src/box/vinyl.c | 12 ++++++------
 src/box/vy_tx.c | 14 +++++++++++---
 src/box/vy_tx.h | 15 ++++++++++++---
 3 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 3ef43e18..a6a5f187 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1034,14 +1034,14 @@ vinyl_space_prepare_alter(struct space *old_space, struct space *new_space)
 /**
  * This function is called after installing on_replace trigger
  * used for propagating changes done during DDL. It aborts all
- * rw transactions affecting the given LSM tree that began
+ * rw transactions affecting the given space that began
  * before the trigger was installed so that DDL doesn't miss
  * their working set.
  */
 static void
-vy_abort_writers_for_ddl(struct vy_env *env, struct vy_lsm *lsm)
+vy_abort_writers_for_ddl(struct vy_env *env, struct space *space)
 {
-	tx_manager_abort_writers_for_ddl(env->xm, lsm);
+	tx_manager_abort_writers_for_ddl(env->xm, space);
 	/*
 	 * Wait for prepared transactions to complete
 	 * (we can't abort them as they reached WAL).
@@ -1115,7 +1115,7 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
 	trigger_create(&on_replace, vy_check_format_on_replace, &ctx, NULL);
 	trigger_add(&space->on_replace, &on_replace);
 
-	vy_abort_writers_for_ddl(env, pk);
+	vy_abort_writers_for_ddl(env, space);
 
 	struct vy_read_iterator itr;
 	vy_read_iterator_open(&itr, pk, NULL, ITER_ALL, pk->env->empty_key,
@@ -2434,7 +2434,7 @@ vinyl_engine_begin_statement(struct engine *engine, struct txn *txn)
 	struct vy_tx *tx = txn->engine_tx;
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	assert(tx != NULL);
-	return vy_tx_begin_statement(tx, &stmt->engine_savepoint);
+	return vy_tx_begin_statement(tx, stmt->space, &stmt->engine_savepoint);
 }
 
 static void
@@ -4229,7 +4229,7 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 	trigger_create(&on_replace, vy_build_on_replace, &ctx, NULL);
 	trigger_add(&src_space->on_replace, &on_replace);
 
-	vy_abort_writers_for_ddl(env, pk);
+	vy_abort_writers_for_ddl(env, src_space);
 
 	struct vy_read_iterator itr;
 	vy_read_iterator_open(&itr, pk, NULL, ITER_ALL, pk->env->empty_key,
diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index 1b8224f4..9c416c0b 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -322,6 +322,7 @@ vy_tx_read_set_free_cb(vy_tx_read_set_t *read_set,
 void
 vy_tx_create(struct tx_manager *xm, struct vy_tx *tx)
 {
+	tx->last_stmt_space = NULL;
 	stailq_create(&tx->log);
 	write_set_new(&tx->write_set);
 	tx->write_set_version = 0;
@@ -869,13 +870,14 @@ vy_tx_rollback(struct vy_tx *tx)
 }
 
 int
-vy_tx_begin_statement(struct vy_tx *tx, void **savepoint)
+vy_tx_begin_statement(struct vy_tx *tx, struct space *space, void **savepoint)
 {
 	if (tx->state == VINYL_TX_ABORT) {
 		diag_set(ClientError, ER_TRANSACTION_CONFLICT);
 		return -1;
 	}
 	assert(tx->state == VINYL_TX_READY);
+	tx->last_stmt_space = space;
 	if (stailq_empty(&tx->log))
 		rlist_add_entry(&tx->xm->writers, tx, in_writers);
 	*savepoint = stailq_last(&tx->log);
@@ -907,6 +909,7 @@ vy_tx_rollback_statement(struct vy_tx *tx, void *svp)
 	}
 	if (stailq_empty(&tx->log))
 		rlist_del_entry(tx, in_writers);
+	tx->last_stmt_space = NULL;
 }
 
 int
@@ -1108,11 +1111,16 @@ vy_tx_set_with_colmask(struct vy_tx *tx, struct vy_lsm *lsm,
 }
 
 void
-tx_manager_abort_writers_for_ddl(struct tx_manager *xm, struct vy_lsm *lsm)
+tx_manager_abort_writers_for_ddl(struct tx_manager *xm, struct space *space)
 {
+	if (space->index_count == 0)
+		return; /* no indexes, no conflicts */
+	struct vy_lsm *lsm = vy_lsm(space->index[0]);
 	struct vy_tx *tx;
 	rlist_foreach_entry(tx, &xm->writers, in_writers) {
-		if (tx->state == VINYL_TX_READY &&
+		if (tx->state != VINYL_TX_READY)
+			continue;
+		if (tx->last_stmt_space == space ||
 		    write_set_search_key(&tx->write_set, lsm,
 					 lsm->env->empty_key) != NULL)
 			vy_tx_abort(tx);
diff --git a/src/box/vy_tx.h b/src/box/vy_tx.h
index aaa31bee..93e3a8cd 100644
--- a/src/box/vy_tx.h
+++ b/src/box/vy_tx.h
@@ -51,6 +51,7 @@
 extern "C" {
 #endif /* defined(__cplusplus) */
 
+struct space;
 struct tuple;
 struct tx_manager;
 struct vy_mem;
@@ -140,6 +141,14 @@ struct vy_tx {
 	/** Transaction manager. */
 	struct tx_manager *xm;
 	/**
+	 * Pointer to the space affected by the last prepared statement.
+	 * We need it so that we can abort a transaction on DDL even
+	 * if it hasn't inserted anything into the write set yet (e.g.
+	 * yielded on unique check) and therefore would otherwise be
+	 * ignored by tx_manager_abort_writers_for_ddl().
+	 */
+	struct space *last_stmt_space;
+	/**
 	 * In memory transaction log. Contains both reads
 	 * and writes.
 	 */
@@ -277,12 +286,12 @@ size_t
 tx_manager_mem_used(struct tx_manager *xm);
 
 /**
- * Abort all rw transactions that affect the given LSM tree
+ * Abort all rw transactions that affect the given space
  * and haven't reached WAL yet. Called before executing a DDL
  * operation.
  */
 void
-tx_manager_abort_writers_for_ddl(struct tx_manager *xm, struct vy_lsm *lsm);
+tx_manager_abort_writers_for_ddl(struct tx_manager *xm, struct space *space);
 
 /**
  * Abort all local rw transactions that haven't reached WAL yet.
@@ -327,7 +336,7 @@ vy_tx_rollback(struct vy_tx *tx);
  * to a save point with vy_tx_rollback_statement().
  */
 int
-vy_tx_begin_statement(struct vy_tx *tx, void **savepoint);
+vy_tx_begin_statement(struct vy_tx *tx, struct space *space, void **savepoint);
 
 /**
  * Rollback a transaction statement.
-- 
2.11.0

[PATCH v3 2/3] vinyl: abort affected transactions when space is removed from cache

[Vladimir Davydov]

A DDL operation creates a new struct space container, moving unaffected
indexes from the old container, then destroying it. The problem is there
may be a DML request for this space, which was passed the old container
in the argument list and then yielded on disk read. When it resumes, it
may try to dereference the old space container, which may have already
been destroyed. This will most certainly result in a crash.

To address this problem, we introduce a new space callback, invalidate,
which is called for the old space on space_cache_replace(). In case of
vinyl, this callback aborts all transactions involving the space. To
prevent a DML request from dereferencing a destroyed space, we also make
the iterator code check the current transaction state after each yield
and return an error if it was aborted. This should make any DML request
bail out early without dereferencing the space anymore.

Closes #3420
---
 src/box/blackhole.c            |   1 +
 src/box/memtx_space.c          |   1 +
 src/box/schema.cc              |   3 ++
 src/box/space.c                |   6 +++
 src/box/space.h                |  15 ++++++
 src/box/sysview.c              |   1 +
 src/box/vinyl.c                |  21 ++++++++
 src/box/vy_point_lookup.c      |  14 ++++++
 src/box/vy_read_iterator.c     |  13 +++++
 test/vinyl/errinj_ddl.result   | 110 +++++++++++++++++++++++++++++++++++++++++
 test/vinyl/errinj_ddl.test.lua |  51 +++++++++++++++++++
 11 files changed, 236 insertions(+)

diff --git a/src/box/blackhole.c b/src/box/blackhole.c
index 95064e1f..b69e543a 100644
--- a/src/box/blackhole.c
+++ b/src/box/blackhole.c
@@ -129,6 +129,7 @@ static const struct space_vtab blackhole_space_vtab = {
 	/* .build_index = */ generic_space_build_index,
 	/* .swap_index = */ generic_space_swap_index,
 	/* .prepare_alter = */ generic_space_prepare_alter,
+	/* .invalidate = */ generic_space_invalidate,
 };
 
 static void
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 92ec0b30..d8529fe0 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -961,6 +961,7 @@ static const struct space_vtab memtx_space_vtab = {
 	/* .build_index = */ memtx_space_build_index,
 	/* .swap_index = */ generic_space_swap_index,
 	/* .prepare_alter = */ memtx_space_prepare_alter,
+	/* .invalidate = */ generic_space_invalidate,
 };
 
 struct space *
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 9e3f5561..79d0de4e 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -257,6 +257,9 @@ space_cache_replace(struct space *old_space, struct space *new_space)
 		diag_log();
 		panic("Can't update space cache");
 	}
+
+	if (old_space != NULL)
+		space_invalidate(old_space);
 }
 
 /** A wrapper around space_new() for data dictionary spaces. */
diff --git a/src/box/space.c b/src/box/space.c
index e140f3d5..1379fa34 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -637,4 +637,10 @@ generic_space_prepare_alter(struct space *old_space, struct space *new_space)
 	return 0;
 }
 
+void
+generic_space_invalidate(struct space *space)
+{
+	(void)space;
+}
+
 /* }}} */
diff --git a/src/box/space.h b/src/box/space.h
index 211706d2..bbb2addd 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -133,6 +133,14 @@ struct space_vtab {
 	 */
 	int (*prepare_alter)(struct space *old_space,
 			     struct space *new_space);
+	/**
+	 * Called right after removing a space from the cache.
+	 * The engine should abort all transactions involving
+	 * the space, because the space will be destroyed soon.
+	 *
+	 * This function isn't allowed to yield or fail.
+	 */
+	void (*invalidate)(struct space *space);
 };
 
 struct space {
@@ -407,6 +415,12 @@ space_prepare_alter(struct space *old_space, struct space *new_space)
 	return new_space->vtab->prepare_alter(old_space, new_space);
 }
 
+static inline void
+space_invalidate(struct space *space)
+{
+	return space->vtab->invalidate(space);
+}
+
 static inline bool
 space_is_memtx(struct space *space) { return space->engine->id == 0; }
 
@@ -469,6 +483,7 @@ int generic_space_check_format(struct space *, struct tuple_format *);
 int generic_space_build_index(struct space *, struct index *,
 			      struct tuple_format *);
 int generic_space_prepare_alter(struct space *, struct space *);
+void generic_space_invalidate(struct space *);
 
 #if defined(__cplusplus)
 } /* extern "C" */
diff --git a/src/box/sysview.c b/src/box/sysview.c
index 63b669d0..f9edd2d0 100644
--- a/src/box/sysview.c
+++ b/src/box/sysview.c
@@ -495,6 +495,7 @@ static const struct space_vtab sysview_space_vtab = {
 	/* .build_index = */ generic_space_build_index,
 	/* .swap_index = */ generic_space_swap_index,
 	/* .prepare_alter = */ generic_space_prepare_alter,
+	/* .invalidate = */ generic_space_invalidate,
 };
 
 static void
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index a6a5f187..da891025 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1031,6 +1031,26 @@ vinyl_space_prepare_alter(struct space *old_space, struct space *new_space)
 	return 0;
 }
 
+static void
+vinyl_space_invalidate(struct space *space)
+{
+	struct vy_env *env = vy_env(space->engine);
+	/*
+	 * Abort all transactions involving the invalidated space.
+	 * An aborted transaction doesn't allow any DML/DQL requests
+	 * so the space won't be used anymore and can be safely
+	 * destroyed.
+	 *
+	 * There's a subtle corner case though - a transaction can
+	 * be reading disk from a DML request right now, with this
+	 * space passed to it in the argument list. However, it's
+	 * handled as well: the iterator will return an error as
+	 * soon as it's done reading disk, which will make the DML
+	 * request bail out early, without dereferencing the space.
+	 */
+	tx_manager_abort_writers_for_ddl(env->xm, space);
+}
+
 /**
  * This function is called after installing on_replace trigger
  * used for propagating changes done during DDL. It aborts all
@@ -4543,6 +4563,7 @@ static const struct space_vtab vinyl_space_vtab = {
 	/* .build_index = */ vinyl_space_build_index,
 	/* .swap_index = */ vinyl_space_swap_index,
 	/* .prepare_alter = */ vinyl_space_prepare_alter,
+	/* .invalidate = */ vinyl_space_invalidate,
 };
 
 static const struct index_vtab vinyl_index_vtab = {
diff --git a/src/box/vy_point_lookup.c b/src/box/vy_point_lookup.c
index 9f7796eb..9e1f3ca7 100644
--- a/src/box/vy_point_lookup.c
+++ b/src/box/vy_point_lookup.c
@@ -198,6 +198,7 @@ vy_point_lookup(struct vy_lsm *lsm, struct vy_tx *tx,
 {
 	/* All key parts must be set for a point lookup. */
 	assert(vy_stmt_is_full_key(key, lsm->cmp_def));
+	assert(tx == NULL || tx->state == VINYL_TX_READY);
 
 	*ret = NULL;
 	double start_time = ev_monotonic_now(loop());
@@ -239,6 +240,19 @@ restart:
 		errinj(ERRINJ_VY_POINT_ITER_WAIT, ERRINJ_BOOL)->bparam = false;
 	});
 
+	if (tx != NULL && tx->state == VINYL_TX_ABORT) {
+		/*
+		 * The transaction was aborted while we were reading
+		 * disk. We must stop now and return an error as this
+		 * function could be called by a DML request aborted
+		 * by a DDL operation: failing early will prevent it
+		 * from dereferencing a destroyed space.
+		 */
+		diag_set(ClientError, ER_TRANSACTION_CONFLICT);
+		rc = -1;
+		goto done;
+	}
+
 	if (mem_list_version != lsm->mem_list_version) {
 		/*
 		 * Mem list was changed during yield. This could be rotation
diff --git a/src/box/vy_read_iterator.c b/src/box/vy_read_iterator.c
index 2864a280..60c43555 100644
--- a/src/box/vy_read_iterator.c
+++ b/src/box/vy_read_iterator.c
@@ -501,6 +501,17 @@ rescan_disk:
 	}
 	vy_read_iterator_unpin_slices(itr);
 	/*
+	 * The transaction could have been aborted while we were
+	 * reading disk. We must stop now and return an error as
+	 * this function could be called by a DML request that
+	 * was aborted by a DDL operation: failing will prevent
+	 * it from dereferencing a destroyed space.
+	 */
+	if (itr->tx != NULL && itr->tx->state == VINYL_TX_ABORT) {
+		diag_set(ClientError, ER_TRANSACTION_CONFLICT);
+		return -1;
+	}
+	/*
 	 * The list of in-memory indexes and/or the range tree could
 	 * have been modified by dump/compaction while we were fetching
 	 * data from disk. Restart the iterator if this is the case.
@@ -842,6 +853,8 @@ vy_read_iterator_track_read(struct vy_read_iterator *itr, struct tuple *stmt)
 NODISCARD int
 vy_read_iterator_next(struct vy_read_iterator *itr, struct tuple **result)
 {
+	assert(itr->tx == NULL || itr->tx->state == VINYL_TX_READY);
+
 	ev_tstamp start_time = ev_monotonic_now(loop());
 
 	struct vy_lsm *lsm = itr->lsm;
diff --git a/test/vinyl/errinj_ddl.result b/test/vinyl/errinj_ddl.result
index 04cb581f..794125e8 100644
--- a/test/vinyl/errinj_ddl.result
+++ b/test/vinyl/errinj_ddl.result
@@ -682,3 +682,113 @@ box.commit()
 s:drop()
 ---
 ...
+--
+-- gh-3420: crash if DML races with DDL.
+--
+fiber = require('fiber')
+---
+...
+-- turn off cache for error injection to work
+default_vinyl_cache = box.cfg.vinyl_cache
+---
+...
+box.cfg{vinyl_cache = 0}
+---
+...
+s = box.schema.space.create('test', {engine = 'vinyl'})
+---
+...
+_ = s:create_index('i1')
+---
+...
+_ = s:create_index('i2', {parts = {2, 'unsigned'}})
+---
+...
+_ = s:create_index('i3', {parts = {3, 'unsigned'}})
+---
+...
+_ = s:create_index('i4', {parts = {4, 'unsigned'}})
+---
+...
+for i = 1, 5 do s:replace({i, i, i, i}) end
+---
+...
+box.snapshot()
+---
+- ok
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+function async(f)
+    local ch = fiber.channel(1)
+    fiber.create(function()
+        local _, res = pcall(f)
+        ch:put(res)
+    end)
+    return ch
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+-- Issue a few DML requests that require reading disk.
+-- Stall disk reads.
+errinj.set('ERRINJ_VY_READ_PAGE_TIMEOUT', 0.01)
+---
+- ok
+...
+ch1 = async(function() s:insert({1, 1, 1, 1}) end)
+---
+...
+ch2 = async(function() s:replace({2, 3, 2, 3}) end)
+---
+...
+ch3 = async(function() s:update({3}, {{'+', 4, 1}}) end)
+---
+...
+ch4 = async(function() s:upsert({5, 5, 5, 5}, {{'+', 4, 1}}) end)
+---
+...
+-- Execute a DDL operation on the space.
+s.index.i4:drop()
+---
+...
+-- Resume the DML requests. Check that they have been aborted.
+errinj.set('ERRINJ_VY_READ_PAGE_TIMEOUT', 0)
+---
+- ok
+...
+ch1:get()
+---
+- Transaction has been aborted by conflict
+...
+ch2:get()
+---
+- Transaction has been aborted by conflict
+...
+ch3:get()
+---
+- Transaction has been aborted by conflict
+...
+ch4:get()
+---
+- Transaction has been aborted by conflict
+...
+s:select()
+---
+- - [1, 1, 1, 1]
+  - [2, 2, 2, 2]
+  - [3, 3, 3, 3]
+  - [4, 4, 4, 4]
+  - [5, 5, 5, 5]
+...
+s:drop()
+---
+...
+box.cfg{vinyl_cache = default_vinyl_cache}
+---
+...
diff --git a/test/vinyl/errinj_ddl.test.lua b/test/vinyl/errinj_ddl.test.lua
index 50dc4aa1..84f33277 100644
--- a/test/vinyl/errinj_ddl.test.lua
+++ b/test/vinyl/errinj_ddl.test.lua
@@ -298,3 +298,54 @@ s:replace{1, 3}
 box.commit()
 
 s:drop()
+
+--
+-- gh-3420: crash if DML races with DDL.
+--
+fiber = require('fiber')
+
+-- turn off cache for error injection to work
+default_vinyl_cache = box.cfg.vinyl_cache
+box.cfg{vinyl_cache = 0}
+
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('i1')
+_ = s:create_index('i2', {parts = {2, 'unsigned'}})
+_ = s:create_index('i3', {parts = {3, 'unsigned'}})
+_ = s:create_index('i4', {parts = {4, 'unsigned'}})
+for i = 1, 5 do s:replace({i, i, i, i}) end
+box.snapshot()
+
+test_run:cmd("setopt delimiter ';'")
+function async(f)
+    local ch = fiber.channel(1)
+    fiber.create(function()
+        local _, res = pcall(f)
+        ch:put(res)
+    end)
+    return ch
+end;
+test_run:cmd("setopt delimiter ''");
+
+-- Issue a few DML requests that require reading disk.
+-- Stall disk reads.
+errinj.set('ERRINJ_VY_READ_PAGE_TIMEOUT', 0.01)
+
+ch1 = async(function() s:insert({1, 1, 1, 1}) end)
+ch2 = async(function() s:replace({2, 3, 2, 3}) end)
+ch3 = async(function() s:update({3}, {{'+', 4, 1}}) end)
+ch4 = async(function() s:upsert({5, 5, 5, 5}, {{'+', 4, 1}}) end)
+
+-- Execute a DDL operation on the space.
+s.index.i4:drop()
+
+-- Resume the DML requests. Check that they have been aborted.
+errinj.set('ERRINJ_VY_READ_PAGE_TIMEOUT', 0)
+ch1:get()
+ch2:get()
+ch3:get()
+ch4:get()
+s:select()
+s:drop()
+
+box.cfg{vinyl_cache = default_vinyl_cache}
-- 
2.11.0

[PATCH v3 3/3] Revert "test: skip ddl test for vinyl on travis"

[Vladimir Davydov]

This reverts commit c2de45c4686405d199eb22273706a3d52cadea3b.

Not needed anymore as the DDL bug it was supposed to work around has
been finally fixed.

Follow-up #3420
---
 rpm/tarantool.spec      | 2 --
 test/vinyl/ddl.skipcond | 6 ------
 2 files changed, 8 deletions(-)
 delete mode 100644 test/vinyl/ddl.skipcond

diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
index bd2469d9..c87b1667 100644
--- a/rpm/tarantool.spec
+++ b/rpm/tarantool.spec
@@ -149,8 +149,6 @@ rm -rf %{buildroot}%{_datarootdir}/doc/tarantool/
 echo "self.skip = True" > ./test/app/socket.skipcond
 # https://github.com/tarantool/tarantool/issues/1322
 echo "self.skip = True" > ./test/app/digest.skipcond
-# https://github.com/tarantool/tarantool/issues/3420
-echo "self.skip = True" > ./test/vinyl/ddl.skipcond
 # run a safe subset of the test suite
 cd test && ./test-run.py -j 1 unit/ app/ app-tap/ box/ box-tap/ engine/ vinyl/
 %endif
diff --git a/test/vinyl/ddl.skipcond b/test/vinyl/ddl.skipcond
deleted file mode 100644
index 3ef55854..00000000
--- a/test/vinyl/ddl.skipcond
+++ /dev/null
@@ -1,6 +0,0 @@
-# vim: set ft=python :
-import os
-
-# Travis CI fails because of bug #3420
-if os.environ.get('TRAVIS_JOB_ID', False):
-    self.skip = 1
-- 
2.11.0

Re: [PATCH v3 0/3] Fix DML vs DDL race

[Vladimir Davydov]

On Thu, Mar 28, 2019 at 07:17:47PM +0300, Vladimir Davydov wrote:
> In Vinyl a DML request may yield on disk read while still having a
> pointer to the target space saved on stack. If the space is altered
> during this time window, the DML request will most certainly crash
> while trying to dereference the stale pointer when it resumes. The
> solution is simple: abort all affected transactions whenever we delete
> a space from the cache.
> 
> https://github.com/tarantool/tarantool/issues/3420
> https://github.com/tarantool/tarantool/commits/dv/gh-3420-vy-abort-tx-in-ddl
> 
> Changes in v3:
>  - Keep track of the *space* affected by the last prepared statement
>    rather than the LSM tree.
> 
> v2: https://www.freelists.org/post/tarantool-patches/PATCH-v2-03-Fix-DML-vs-DDL-race
> 
> Changes in v2:
>  - Instead of introducing a per transaction space hash, simply remember
>    the pointer to the last prepared index. Thanks to Kostja for the tip.
>  - Drop the patch allowing transactions affecting only replica local
>    spaces outlast a switch to read-only mode, as we can't easily
>    implement it without a per transaction space hash. Anyway, it was out
>    of the scope of this issue.
> 
> v1: https://www.freelists.org/post/tarantool-patches/PATCH-04-Fix-DML-vs-DDL-race
> 
> Vladimir Davydov (3):
>   vinyl: make tx_manager_abort_writers_for_ddl more thorough
>   vinyl: abort affected transactions when space is removed from cache
>   Revert "test: skip ddl test for vinyl on travis"
> 
>  rpm/tarantool.spec             |   2 -
>  src/box/blackhole.c            |   1 +
>  src/box/memtx_space.c          |   1 +
>  src/box/schema.cc              |   3 ++
>  src/box/space.c                |   6 +++
>  src/box/space.h                |  15 ++++++
>  src/box/sysview.c              |   1 +
>  src/box/vinyl.c                |  33 ++++++++++---
>  src/box/vy_point_lookup.c      |  14 ++++++
>  src/box/vy_read_iterator.c     |  13 +++++
>  src/box/vy_tx.c                |  14 ++++--
>  src/box/vy_tx.h                |  15 ++++--
>  test/vinyl/ddl.skipcond        |   6 ---
>  test/vinyl/errinj_ddl.result   | 110 +++++++++++++++++++++++++++++++++++++++++
>  test/vinyl/errinj_ddl.test.lua |  51 +++++++++++++++++++
>  15 files changed, 265 insertions(+), 20 deletions(-)
>  delete mode 100644 test/vinyl/ddl.skipcond

Pushed to master, 2.1, 1.10.