From: Vladimir Davydov <vdavydov.dev@gmail.com> To: kostja@tarantool.org Cc: tarantool-patches@freelists.org Subject: [PATCH v2 2/3] ddl: don't use space_index from AlterSpaceOp::commit,rollback Date: Wed, 10 Jul 2019 16:09:28 +0300 [thread overview] Message-ID: <32e42602f21a0b40394e915f013b837cea35ce6f.1562763283.git.vdavydov.dev@gmail.com> (raw) In-Reply-To: <cover.1562763283.git.vdavydov.dev@gmail.com> In-Reply-To: <cover.1562763283.git.vdavydov.dev@gmail.com> If there are multiple DDL operations in the same transactions, which is impossible now, but will be implemented soon, AlterSpaceOp::commit and rollback methods must not access space index map. To understand that, consider the following example: - on_replace: AlterSpaceOp1 creates index I1 for space S1 - on_replace: AlterSpaceOp2 moves index I1 from space S1 to space S2 - on_commit: AlterSpaceOp1 commits creation of index I1 AlterSpaceOp1 can't lookup I1 in S1 by id, because the index was moved from S1 to S2 by AlterSpaceOp2. If AlterSpaceOp1 attempts to look it up, it will access a wrong index. Fix that by storing pointers to old and new indexes in AlterSpaceOp if required. --- src/box/alter.cc | 77 +++++++++++++++++++++++--------------------------------- 1 file changed, 32 insertions(+), 45 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index ce0cf2d9..612bcd89 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -1088,10 +1088,9 @@ ModifySpace::~ModifySpace() class DropIndex: public AlterSpaceOp { public: - DropIndex(struct alter_space *alter, struct index_def *def_arg) - :AlterSpaceOp(alter), old_index_def(def_arg) {} - /** A reference to the definition of the dropped index. */ - struct index_def *old_index_def; + DropIndex(struct alter_space *alter, struct index *index) + :AlterSpaceOp(alter), old_index(index) {} + struct index *old_index; virtual void alter_def(struct alter_space *alter); virtual void prepare(struct alter_space *alter); virtual void commit(struct alter_space *alter, int64_t lsn); @@ -1104,24 +1103,22 @@ public: void DropIndex::alter_def(struct alter_space * /* alter */) { - rlist_del_entry(old_index_def, link); + rlist_del_entry(old_index->def, link); } /* Do the drop. */ void DropIndex::prepare(struct alter_space *alter) { - if (old_index_def->iid == 0) + if (old_index->def->iid == 0) space_drop_primary_key(alter->new_space); } void DropIndex::commit(struct alter_space *alter, int64_t signature) { - struct index *index = space_index(alter->old_space, - old_index_def->iid); - assert(index != NULL); - index_commit_drop(index, signature); + (void)alter; + index_commit_drop(old_index, signature); } /** @@ -1160,15 +1157,14 @@ class ModifyIndex: public AlterSpaceOp { public: ModifyIndex(struct alter_space *alter, - struct index_def *new_index_def_arg, - struct index_def *old_index_def_arg) - : AlterSpaceOp(alter),new_index_def(new_index_def_arg), - old_index_def(old_index_def_arg) { + struct index *index, struct index_def *def) + : AlterSpaceOp(alter), old_index(index), + new_index(NULL), new_index_def(def) { if (new_index_def->iid == 0 && key_part_cmp(new_index_def->key_def->parts, new_index_def->key_def->part_count, - old_index_def->key_def->parts, - old_index_def->key_def->part_count) != 0) { + old_index->def->key_def->parts, + old_index->def->key_def->part_count) != 0) { /* * Primary parts have been changed - * update secondary indexes. @@ -1176,8 +1172,9 @@ public: alter->pk_def = new_index_def->key_def; } } + struct index *old_index; + struct index *new_index; struct index_def *new_index_def; - struct index_def *old_index_def; virtual void alter_def(struct alter_space *alter); virtual void alter(struct alter_space *alter); virtual void commit(struct alter_space *alter, int64_t lsn); @@ -1189,26 +1186,22 @@ public: void ModifyIndex::alter_def(struct alter_space *alter) { - rlist_del_entry(old_index_def, link); + rlist_del_entry(old_index->def, link); index_def_list_add(&alter->key_list, new_index_def); } void ModifyIndex::alter(struct alter_space *alter) { - assert(old_index_def->iid == new_index_def->iid); + 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. */ space_swap_index(alter->old_space, alter->new_space, - old_index_def->iid, new_index_def->iid); - struct index *old_index = space_index(alter->old_space, - old_index_def->iid); - assert(old_index != NULL); - struct index *new_index = space_index(alter->new_space, - new_index_def->iid); - assert(new_index != NULL); + 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); } @@ -1216,27 +1209,19 @@ ModifyIndex::alter(struct alter_space *alter) void ModifyIndex::commit(struct alter_space *alter, int64_t signature) { - struct index *new_index = space_index(alter->new_space, - new_index_def->iid); - assert(new_index != NULL); + (void)alter; index_commit_modify(new_index, signature); } void ModifyIndex::rollback(struct alter_space *alter) { - assert(old_index_def->iid == new_index_def->iid); /* * Restore indexes. */ space_swap_index(alter->old_space, alter->new_space, - old_index_def->iid, new_index_def->iid); - struct index *old_index = space_index(alter->old_space, - old_index_def->iid); - assert(old_index != NULL); - struct index *new_index = space_index(alter->new_space, - new_index_def->iid); - assert(new_index != NULL); + 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); } @@ -1400,10 +1385,12 @@ class TruncateIndex: public AlterSpaceOp * In case TRUNCATE fails, we need to clean up the new * index data in the engine. */ + struct index *old_index; struct index *new_index; public: TruncateIndex(struct alter_space *alter, uint32_t iid) - : AlterSpaceOp(alter), iid(iid) {} + : AlterSpaceOp(alter), iid(iid), + old_index(NULL), new_index(NULL) {} virtual void prepare(struct alter_space *alter); virtual void commit(struct alter_space *alter, int64_t signature); virtual ~TruncateIndex(); @@ -1412,7 +1399,9 @@ public: void TruncateIndex::prepare(struct alter_space *alter) { + old_index = space_index(alter->old_space, iid); new_index = space_index(alter->new_space, iid); + if (iid == 0) { /* * Notify the engine that the primary index @@ -1437,8 +1426,7 @@ TruncateIndex::prepare(struct alter_space *alter) void TruncateIndex::commit(struct alter_space *alter, int64_t signature) { - struct index *old_index = space_index(alter->old_space, iid); - + (void)alter; index_commit_drop(old_index, signature); index_commit_create(new_index, signature); new_index = NULL; @@ -1655,7 +1643,7 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin, new_def = index_def_dup(old_def); index_def_update_optionality(new_def, min_field_count); - (void) new ModifyIndex(alter, new_def, old_def); + (void) new ModifyIndex(alter, old_index, new_def); } else { (void) new MoveIndex(alter, old_def->iid); } @@ -1672,7 +1660,7 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin, index_def_update_optionality(new_def, min_field_count); auto guard = make_scoped_guard([=] { index_def_delete(new_def); }); if (!index_def_change_requires_rebuild(old_index, new_def)) - (void) new ModifyIndex(alter, new_def, old_def); + (void) new ModifyIndex(alter, old_index, new_def); else (void) new RebuildIndex(alter, new_def, old_def); guard.is_active = false; @@ -2207,7 +2195,7 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) "can not drop a referenced index"); } alter_space_move_indexes(alter, 0, iid); - (void) new DropIndex(alter, old_index->def); + (void) new DropIndex(alter, old_index); } /* Case 2: create an index, if it is simply created. */ if (old_index == NULL && new_tuple != NULL) { @@ -2285,8 +2273,7 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) * in the space conform to the new format. */ (void) new CheckSpaceFormat(alter); - (void) new ModifyIndex(alter, index_def, - old_index->def); + (void) new ModifyIndex(alter, old_index, index_def); index_def_guard.is_active = false; } } -- 2.11.0
next prev parent reply other threads:[~2019-07-10 13:09 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-10 13:09 [PATCH v2 0/3] Transactional DDL Vladimir Davydov 2019-07-10 13:09 ` [PATCH v2 1/3] memtx: fix txn_on_yield for DDL transactions Vladimir Davydov 2019-07-10 20:34 ` Konstantin Osipov 2019-07-12 14:54 ` Vladimir Davydov 2019-07-12 15:16 ` Konstantin Osipov 2019-07-10 13:09 ` Vladimir Davydov [this message] 2019-07-15 15:05 ` [PATCH v2 2/3] ddl: don't use space_index from AlterSpaceOp::commit,rollback Konstantin Osipov 2019-07-10 13:09 ` [PATCH v2 3/3] ddl: allow to execute non-yielding DDL statements in transactions Vladimir Davydov 2019-07-10 20:43 ` Konstantin Osipov 2019-07-12 14:55 ` Vladimir Davydov 2019-07-10 21:07 ` Konstantin Osipov 2019-07-15 15:03 ` Konstantin Osipov 2019-07-15 16:23 ` [PATCH v2 0/3] Transactional DDL Vladimir Davydov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=32e42602f21a0b40394e915f013b837cea35ce6f.1562763283.git.vdavydov.dev@gmail.com \ --to=vdavydov.dev@gmail.com \ --cc=kostja@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='Re: [PATCH v2 2/3] ddl: don'\''t use space_index from AlterSpaceOp::commit,rollback' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox