From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Vladimir Davydov Subject: [PATCH 3/3] alter: fix rollback in case DDL and DML are used in the same transaction Date: Tue, 23 Jul 2019 18:49:28 +0300 Message-Id: In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit To: tarantool-patches@freelists.org List-ID: 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