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 592D0291C4 for ; Tue, 14 Aug 2018 18:22:28 -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 Mfjq15fqH5ti for ; Tue, 14 Aug 2018 18:22:28 -0400 (EDT) Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (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 14DF9291BC for ; Tue, 14 Aug 2018 18:22:27 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] sql: after table rename properly update indexes References: From: Vladislav Shpilevoy Message-ID: Date: Wed, 15 Aug 2018 01:22:25 +0300 MIME-Version: 1.0 In-Reply-To: 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 patch! See 6 comments below and a patch on the branch. On 13/08/2018 07:54, Kirill Yukhin wrote: > After table was altered due to rename, indexes were not updated. > Re-create all table's indexes (in legacy SQL DD), also perform > udate of corresponding entry in _index for each index to fix table 1. Typo: 'udate'. > name in create statement. > > Closes #3613 > --- 2. Please, put here the links to the issue and branch. > src/box/sql.c | 55 +++++++++++++++++++++++++++--- > 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 | 37 ++++++++++++++++++++ > test/sql/gh-3613-idx-alter-update.test.lua | 21 ++++++++++++ > 6 files changed, 161 insertions(+), 10 deletions(-) > create mode 100644 test/sql/gh-3613-idx-alter-update.result > create mode 100644 test/sql/gh-3613-idx-alter-update.test.lua > 3. Running the tests I caught an assertion: [010] sql-tap/alter.test.lua vinyl [ fail ] [010] Test failed! Output from reject file sql-tap/alter.reject: [010] TAP version 13 [010] 1..41 [010] ok - alter-1.1 [010] ok - alter-1.2 [010] [010] Last 15 lines of Tarantool Log file [Instance "app_server"][/Users/v.shpilevoy/Work/Repositories/tarantool/test/var/010_sql-tap/alter.test.lua:vinyl.tarantool.log]: [010] ls: */: No such file or directory [010] Assertion failed: (iid < space->index_count), function sql_update_index_table_name, file /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql.c, line 851. Also just failing test: [016] sql/gh-3613-idx-alter-update.test.lua vinyl [ fail ] [016] [016] Test failed! Result content mismatch: [016] --- sql/gh-3613-idx-alter-update.result Wed Aug 15 00:41:34 2018 [016] +++ sql/gh-3613-idx-alter-update.reject Wed Aug 15 00:42:19 2018 [016] @@ -32,6 +32,7 @@ [016] box.sql.execute('DROP INDEX i ON j3') [016] --- [016] ... [016] +-- Cleanup [016] box.sql.execute('DROP TABLE j3') [016] --- [016] ... [016] > 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. > +{ > + assert(space_id != 0); > + assert(new_tbl_name != NULL); > + struct space *space = space_cache_find(space_id); > + assert(space != NULL); > + assert(iid < space->index_count); > + struct index_def *def = space->index[iid]->def; > + > + bool is_quoted = false; > + *sql_stmt = rename_table(db, *sql_stmt, new_tbl_name, &is_quoted); > + > + uint32_t key_len = mp_sizeof_uint(space_id) + mp_sizeof_uint(iid) + > + mp_sizeof_array(2); > + uint32_t new_opts_sz = tarantoolSqlite3MakeIdxOpts(def->opts.is_unique, > + *sql_stmt, NULL); > + uint32_t op_sz = mp_sizeof_array(1) + mp_sizeof_array(3) + > + mp_sizeof_str(1) + mp_sizeof_uint(4) + new_opts_sz; > + > + char *key_begin = (char*) region_alloc(&fiber()->gc, key_len + op_sz); > + if (key_begin == NULL) { > + diag_set(OutOfMemory, key_len, "region_alloc", "key_begin"); > + return SQL_TARANTOOL_ERROR; > + } > + char *key = mp_encode_array(key_begin, 2); > + key = mp_encode_uint(key, space_id); > + key = mp_encode_uint(key, iid); > + > + char *op_begin = key; > + char *op = mp_encode_array(op_begin, 1); > + op = mp_encode_array(op, 3); > + op = mp_encode_str(op, "=", 1); > + op = mp_encode_uint(op, 4); > + op += tarantoolSqlite3MakeIdxOpts(def->opts.is_unique, *sql_stmt, op); > + > + box_tuple_t *res = NULL; 5. You do not need this dummy res. box_update (as all other DML C API functions) is tolerant to NULL in the last parameter. > + > + if (box_update(BOX_INDEX_ID, 0, key_begin, key, op_begin, op, > + 0, &res) != 0) { > + diag_set(ClientError, ER_UPDATE_FIELD, 4, > + "while updating SQL field"); 6. box_update has already set diag. > + return SQL_TARANTOOL_ERROR; > + } > + > + return SQLITE_OK; > +} > + > int > tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor, > struct UnpackedRecord *unpacked)