* [tarantool-patches] [PATCH] sql: after table rename properly update indexes @ 2018-08-13 4:54 Kirill Yukhin 2018-08-14 22:22 ` [tarantool-patches] " Vladislav Shpilevoy 0 siblings, 1 reply; 9+ messages in thread From: Kirill Yukhin @ 2018-08-13 4:54 UTC (permalink / raw) To: v.shpilevoy; +Cc: tarantool-patches, Kirill Yukhin 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 name in create statement. Closes #3613 --- 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 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) +{ + 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; + + 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"); + return SQL_TARANTOOL_ERROR; + } + + return SQLITE_OK; +} + int tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor, struct UnpackedRecord *unpacked) @@ -1494,13 +1543,11 @@ int tarantoolSqlite3MakeIdxParts(SqliteIndex *pIndex, void *buf) * "sql": "CREATE INDEX student_by_name ON students(name)" * } */ -int tarantoolSqlite3MakeIdxOpts(SqliteIndex *index, const char *zSql, void *buf) +int tarantoolSqlite3MakeIdxOpts(bool is_unique, const char *zSql, void *buf) { const struct Enc *enc = get_enc(buf); char *base = buf, *p; - (void)index; - p = enc->encode_map(base, 2); /* Mark as unique pk and unique indexes */ p = enc->encode_str(p, "unique", 6); @@ -1511,7 +1558,7 @@ int tarantoolSqlite3MakeIdxOpts(SqliteIndex *index, const char *zSql, void *buf) * INSERT OR REPLACE/IGNORE uniqueness checks will be also done by * Tarantool. */ - p = enc->encode_bool(p, IsUniqueIndex(index)); + p = enc->encode_bool(p, is_unique); p = enc->encode_str(p, "sql", 3); p = enc->encode_str(p, zSql, zSql ? strlen(zSql) : 0); return (int)(p - base); 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) + tarantoolSqlite3MakeIdxParts(pIndex, NULL) + 2); if (!zOpts) return; - zOptsSz = tarantoolSqlite3MakeIdxOpts(pIndex, zSql, zOpts); + zOptsSz = tarantoolSqlite3MakeIdxOpts(IsUniqueIndex(pIndex), zSql, + zOpts); zParts = zOpts + zOptsSz + 1; zPartsSz = tarantoolSqlite3MakeIdxParts(pIndex, zParts); #if SQLITE_DEBUG diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h index 94517f6..560acf1 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 iid Index identifier. + * @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, uint32_t iid, + const char *new_tbl_name, char **sql_stmt); + + /* Alter trigger statement after rename table. */ int tarantoolSqlite3RenameTrigger(const char *zTriggerName, const char *zOldName, const char *zNewName); @@ -171,12 +188,16 @@ fkey_encode_links(const struct fkey_def *def, int type, char *buf); */ int tarantoolSqlite3MakeIdxParts(Index * index, void *buf); -/* +/** * Format "opts" dictionary for _index entry. * Returns result size. * If buf==NULL estimate result size. + * + * @param is_unique Flag if index is unique. + * @param zSql SQL create stmt. + * @param[out] buf destination buffer. */ -int tarantoolSqlite3MakeIdxOpts(Index * index, const char *zSql, void *buf); +int tarantoolSqlite3MakeIdxOpts(bool is_unique, const char *zSql, void *buf); /** * Extract next id from _sequence space. diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index dc5146f..f70ce1b 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; + uint32_t iid = space->index[i]->def->iid; + rc = sql_update_index_table_name(space_id, iid, + zNewTableName, &sql_stmt); + if (rc != SQLITE_OK) + goto abort_due_to_error; + + sql_init_callback(&init, zNewTableName, space_id, 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 diff --git a/test/sql/gh-3613-idx-alter-update.result b/test/sql/gh-3613-idx-alter-update.result new file mode 100644 index 0000000..6f0b688 --- /dev/null +++ b/test/sql/gh-3613-idx-alter-update.result @@ -0,0 +1,37 @@ +test_run = require('test_run').new() +--- +... +engine = test_run:get_cfg('engine') +--- +... +box.sql.execute('pragma sql_default_engine=\''..engine..'\'') +--- +... +box.sql.execute('CREATE TABLE t (s1 INT PRIMARY KEY)') +--- +... +box.sql.execute('CREATE INDEX i ON t (s1)') +--- +... +box.sql.execute('ALTER TABLE t RENAME TO j3') +--- +... +-- Due to gh-3613, next stmt caused segfault +box.sql.execute('DROP INDEX i ON j3') +--- +... +box.sql.execute('CREATE INDEX i ON j3 (s1)') +--- +... +-- Check that _index was altered properly +box.snapshot() +--- +- ok +... +test_run:cmd('restart server default') +box.sql.execute('DROP INDEX i ON j3') +--- +... +box.sql.execute('DROP TABLE j3') +--- +... diff --git a/test/sql/gh-3613-idx-alter-update.test.lua b/test/sql/gh-3613-idx-alter-update.test.lua new file mode 100644 index 0000000..287096b --- /dev/null +++ b/test/sql/gh-3613-idx-alter-update.test.lua @@ -0,0 +1,21 @@ +test_run = require('test_run').new() +engine = test_run:get_cfg('engine') +box.sql.execute('pragma sql_default_engine=\''..engine..'\'') + +box.sql.execute('CREATE TABLE t (s1 INT PRIMARY KEY)') +box.sql.execute('CREATE INDEX i ON t (s1)') +box.sql.execute('ALTER TABLE t RENAME TO j3') + +-- Due to gh-3613, next stmt caused segfault +box.sql.execute('DROP INDEX i ON j3') + +box.sql.execute('CREATE INDEX i ON j3 (s1)') + +-- Check that _index was altered properly +box.snapshot() +test_run:cmd('restart server default') + +box.sql.execute('DROP INDEX i ON j3') + +-- Cleanup +box.sql.execute('DROP TABLE j3') -- 2.16.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: after table rename properly update indexes 2018-08-13 4:54 [tarantool-patches] [PATCH] sql: after table rename properly update indexes Kirill Yukhin @ 2018-08-14 22:22 ` Vladislav Shpilevoy 2018-08-15 8:54 ` Kirill Yukhin 0 siblings, 1 reply; 9+ messages in thread From: Vladislav Shpilevoy @ 2018-08-14 22:22 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches 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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: after table rename properly update indexes 2018-08-14 22:22 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-08-15 8:54 ` Kirill Yukhin 2018-08-16 22:24 ` Vladislav Shpilevoy 0 siblings, 1 reply; 9+ messages in thread From: Kirill Yukhin @ 2018-08-15 8:54 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches Hello Vlad, Thanks for review! My answers are inlined. Updated patch in the bottom. On 15 авг 01:22, Vladislav Shpilevoy wrote: > 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'. Fixed. > 2. Please, put here the links to the issue and branch. Sure, will do that in future. Here they're, for reference: Branch: https://github.com/tarantool/tarantool/commits/kyukhin/gh-3613-update-idx-afer-alter Issue: https://github.com/tarantool/tarantool/issues/3613 > 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: Good catch! I've added re-fetch of space pointer after alter. > > 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 > > + 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. Fixed. > > + 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. Removed. -- Regards, Kirill Yukhin commit 4cf29331f3730658351701122d8714e157e0d77b Author: Kirill Yukhin <kyukhin@tarantool.org> 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.c b/src/box/sql.c index ae12cae..d9c4df2 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -840,6 +840,49 @@ rename_fail: return SQL_TARANTOOL_ERROR; } +int +sql_update_index_table_name(uint32_t space_id, struct index_def *def, + const char *new_tbl_name, char **sql_stmt) +{ + assert(space_id != 0); + assert(new_tbl_name != NULL); + struct space *space = space_cache_find(space_id); + assert(space != NULL); + uint32_t iid = def->iid; + + 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); + + if (box_update(BOX_INDEX_ID, 0, key_begin, key, op_begin, op, + 0, NULL) != 0) + return SQL_TARANTOOL_ERROR; + + return SQLITE_OK; +} + int tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor, struct UnpackedRecord *unpacked) @@ -1494,13 +1537,11 @@ int tarantoolSqlite3MakeIdxParts(SqliteIndex *pIndex, void *buf) * "sql": "CREATE INDEX student_by_name ON students(name)" * } */ -int tarantoolSqlite3MakeIdxOpts(SqliteIndex *index, const char *zSql, void *buf) +int tarantoolSqlite3MakeIdxOpts(bool is_unique, const char *zSql, void *buf) { const struct Enc *enc = get_enc(buf); char *base = buf, *p; - (void)index; - p = enc->encode_map(base, 2); /* Mark as unique pk and unique indexes */ p = enc->encode_str(p, "unique", 6); @@ -1511,7 +1552,7 @@ int tarantoolSqlite3MakeIdxOpts(SqliteIndex *index, const char *zSql, void *buf) * INSERT OR REPLACE/IGNORE uniqueness checks will be also done by * Tarantool. */ - p = enc->encode_bool(p, IsUniqueIndex(index)); + p = enc->encode_bool(p, is_unique); p = enc->encode_str(p, "sql", 3); p = enc->encode_str(p, zSql, zSql ? strlen(zSql) : 0); return (int)(p - base); 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) + tarantoolSqlite3MakeIdxParts(pIndex, NULL) + 2); if (!zOpts) return; - zOptsSz = tarantoolSqlite3MakeIdxOpts(pIndex, zSql, zOpts); + zOptsSz = tarantoolSqlite3MakeIdxOpts(IsUniqueIndex(pIndex), zSql, + zOpts); zParts = zOpts + zOptsSz + 1; zPartsSz = tarantoolSqlite3MakeIdxParts(pIndex, zParts); #if SQLITE_DEBUG 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); + + /* Alter trigger statement after rename table. */ int tarantoolSqlite3RenameTrigger(const char *zTriggerName, const char *zOldName, const char *zNewName); @@ -171,12 +188,16 @@ fkey_encode_links(const struct fkey_def *def, int type, char *buf); */ int tarantoolSqlite3MakeIdxParts(Index * index, void *buf); -/* +/** * Format "opts" dictionary for _index entry. * Returns result size. * If buf==NULL estimate result size. + * + * @param is_unique Flag if index is unique. + * @param zSql SQL create stmt. + * @param[out] buf destination buffer. */ -int tarantoolSqlite3MakeIdxOpts(Index * index, const char *zSql, void *buf); +int tarantoolSqlite3MakeIdxOpts(bool is_unique, const char *zSql, void *buf); /** * Extract next id from _sequence space. 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); + 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 diff --git a/test/sql/gh-3613-idx-alter-update.result b/test/sql/gh-3613-idx-alter-update.result new file mode 100644 index 0000000..2dcb88e --- /dev/null +++ b/test/sql/gh-3613-idx-alter-update.result @@ -0,0 +1,38 @@ +test_run = require('test_run').new() +--- +... +engine = test_run:get_cfg('engine') +--- +... +box.sql.execute('pragma sql_default_engine=\''..engine..'\'') +--- +... +box.sql.execute('CREATE TABLE t (s1 INT PRIMARY KEY)') +--- +... +box.sql.execute('CREATE INDEX i ON t (s1)') +--- +... +box.sql.execute('ALTER TABLE t RENAME TO j3') +--- +... +-- Due to gh-3613, next stmt caused segfault +box.sql.execute('DROP INDEX i ON j3') +--- +... +box.sql.execute('CREATE INDEX i ON j3 (s1)') +--- +... +-- Check that _index was altered properly +box.snapshot() +--- +- ok +... +test_run:cmd('restart server default') +box.sql.execute('DROP INDEX i ON j3') +--- +... +-- Cleanup +box.sql.execute('DROP TABLE j3') +--- +... diff --git a/test/sql/gh-3613-idx-alter-update.test.lua b/test/sql/gh-3613-idx-alter-update.test.lua new file mode 100644 index 0000000..287096b --- /dev/null +++ b/test/sql/gh-3613-idx-alter-update.test.lua @@ -0,0 +1,21 @@ +test_run = require('test_run').new() +engine = test_run:get_cfg('engine') +box.sql.execute('pragma sql_default_engine=\''..engine..'\'') + +box.sql.execute('CREATE TABLE t (s1 INT PRIMARY KEY)') +box.sql.execute('CREATE INDEX i ON t (s1)') +box.sql.execute('ALTER TABLE t RENAME TO j3') + +-- Due to gh-3613, next stmt caused segfault +box.sql.execute('DROP INDEX i ON j3') + +box.sql.execute('CREATE INDEX i ON j3 (s1)') + +-- Check that _index was altered properly +box.snapshot() +test_run:cmd('restart server default') + +box.sql.execute('DROP INDEX i ON j3') + +-- Cleanup +box.sql.execute('DROP TABLE j3') ^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: after table rename properly update indexes 2018-08-15 8:54 ` Kirill Yukhin @ 2018-08-16 22:24 ` Vladislav Shpilevoy 2018-08-17 5:08 ` Kirill Yukhin 0 siblings, 1 reply; 9+ messages in thread From: Vladislav Shpilevoy @ 2018-08-16 22:24 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches 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 <kyukhin@tarantool.org> > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: after table rename properly update indexes 2018-08-16 22:24 ` Vladislav Shpilevoy @ 2018-08-17 5:08 ` Kirill Yukhin 2018-08-17 8:31 ` Vladislav Shpilevoy 0 siblings, 1 reply; 9+ messages in thread From: Kirill Yukhin @ 2018-08-17 5:08 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches Hello Vlad, My answers are inlined, patch in the bottom, branch force-pushed. On 17 авг 01:24, Vladislav Shpilevoy wrote: > 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. I've picked most of parts of your patch, thanks! > > > > 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. Forgot to remove space_id, thanks for fixing that. > > 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? I've accidentally force pushed my branch removing your fixes. I think we need to post such a `review fixes` patches in the bottom of review message: it can be easily applied w/o git at all and will be saved in history. I'll udate SOP. > > --- 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. Removed. > > 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. We must re-fetch space pointer here since sql_update_index_table_name() causes space to alter so pointer might change (and it does sporadically on vinyl tests). -- Regards, Kirill Yukhin commit c2af7245a9da1f07d0b29a9e80164b1eea6a7a54 Author: Kirill Yukhin <kyukhin@tarantool.org> 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 | 57 ++++++++++++++++++++++-------- src/box/sql/build.c | 7 ++-- src/box/sql/tarantoolInt.h | 23 ++++++++++-- src/box/sql/vdbe.c | 27 ++++++++++++-- test-run | 2 +- test/sql/gh-3613-idx-alter-update.result | 38 ++++++++++++++++++++ test/sql/gh-3613-idx-alter-update.test.lua | 21 +++++++++++ 7 files changed, 153 insertions(+), 22 deletions(-) diff --git a/src/box/sql.c b/src/box/sql.c index ae12cae..9485899 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -840,6 +840,46 @@ rename_fail: return SQL_TARANTOOL_ERROR; } +int +sql_update_index_table_name(struct index_def *def, const char *new_tbl_name, + char **sql_stmt) +{ + assert(new_tbl_name != NULL); + uint32_t iid = def->iid; + + bool is_quoted = false; + *sql_stmt = rename_table(db, def->opts.sql, new_tbl_name, &is_quoted); + + uint32_t key_len = mp_sizeof_uint(def->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, def->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); + + if (box_update(BOX_INDEX_ID, 0, key_begin, key, op_begin, op, + 0, NULL) != 0) + return SQL_TARANTOOL_ERROR; + + return SQLITE_OK; +} + int tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor, struct UnpackedRecord *unpacked) @@ -1484,23 +1524,12 @@ int tarantoolSqlite3MakeIdxParts(SqliteIndex *pIndex, void *buf) return p - base; } -/* - * Format "opts" dictionary for _index entry. - * Returns result size. - * If buf==NULL estimate result size. - * - * Ex: { - * "unique": "true", - * "sql": "CREATE INDEX student_by_name ON students(name)" - * } - */ -int tarantoolSqlite3MakeIdxOpts(SqliteIndex *index, const char *zSql, void *buf) +int +tarantoolSqlite3MakeIdxOpts(bool is_unique, const char *zSql, void *buf) { const struct Enc *enc = get_enc(buf); char *base = buf, *p; - (void)index; - p = enc->encode_map(base, 2); /* Mark as unique pk and unique indexes */ p = enc->encode_str(p, "unique", 6); @@ -1511,7 +1540,7 @@ int tarantoolSqlite3MakeIdxOpts(SqliteIndex *index, const char *zSql, void *buf) * INSERT OR REPLACE/IGNORE uniqueness checks will be also done by * Tarantool. */ - p = enc->encode_bool(p, IsUniqueIndex(index)); + p = enc->encode_bool(p, is_unique); p = enc->encode_str(p, "sql", 3); p = enc->encode_str(p, zSql, zSql ? strlen(zSql) : 0); return (int)(p - base); diff --git a/src/box/sql/build.c b/src/box/sql/build.c index cdf2bfc..8d30e1c 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -1245,13 +1245,14 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId, /* Format "opts" and "parts" for _index entry. */ zOpts = sqlite3DbMallocRaw(pParse->db, - tarantoolSqlite3MakeIdxOpts(pIndex, zSql, - NULL) + + tarantoolSqlite3MakeIdxOpts(IsUniqueIndex(pIndex), + zSql, NULL) + tarantoolSqlite3MakeIdxParts(pIndex, NULL) + 2); if (!zOpts) return; - zOptsSz = tarantoolSqlite3MakeIdxOpts(pIndex, zSql, zOpts); + zOptsSz = tarantoolSqlite3MakeIdxOpts(IsUniqueIndex(pIndex), zSql, + zOpts); zParts = zOpts + zOptsSz + 1; zPartsSz = tarantoolSqlite3MakeIdxParts(pIndex, zParts); #if SQLITE_DEBUG diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h index 94517f6..536b0e3 100644 --- a/src/box/sql/tarantoolInt.h +++ b/src/box/sql/tarantoolInt.h @@ -89,6 +89,20 @@ 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 idef Index definition. + * @param new_tbl_name new name of table + * @param[out] sql_stmt New CREATE INDEX statement. + * + * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise. + */ +int +sql_update_index_table_name(struct index_def *idef, const char *new_tbl_name, + char **sql_stmt); + /* Alter trigger statement after rename table. */ int tarantoolSqlite3RenameTrigger(const char *zTriggerName, const char *zOldName, const char *zNewName); @@ -171,12 +185,17 @@ fkey_encode_links(const struct fkey_def *def, int type, char *buf); */ int tarantoolSqlite3MakeIdxParts(Index * index, void *buf); -/* +/** * Format "opts" dictionary for _index entry. * Returns result size. * If buf==NULL estimate result size. + * + * @param is_unique Flag if index is unique. + * @param zSql SQL create stmt. + * @param[out] buf destination buffer. */ -int tarantoolSqlite3MakeIdxOpts(Index * index, const char *zSql, void *buf); +int +tarantoolSqlite3MakeIdxOpts(bool is_unique, const char *zSql, void *buf); /** * Extract next id from _sequence space. diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index dc5146f..82ad54e 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) { + struct index_def *def = space->index[i]->def; + if (def->opts.sql == NULL) + continue; + char *sql_stmt; + rc = sql_update_index_table_name(def, zNewTableName, &sql_stmt); + if (rc != SQLITE_OK) + goto abort_due_to_error; + + space = space_by_id(space_id); + sql_init_callback(&init, zNewTableName, space_id, 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 diff --git a/test-run b/test-run index ed45e1d..0aa25ae 160000 --- a/test-run +++ b/test-run @@ -1 +1 @@ -Subproject commit ed45e1dbd36ab9109b84ef7189ef9d7e4b813fb9 +Subproject commit 0aa25ae8a9d4af977b3c3478cba3ccdc4ef81d35 diff --git a/test/sql/gh-3613-idx-alter-update.result b/test/sql/gh-3613-idx-alter-update.result new file mode 100644 index 0000000..2dcb88e --- /dev/null +++ b/test/sql/gh-3613-idx-alter-update.result @@ -0,0 +1,38 @@ +test_run = require('test_run').new() +--- +... +engine = test_run:get_cfg('engine') +--- +... +box.sql.execute('pragma sql_default_engine=\''..engine..'\'') +--- +... +box.sql.execute('CREATE TABLE t (s1 INT PRIMARY KEY)') +--- +... +box.sql.execute('CREATE INDEX i ON t (s1)') +--- +... +box.sql.execute('ALTER TABLE t RENAME TO j3') +--- +... +-- Due to gh-3613, next stmt caused segfault +box.sql.execute('DROP INDEX i ON j3') +--- +... +box.sql.execute('CREATE INDEX i ON j3 (s1)') +--- +... +-- Check that _index was altered properly +box.snapshot() +--- +- ok +... +test_run:cmd('restart server default') +box.sql.execute('DROP INDEX i ON j3') +--- +... +-- Cleanup +box.sql.execute('DROP TABLE j3') +--- +... diff --git a/test/sql/gh-3613-idx-alter-update.test.lua b/test/sql/gh-3613-idx-alter-update.test.lua new file mode 100644 index 0000000..287096b --- /dev/null +++ b/test/sql/gh-3613-idx-alter-update.test.lua @@ -0,0 +1,21 @@ +test_run = require('test_run').new() +engine = test_run:get_cfg('engine') +box.sql.execute('pragma sql_default_engine=\''..engine..'\'') + +box.sql.execute('CREATE TABLE t (s1 INT PRIMARY KEY)') +box.sql.execute('CREATE INDEX i ON t (s1)') +box.sql.execute('ALTER TABLE t RENAME TO j3') + +-- Due to gh-3613, next stmt caused segfault +box.sql.execute('DROP INDEX i ON j3') + +box.sql.execute('CREATE INDEX i ON j3 (s1)') + +-- Check that _index was altered properly +box.snapshot() +test_run:cmd('restart server default') + +box.sql.execute('DROP INDEX i ON j3') + +-- Cleanup +box.sql.execute('DROP TABLE j3') ^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: after table rename properly update indexes 2018-08-17 5:08 ` Kirill Yukhin @ 2018-08-17 8:31 ` Vladislav Shpilevoy 2018-08-17 13:48 ` Kirill Yukhin 0 siblings, 1 reply; 9+ messages in thread From: Vladislav Shpilevoy @ 2018-08-17 8:31 UTC (permalink / raw) To: tarantool-patches, Kirill Yukhin Hi! Thanks for the fixes! > diff --git a/test-run b/test-run > index ed45e1d..0aa25ae 160000 > --- a/test-run > +++ b/test-run > @@ -1 +1 @@ > -Subproject commit ed45e1dbd36ab9109b84ef7189ef9d7e4b813fb9 > +Subproject commit 0aa25ae8a9d4af977b3c3478cba3ccdc4ef81d35 Test-run is updated accidentally. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: after table rename properly update indexes 2018-08-17 8:31 ` Vladislav Shpilevoy @ 2018-08-17 13:48 ` Kirill Yukhin 2018-08-17 18:26 ` n.pettik 0 siblings, 1 reply; 9+ messages in thread From: Kirill Yukhin @ 2018-08-17 13:48 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches Hello Vlad, On 17 авг 11:31, Vladislav Shpilevoy wrote: > Hi! Thanks for the fixes! > > > diff --git a/test-run b/test-run > > index ed45e1d..0aa25ae 160000 > > --- a/test-run > > +++ b/test-run > > @@ -1 +1 @@ > > -Subproject commit ed45e1dbd36ab9109b84ef7189ef9d7e4b813fb9 > > +Subproject commit 0aa25ae8a9d4af977b3c3478cba3ccdc4ef81d35 > > Test-run is updated accidentally. I've discarded the hunk & force-pushed the branch. Don't think repost is needed here. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: after table rename properly update indexes 2018-08-17 13:48 ` Kirill Yukhin @ 2018-08-17 18:26 ` n.pettik 2018-08-20 5:21 ` Kirill Yukhin 0 siblings, 1 reply; 9+ messages in thread From: n.pettik @ 2018-08-17 18:26 UTC (permalink / raw) To: tarantool-patches; +Cc: Kirill Yukhin, Vladislav Shpilevoy >> Hi! Thanks for the fixes! >> >>> diff --git a/test-run b/test-run >>> index ed45e1d..0aa25ae 160000 >>> --- a/test-run >>> +++ b/test-run >>> @@ -1 +1 @@ >>> -Subproject commit ed45e1dbd36ab9109b84ef7189ef9d7e4b813fb9 >>> +Subproject commit 0aa25ae8a9d4af977b3c3478cba3ccdc4ef81d35 >> >> Test-run is updated accidentally. > > I've discarded the hunk & force-pushed the branch. Don't think > repost is needed here. Hello, guys. I found this seagfault on updated branch: tarantool> box.sql.execute('CREATE TABLE t (s1 INT PRIMARY KEY)') --- ... tarantool> box.sql.execute('CREATE INDEX i ON t (s1)') --- ... tarantool> box.sql.execute('ALTER TABLE t RENAME TO j3') Ctrl+C tarantool> n:tarantool n.pettik$ ./src/tarantool Tarantool 2.0.4-838-g9d879b042 type 'help' for interactive help tarantool> box.cfg{} 2018-08-17 21:26:17.797 [23781] main/101/interactive C> Tarantool 2.0.4-838-g9d879b042 2018-08-17 21:26:17.797 [23781] main/101/interactive C> log level 5 2018-08-17 21:26:17.798 [23781] main/101/interactive I> mapping 268435456 bytes for memtx tuple arena... 2018-08-17 21:26:17.798 [23781] main/101/interactive I> mapping 134217728 bytes for vinyl tuple arena... 2018-08-17 21:26:17.807 [23781] main/101/interactive I> recovery start 2018-08-17 21:26:17.807 [23781] main/101/interactive I> recovering from `./00000000000000000000.snap' 2018-08-17 21:26:17.849 [23781] main/101/interactive I> recover from `./00000000000000000000.xlog' 2018-08-17 21:26:17.849 [23781] main/101/interactive I> done `./00000000000000000000.xlog' 2018-08-17 21:26:17.850 [23781] main/101/interactive I> recover from `./00000000000000000006.xlog' 2018-08-17 21:26:17.850 [23781] main/101/interactive I> done `./00000000000000000006.xlog' Segmentation fault code: SEGV_MAPERR addr: 0x0 context: 0x10881f4c8 siginfo: 0x10881f460 Current time: 1534530377 Please file a bug at http://github.com/tarantool/tarantool/issues Attempting backtrace... Note: since the server has already crashed, this may fail as well #0 0x105e087ed in print_backtrace+d #1 0x105cbb613 in _ZL12sig_fatal_cbiP9__siginfoPv+1f3 #2 0x7fffcae1eb3a in _sigtramp+1a #3 0x1060690b3 in corruptSchema+93 #4 0x106068f5a in sql_init_callback+25a #5 0x105dbe73a in space_foreach_put_cb+da #6 0x105d86460 in space_foreach+2b0 #7 0x105dbe646 in tarantoolSqlite3LoadSchema+166 #8 0x10606927a in sqlite3InitDatabase+7a #9 0x105dba3de in sql_load_schema+de #10 0x105d99fe7 in _ZL10box_cfg_xcv+7a7 #11 0x105d9973d in box_cfg+d #12 0x105cb9e81 in load_cfg+861 #13 0x105dca8c1 in _ZL13lbox_cfg_loadP9lua_State+11 #14 0x105e3653d in lj_BC_FUNCC+44 #15 0x105e64b90 in lua_pcall+210 #16 0x105dec823 in luaT_call+23 #17 0x105de3761 in lua_main+d1 #18 0x105de3548 in run_script_f+8e8 #19 0x105cbaf0a in _ZL16fiber_cxx_invokePFiP13__va_list_tagES0_+1a #20 0x105e042db in fiber_loop+bb #21 0x106023947 in coro_init+57 Abort trap: 6 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: after table rename properly update indexes 2018-08-17 18:26 ` n.pettik @ 2018-08-20 5:21 ` Kirill Yukhin 0 siblings, 0 replies; 9+ messages in thread From: Kirill Yukhin @ 2018-08-20 5:21 UTC (permalink / raw) To: n.pettik; +Cc: tarantool-patches, Vladislav Shpilevoy Hello Nikita, On 17 авг 21:26, n.pettik wrote: > > >> Hi! Thanks for the fixes! > >> > >>> diff --git a/test-run b/test-run > >>> index ed45e1d..0aa25ae 160000 > >>> --- a/test-run > >>> +++ b/test-run > >>> @@ -1 +1 @@ > >>> -Subproject commit ed45e1dbd36ab9109b84ef7189ef9d7e4b813fb9 > >>> +Subproject commit 0aa25ae8a9d4af977b3c3478cba3ccdc4ef81d35 > >> > >> Test-run is updated accidentally. > > > > I've discarded the hunk & force-pushed the branch. Don't think > > repost is needed here. > > Hello, guys. I found this seagfault on updated branch: This is a separate issue which was uncovered by my patch. So, I'll re-send my change as a patchset of 2 patches: one fising original issue and another, which fixes this one. So, please disregard this patch. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-08-20 5:21 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-13 4:54 [tarantool-patches] [PATCH] sql: after table rename properly update indexes Kirill Yukhin 2018-08-14 22:22 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-15 8:54 ` Kirill Yukhin 2018-08-16 22:24 ` Vladislav Shpilevoy 2018-08-17 5:08 ` Kirill Yukhin 2018-08-17 8:31 ` Vladislav Shpilevoy 2018-08-17 13:48 ` Kirill Yukhin 2018-08-17 18:26 ` n.pettik 2018-08-20 5:21 ` Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox