[PATCH 3/3] alter: fix rollback in case DDL and DML are used in the same transaction
Vladimir Davydov
vdavydov.dev at gmail.com
Tue Jul 23 18:49:28 MSK 2019
A txn_stmt keeps a reference to the space it modifies. Memtx uses this
space reference to revert the statement on error or voluntary rollback
so the space must stay valid throughout the whole transaction.
The problem is DML statements may be mixed in a DDL transaction, in
which case we always roll back DML statements first and only then run
rollback triggers to revert changes done to the schema by the DDL
statements. This means every DDL statement modifying the space cache
must be implemented in such a way that it leaves both the old space
and the new space valid until commit or rollback. All DDL operations
follow this rule except MoveIndex and ModifyIndex, which swap indexes
between the old and the new spaces thus making the old space refer to
a place-holder index. An attempt to revert a change from such a dummy
index will most likely lead to a crash.
To fix this, let's split the index swapping operation into two steps:
first assign the old index to the new space without removing it from
the old space; then, on commit, replace the old index with the new
(place-holder) index in the old space so that it gets destroyed by the
alter_space destructor.
Closes #4368
---
src/box/alter.cc | 65 +++++++++++++++++++++++------------
src/box/blackhole.c | 2 +-
src/box/memtx_space.c | 2 +-
src/box/space.c | 15 ++++----
src/box/space.h | 26 +++++---------
src/box/sysview.c | 2 +-
src/box/vinyl.c | 26 ++++----------
src/box/vy_lsm.c | 17 +++++++++
src/box/vy_lsm.h | 9 +++++
test/box/transaction.result | 27 +++++++++++++++
test/box/transaction.test.lua | 20 +++++++++++
11 files changed, 140 insertions(+), 71 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 8668688c..cd01d734 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1134,24 +1134,46 @@ DropIndex::commit(struct alter_space *alter, int64_t signature)
class MoveIndex: public AlterSpaceOp
{
public:
- MoveIndex(struct alter_space *alter, uint32_t iid_arg)
- :AlterSpaceOp(alter), iid(iid_arg) {}
- /** id of the index on the move. */
- uint32_t iid;
+ MoveIndex(struct alter_space *alter, struct index *index)
+ : AlterSpaceOp(alter), old_index(index), new_index(NULL) {}
+ struct index *old_index;
+ struct index *new_index;
virtual void alter(struct alter_space *alter);
+ virtual void commit(struct alter_space *alter, int64_t lsn);
virtual void rollback(struct alter_space *alter);
};
void
MoveIndex::alter(struct alter_space *alter)
{
- space_swap_index(alter->old_space, alter->new_space, iid, iid);
+ /*
+ * Replace the new index with the old index in the new space,
+ * but don't remove the old index from the old space so that
+ * the old space stays valid. We must guarantee the validity
+ * of the old space, because txn statements corresponding to
+ * DML requests processed in the same transaction may still
+ * use it for rollback.
+ */
+ new_index = space_index(alter->new_space, old_index->def->iid);
+ alter->new_space->index_map[new_index->def->iid] = old_index;
+ space_move_index(alter->new_space, old_index);
+}
+
+void
+MoveIndex::commit(struct alter_space *alter, int64_t)
+{
+ /*
+ * Assign the new index to the old space so that it gets
+ * destroyed by the alter_space destructor.
+ */
+ alter->old_space->index_map[old_index->def->iid] = new_index;
}
void
MoveIndex::rollback(struct alter_space *alter)
{
- space_swap_index(alter->old_space, alter->new_space, iid, iid);
+ alter->new_space->index_map[new_index->def->iid] = new_index;
+ space_move_index(alter->old_space, old_index);
}
/**
@@ -1201,34 +1223,33 @@ ModifyIndex::alter(struct alter_space *alter)
new_index = space_index(alter->new_space, new_index_def->iid);
assert(old_index->def->iid == new_index->def->iid);
/*
- * Move the old index to the new space to preserve the
- * original data, but use the new definition.
+ * Update the old index definition and copy it to the new
+ * space. See also MoveIndex::alter.
*/
- space_swap_index(alter->old_space, alter->new_space,
- old_index->def->iid, new_index->def->iid);
- SWAP(old_index, new_index);
SWAP(old_index->def, new_index->def);
- index_update_def(new_index);
+ index_update_def(old_index);
+ alter->new_space->index_map[new_index->def->iid] = old_index;
+ space_move_index(alter->new_space, old_index);
}
void
ModifyIndex::commit(struct alter_space *alter, int64_t signature)
{
- (void)alter;
- index_commit_modify(new_index, signature);
+ index_commit_modify(old_index, signature);
+ /*
+ * Assign the new index to the old space so that it gets
+ * destroyed by the alter_space destructor.
+ */
+ alter->old_space->index_map[old_index->def->iid] = new_index;
}
void
ModifyIndex::rollback(struct alter_space *alter)
{
- /*
- * Restore indexes.
- */
- space_swap_index(alter->old_space, alter->new_space,
- old_index->def->iid, new_index->def->iid);
- SWAP(old_index, new_index);
SWAP(old_index->def, new_index->def);
index_update_def(old_index);
+ alter->new_space->index_map[new_index->def->iid] = new_index;
+ space_move_index(alter->old_space, old_index);
}
ModifyIndex::~ModifyIndex()
@@ -1647,7 +1668,7 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
min_field_count);
(void) new ModifyIndex(alter, old_index, new_def);
} else {
- (void) new MoveIndex(alter, old_def->iid);
+ (void) new MoveIndex(alter, old_index);
}
continue;
}
@@ -2251,7 +2272,7 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
alter_space_move_indexes(alter, 0, iid);
if (index_def_cmp(index_def, old_index->def) == 0) {
/* Index is not changed so just move it. */
- (void) new MoveIndex(alter, old_index->def->iid);
+ (void) new MoveIndex(alter, old_index);
} else if (index_def_change_requires_rebuild(old_index,
index_def)) {
if (index_is_used_by_fk_constraint(&old_space->parent_fk_constraint,
diff --git a/src/box/blackhole.c b/src/box/blackhole.c
index b69e543a..c0652fe1 100644
--- a/src/box/blackhole.c
+++ b/src/box/blackhole.c
@@ -127,7 +127,7 @@ static const struct space_vtab blackhole_space_vtab = {
/* .drop_primary_key = */ generic_space_drop_primary_key,
/* .check_format = */ generic_space_check_format,
/* .build_index = */ generic_space_build_index,
- /* .swap_index = */ generic_space_swap_index,
+ /* .move_index = */ generic_space_move_index,
/* .prepare_alter = */ generic_space_prepare_alter,
/* .invalidate = */ generic_space_invalidate,
};
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index f6cc943f..65f0bc6f 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -1165,7 +1165,7 @@ static const struct space_vtab memtx_space_vtab = {
/* .drop_primary_key = */ memtx_space_drop_primary_key,
/* .check_format = */ memtx_space_check_format,
/* .build_index = */ memtx_space_build_index,
- /* .swap_index = */ generic_space_swap_index,
+ /* .move_index = */ generic_space_move_index,
/* .prepare_alter = */ memtx_space_prepare_alter,
/* .invalidate = */ generic_space_invalidate,
};
diff --git a/src/box/space.c b/src/box/space.c
index f520b307..9dcba2fe 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -283,14 +283,6 @@ space_index_key_def(struct space *space, uint32_t id)
return NULL;
}
-void
-generic_space_swap_index(struct space *old_space, struct space *new_space,
- uint32_t old_index_id, uint32_t new_index_id)
-{
- SWAP(old_space->index_map[old_index_id],
- new_space->index_map[new_index_id]);
-}
-
void
space_run_triggers(struct space *space, bool yesno)
{
@@ -679,6 +671,13 @@ generic_space_build_index(struct space *src_space, struct index *new_index,
return 0;
}
+void
+generic_space_move_index(struct space *space, struct index *index)
+{
+ (void)space;
+ (void)index;
+}
+
int
generic_space_prepare_alter(struct space *old_space, struct space *new_space)
{
diff --git a/src/box/space.h b/src/box/space.h
index d44c9fdc..355918d2 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -128,12 +128,12 @@ struct space_vtab {
int (*build_index)(struct space *src_space, struct index *new_index,
struct tuple_format *new_format, bool check_unique);
/**
- * Exchange two index objects in two spaces. Used
- * to update a space with a newly built index, while
- * making sure the old index doesn't leak.
+ * Called after assigning the given index to the space to
+ * update engine-specific data that depends on the space
+ * definition. E.g. the vinyl engine uses this method to
+ * update tuple formats referenced by the index.
*/
- void (*swap_index)(struct space *old_space, struct space *new_space,
- uint32_t old_index_id, uint32_t new_index_id);
+ void (*move_index)(struct space *space, struct index *index);
/**
* Notify the engine about the changed space,
* before it's done, to prepare 'new_space' object.
@@ -384,14 +384,6 @@ space_ephemeral_delete(struct space *space, const char *key)
return space->vtab->ephemeral_delete(space, key);
}
-/**
- * Generic implementation of space_vtab::swap_index
- * that simply swaps the two indexes in index maps.
- */
-void
-generic_space_swap_index(struct space *old_space, struct space *new_space,
- uint32_t old_index_id, uint32_t new_index_id);
-
static inline void
init_system_space(struct space *space)
{
@@ -437,12 +429,9 @@ space_build_index(struct space *src_space, struct index *new_index,
}
static inline void
-space_swap_index(struct space *old_space, struct space *new_space,
- uint32_t old_index_id, uint32_t new_index_id)
+space_move_index(struct space *space, struct index *index)
{
- assert(old_space->vtab == new_space->vtab);
- return new_space->vtab->swap_index(old_space, new_space,
- old_index_id, new_index_id);
+ return space->vtab->move_index(space, index);
}
static inline int
@@ -519,6 +508,7 @@ void generic_space_drop_primary_key(struct space *space);
int generic_space_check_format(struct space *, struct tuple_format *);
int generic_space_build_index(struct space *, struct index *,
struct tuple_format *, bool);
+void generic_space_move_index(struct space *, struct index *);
int generic_space_prepare_alter(struct space *, struct space *);
void generic_space_invalidate(struct space *);
diff --git a/src/box/sysview.c b/src/box/sysview.c
index 46cf1e13..13ade9b1 100644
--- a/src/box/sysview.c
+++ b/src/box/sysview.c
@@ -506,7 +506,7 @@ static const struct space_vtab sysview_space_vtab = {
/* .drop_primary_key = */ generic_space_drop_primary_key,
/* .check_format = */ generic_space_check_format,
/* .build_index = */ generic_space_build_index,
- /* .swap_index = */ generic_space_swap_index,
+ /* .move_index = */ generic_space_move_index,
/* .prepare_alter = */ generic_space_prepare_alter,
/* .invalidate = */ generic_space_invalidate,
};
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index d2a02a21..d3658a31 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1171,26 +1171,12 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
}
static void
-vinyl_space_swap_index(struct space *old_space, struct space *new_space,
- uint32_t old_index_id, uint32_t new_index_id)
+vinyl_space_move_index(struct space *space, struct index *index)
{
- struct vy_lsm *old_lsm = vy_lsm(old_space->index_map[old_index_id]);
- struct vy_lsm *new_lsm = vy_lsm(new_space->index_map[new_index_id]);
-
- /*
- * Swap the two indexes between the two spaces,
- * but leave tuple formats.
- */
- generic_space_swap_index(old_space, new_space,
- old_index_id, new_index_id);
-
- SWAP(old_lsm, new_lsm);
- SWAP(old_lsm->mem_format, new_lsm->mem_format);
- SWAP(old_lsm->disk_format, new_lsm->disk_format);
-
- /* Update pointer to the primary key. */
- vy_lsm_update_pk(old_lsm, vy_lsm(old_space->index_map[0]));
- vy_lsm_update_pk(new_lsm, vy_lsm(new_space->index_map[0]));
+ struct vy_lsm *lsm = vy_lsm(index);
+ struct vy_lsm *pk = vy_lsm(space_index(space, 0));
+ vy_lsm_update_pk(lsm, pk);
+ vy_lsm_update_format(lsm, space->format);
}
static int
@@ -4726,7 +4712,7 @@ static const struct space_vtab vinyl_space_vtab = {
/* .drop_primary_key = */ generic_space_drop_primary_key,
/* .check_format = */ vinyl_space_check_format,
/* .build_index = */ vinyl_space_build_index,
- /* .swap_index = */ vinyl_space_swap_index,
+ /* .move_index = */ vinyl_space_move_index,
/* .prepare_alter = */ vinyl_space_prepare_alter,
/* .invalidate = */ vinyl_space_invalidate,
};
diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
index c67db87a..e824a6aa 100644
--- a/src/box/vy_lsm.c
+++ b/src/box/vy_lsm.c
@@ -284,6 +284,23 @@ vy_lsm_delete(struct vy_lsm *lsm)
free(lsm);
}
+void
+vy_lsm_update_format(struct vy_lsm *lsm, struct tuple_format *format)
+{
+ tuple_format_ref(format);
+ tuple_format_unref(lsm->mem_format);
+ lsm->mem_format = format;
+ /*
+ * Disk format is the same across all secondary indexes
+ * and it doesn't depend on the space format.
+ */
+ if (lsm->index_id == 0) {
+ tuple_format_ref(format);
+ tuple_format_unref(lsm->disk_format);
+ lsm->disk_format = format;
+ }
+}
+
int
vy_lsm_create(struct vy_lsm *lsm)
{
diff --git a/src/box/vy_lsm.h b/src/box/vy_lsm.h
index d9e4b582..597ad099 100644
--- a/src/box/vy_lsm.h
+++ b/src/box/vy_lsm.h
@@ -340,6 +340,15 @@ vy_lsm_new(struct vy_lsm_env *lsm_env, struct vy_cache_env *cache_env,
void
vy_lsm_delete(struct vy_lsm *lsm);
+/**
+ * Update the formats referenced by the given LSM tree after
+ * it has been assigned to another space container on DDL.
+ * The new format must be compatible with tuples stored in
+ * the space.
+ */
+void
+vy_lsm_update_format(struct vy_lsm *lsm, struct tuple_format *format);
+
/**
* Return true if the LSM tree has no statements, neither on disk
* nor in memory.
diff --git a/test/box/transaction.result b/test/box/transaction.result
index 9a48f2f7..4ebd820e 100644
--- a/test/box/transaction.result
+++ b/test/box/transaction.result
@@ -778,3 +778,30 @@ box.begin() drop() box.rollback()
box.begin() drop() box.commit()
---
...
+--
+-- gh-4368: transaction rollback leads to a crash if DDL and DML statements
+-- are mixed in the same transaction.
+--
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+for i = 1, 100 do
+ box.begin()
+ s = box.schema.space.create('test')
+ s:create_index('pk')
+ s:create_index('sk', {unique = true, parts = {2, 'unsigned'}})
+ s:insert{1, 1}
+ s.index.sk:alter{unique = false}
+ s:insert{2, 1}
+ s.index.sk:drop()
+ s:delete{2}
+ box.rollback()
+ fiber.sleep(0)
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
diff --git a/test/box/transaction.test.lua b/test/box/transaction.test.lua
index 0792cc3c..79929b93 100644
--- a/test/box/transaction.test.lua
+++ b/test/box/transaction.test.lua
@@ -415,3 +415,23 @@ box.begin() create() box.rollback()
box.begin() create() box.commit()
box.begin() drop() box.rollback()
box.begin() drop() box.commit()
+
+--
+-- gh-4368: transaction rollback leads to a crash if DDL and DML statements
+-- are mixed in the same transaction.
+--
+test_run:cmd("setopt delimiter ';'")
+for i = 1, 100 do
+ box.begin()
+ s = box.schema.space.create('test')
+ s:create_index('pk')
+ s:create_index('sk', {unique = true, parts = {2, 'unsigned'}})
+ s:insert{1, 1}
+ s.index.sk:alter{unique = false}
+ s:insert{2, 1}
+ s.index.sk:drop()
+ s:delete{2}
+ box.rollback()
+ fiber.sleep(0)
+end;
+test_run:cmd("setopt delimiter ''");
--
2.20.1
More information about the Tarantool-patches
mailing list