From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 31A2A293D5 for ; Thu, 16 Aug 2018 18:24:58 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id esa21LyGq0qD for ; Thu, 16 Aug 2018 18:24:58 -0400 (EDT) Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id CCE2321A77 for ; Thu, 16 Aug 2018 18:24:57 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] sql: after table rename properly update indexes References: <20180815085442.7bajsapuf7wrlu3r@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 17 Aug 2018 01:24:55 +0300 MIME-Version: 1.0 In-Reply-To: <20180815085442.7bajsapuf7wrlu3r@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Kirill Yukhin Cc: tarantool-patches@freelists.org Hi! Thanks for the fixes! Unfortunately you have ignored some of my fixes pushed on the branch, so I pushed them again on a separate commit. Please, be careful doing a force push. >>> diff --git a/src/box/sql.c b/src/box/sql.c >>> index ae12cae..e2d3cc1 100644 >>> --- a/src/box/sql.c >>> +++ b/src/box/sql.c >>> @@ -840,6 +840,55 @@ rename_fail: >>> return SQL_TARANTOOL_ERROR; >>> } >>> +int >>> +sql_update_index_table_name(uint32_t space_id, uint32_t iid, >>> + const char *new_tbl_name, char **sql_stmt) >> >> 4. I think, it would be enough to pass index_def (that you have >> on the stack above). It has both index id and space id. And here >> you use space_cache_find and struct space only to get index_def. > Done. This fix cures assert in p 3 1. Unfortunately, not done. > Regards, Kirill Yukhin > > commit 4cf29331f3730658351701122d8714e157e0d77b > Author: Kirill Yukhin > Date: Fri Aug 10 15:22:19 2018 +0300 > > sql: after table rename properly update indexes > > After table was altered due to rename, indexes were not updated. > Re-create all table's indexes (in legacy SQL DD), also perform > update of corresponding entry in _index for each index to fix > table name in create statement. > > Closes #3613 > --- > src/box/sql.c | 49 +++++++++++++++++++++++++++--- > src/box/sql/build.c | 6 ++-- > src/box/sql/tarantoolInt.h | 25 +++++++++++++-- > src/box/sql/vdbe.c | 27 ++++++++++++++-- > test/sql/gh-3613-idx-alter-update.result | 38 +++++++++++++++++++++++ > test/sql/gh-3613-idx-alter-update.test.lua | 21 +++++++++++++ > 6 files changed, 156 insertions(+), 10 deletions(-) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index cdf2bfc..6247f18 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1245,13 +1245,15 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId, > > /* Format "opts" and "parts" for _index entry. */ > zOpts = sqlite3DbMallocRaw(pParse->db, > - tarantoolSqlite3MakeIdxOpts(pIndex, zSql, > + tarantoolSqlite3MakeIdxOpts(IsUniqueIndex(pIndex), > + zSql, > NULL) + 2. Looks like you has ignored my fixes here and in most other places. Why? > tarantoolSqlite3MakeIdxParts(pIndex, > NULL) + 2); > if (!zOpts) > diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h > index 94517f6..234c94b 100644 > --- a/src/box/sql/tarantoolInt.h > +++ b/src/box/sql/tarantoolInt.h > @@ -89,6 +89,23 @@ int tarantoolSqlite3ClearTable(struct space *space); > int > sql_rename_table(uint32_t space_id, const char *new_name, char **sql_stmt); > > +/** > + * Update CREATE INDEX field (def->opt.sql) replacing table name > + * w/ new one in _index space. > + * > + * @param space_id Table's space identifier. > + * @param idef Index definition. > + * @param new_tbl_name new name of table > + * @param[out] sql_stmt CREATE INDEX statement for new name table, > + * can be NULL. > + * > + * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise. > + */ > +int > +sql_update_index_table_name(uint32_t space_id, struct index_def *idef, > + const char *new_tbl_name, char **sql_stmt); > + > + 3. Redundant empty line. > /* Alter trigger statement after rename table. */ > int tarantoolSqlite3RenameTrigger(const char *zTriggerName, > const char *zOldName, const char *zNewName); > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index dc5146f..1166ddb 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -4625,13 +4625,36 @@ case OP_RenameTable: { > db->init.busy = 1; > init.rc = SQLITE_OK; > sql_init_callback(&init, zNewTableName, space_id, 0, zSqlStmt); > - db->init.busy = 0; > rc = init.rc; > - if (rc) { > + if (rc != SQLITE_OK) { > sqlite3CommitInternalChanges(); > + db->init.busy = 0; > goto abort_due_to_error; > } > > + /* Space was altered, refetch the pointer. */ > + space = space_by_id(space_id); > + for (uint32_t i = 0; i < space->index_count; ++i) { > + char *sql_stmt = space->index[i]->def->opts.sql; > + if (sql_stmt == NULL) > + continue; > + rc = sql_update_index_table_name(space_id, space->index[i]->def, > + zNewTableName, &sql_stmt); > + if (rc != SQLITE_OK) > + goto abort_due_to_error; > + > + space = space_by_id(space_id); 4. The same space is looked up before the cycle. > + sql_init_callback(&init, zNewTableName, space_id, > + space->index[i]->def->iid, sql_stmt); > + rc = init.rc; > + if (rc != SQLITE_OK) { > + sqlite3CommitInternalChanges(); > + db->init.busy = 0; > + goto abort_due_to_error; > + } > + } > + db->init.busy = 0; > + > /* > * Rebuild 'CREATE TRIGGER' expressions of all triggers > * created on this table. Sure, this action is not atomic