* [tarantool-patches] [PATCH 0/2] sql: update inexes after table rename @ 2018-08-20 16:49 Kirill Yukhin 2018-08-20 16:49 ` [tarantool-patches] [PATCH 1/2] sql: after table rename properly update indexes Kirill Yukhin ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Kirill Yukhin @ 2018-08-20 16:49 UTC (permalink / raw) To: korablev; +Cc: tarantool-patches, Kirill Yukhin First patch in the patchset fixes indexes update, absense of which caused gh-3613 to fail. After that, another issue was uncovered. It seems that during comparing of index defs, opts.sql field was ignored, which in turn leads to ifnorance of updated indexes from xlog. Second patch takes ops.sql into account while comparing. Issue: https://github.com/tarantool/tarantool/issues/3613 Branch: https://github.com/tarantool/tarantool/commits/kyukhin/gh-3613-update-idx-afer-alter Kirill Yukhin (2): sql: after table rename properly update indexes sql: take sql field in index_opts_cmp src/box/index_def.h | 8 ++++ src/box/sql.c | 57 +++++++++++++++++++++------- src/box/sql/build.c | 7 ++-- src/box/sql/tarantoolInt.h | 23 ++++++++++- src/box/sql/vdbe.c | 26 ++++++++++++- test/sql/gh-3613-idx-alter-update-2.result | 28 ++++++++++++++ test/sql/gh-3613-idx-alter-update-2.test.lua | 16 ++++++++ test/sql/gh-3613-idx-alter-update.result | 38 +++++++++++++++++++ test/sql/gh-3613-idx-alter-update.test.lua | 21 ++++++++++ 9 files changed, 203 insertions(+), 21 deletions(-) create mode 100644 test/sql/gh-3613-idx-alter-update-2.result create mode 100644 test/sql/gh-3613-idx-alter-update-2.test.lua create mode 100644 test/sql/gh-3613-idx-alter-update.result create mode 100644 test/sql/gh-3613-idx-alter-update.test.lua -- 2.16.2 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] [PATCH 1/2] sql: after table rename properly update indexes 2018-08-20 16:49 [tarantool-patches] [PATCH 0/2] sql: update inexes after table rename Kirill Yukhin @ 2018-08-20 16:49 ` Kirill Yukhin 2018-08-21 10:26 ` [tarantool-patches] " n.pettik 2018-08-20 16:49 ` [tarantool-patches] [PATCH 2/2] sql: take sql field in index_opts_cmp Kirill Yukhin 2018-08-22 6:47 ` [tarantool-patches] Re: [PATCH 0/2] sql: update inexes after table rename Kirill Yukhin 2 siblings, 1 reply; 11+ messages in thread From: Kirill Yukhin @ 2018-08-20 16:49 UTC (permalink / raw) To: korablev; +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 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 | 26 ++++++++++++-- test/sql/gh-3613-idx-alter-update.result | 38 ++++++++++++++++++++ test/sql/gh-3613-idx-alter-update.test.lua | 21 +++++++++++ 6 files changed, 151 insertions(+), 21 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..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 dddeb12..05b729d 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -1246,13 +1246,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..4516ee9 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4625,13 +4625,35 @@ 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, + 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') -- 2.16.2 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 1/2] sql: after table rename properly update indexes 2018-08-20 16:49 ` [tarantool-patches] [PATCH 1/2] sql: after table rename properly update indexes Kirill Yukhin @ 2018-08-21 10:26 ` n.pettik 2018-08-21 12:20 ` Kirill Yukhin 0 siblings, 1 reply; 11+ messages in thread From: n.pettik @ 2018-08-21 10:26 UTC (permalink / raw) To: tarantool-patches; +Cc: Kirill Yukhin > 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) I guess name should be like (according to naming convention): sql_index_update_table_name().. > +{ > + assert(new_tbl_name != NULL); > + uint32_t iid = def->iid; Do you really need this shortcut for single (or two) usage(s)? > + > + bool is_quoted = false; > + *sql_stmt = rename_table(db, def->opts.sql, new_tbl_name, &is_quoted); You need here check: rename_table() may fail and return NULL. > + > + 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”); In diag_set you pass key_len, but allocate key_len + op_sz. > + 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); Mb it would be better to use BOX_INDEX_FIELD_OPTS? At least it would be easier to understand what happen here. > + 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; AFAIR we decided to avoid using SQLITE_OK macros. Lets use instead simple return 0/-1. > +} > + > 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) > { You can pass here struct index_def (or struct index_opts) instead of two args: is_unique ---> def->opst.is_unique and zSql ---> def->opts.sql. I see that you pass new zSql, but you still can reassign def->opts.sql. It is up to you, thou. > 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 dddeb12..05b729d 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1246,13 +1246,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 Nitpicking: put dot at the end of sentence and start sentence with capital letter. > + * @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. You didn’t specify @retval for this function. > /** > * Extract next id from _sequence space. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index dc5146f..4516ee9 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -4625,13 +4625,35 @@ 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; Again the same problem with RENAME: in case of fail part of indexes would remain with old ‘create index …’ statement and after server’s restart it is likely to result in segfault/assertion. > + space = space_by_id(space_id); Does opts.sql update can lead to renewal of space ptr? If so, it looks quite weird: string with ‘CREATE INDEX …’ statement seems to be sort of static attribute which may be useful only during instance restart... > + 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.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)’) Well, actually I would also add test when instance is restarted right after dropping to make sure that no artefacts are left after rename. Recreation may be misleading in this case. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 1/2] sql: after table rename properly update indexes 2018-08-21 10:26 ` [tarantool-patches] " n.pettik @ 2018-08-21 12:20 ` Kirill Yukhin 2018-08-21 20:38 ` n.pettik 0 siblings, 1 reply; 11+ messages in thread From: Kirill Yukhin @ 2018-08-21 12:20 UTC (permalink / raw) To: n.pettik; +Cc: tarantool-patches Hello Nikita, Thanks for review. My ansewers inlined, updated patch in the bottom. Branch force-pushed. On 21 авг 13:26, n.pettik wrote: > > > 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) > > I guess name should be like (according to naming convention): > sql_index_update_table_name().. Renamed. > > +{ > > + assert(new_tbl_name != NULL); > > + uint32_t iid = def->iid; > > Do you really need this shortcut for single (or two) usage(s)? Removed. > > + > > + bool is_quoted = false; > > + *sql_stmt = rename_table(db, def->opts.sql, new_tbl_name, &is_quoted); > > You need here check: rename_table() may fail and return NULL. Added. > > + > > + 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”); > > In diag_set you pass key_len, but allocate key_len + op_sz. Added op_sz. > > + 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); > > Mb it would be better to use BOX_INDEX_FIELD_OPTS? > At least it would be easier to understand what happen here. Used. > > + 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; > > AFAIR we decided to avoid using SQLITE_OK macros. > Lets use instead simple return 0/-1. Okay, let it be 0. > > +} > > + > > 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) > > { > > You can pass here struct index_def (or struct index_opts) > instead of two args: > is_unique ---> def->opst.is_unique and zSql ---> def->opts.sql. > I see that you pass new zSql, but you still can reassign def->opts.sql. > It is up to you, thou. Refactored. > > 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 dddeb12..05b729d 100644 > > --- a/src/box/sql/build.c > > +++ b/src/box/sql/build.c > > @@ -1246,13 +1246,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 > > Nitpicking: put dot at the end of sentence and start sentence with capital letter. > > > + * @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. > > You didn’t specify @retval for this function. Added. > > /** > > * Extract next id from _sequence space. > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > > index dc5146f..4516ee9 100644 > > --- a/src/box/sql/vdbe.c > > +++ b/src/box/sql/vdbe.c > > @@ -4625,13 +4625,35 @@ 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; > > Again the same problem with RENAME: in case of fail part of indexes would > remain with old ‘create index …’ statement and after server’s restart it is likely > to result in segfault/assertion. Yes, w/o transactional DDL we can end up w/ inconsistent state. So, any alter in SQL is a dangerous zone. > > + space = space_by_id(space_id); > > Does opts.sql update can lead to renewal of space ptr? > If so, it looks quite weird: string with ‘CREATE INDEX …’ statement > seems to be sort of static attribute which may be useful only during > instance restart... It is state of the things right now. Index update, even in SQL opt, re-creates space. We can try to somwhat optimizw alter, but I don't think in that patch. > > + 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.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)’) > > Well, actually I would also add test when instance is restarted > right after dropping to make sure that no artefacts are left after rename. > Recreation may be misleading in this case. Added. -- Regards, Kirill Yukhin commit 31aae70d08fe3e799641a382c8c25ad92c6ee995 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 | 62 ++++++++++++++++++++++-------- src/box/sql/build.c | 7 ++-- src/box/sql/tarantoolInt.h | 25 ++++++++++-- src/box/sql/vdbe.c | 26 ++++++++++++- test/sql/gh-3613-idx-alter-update.result | 48 +++++++++++++++++++++++ test/sql/gh-3613-idx-alter-update.test.lua | 26 +++++++++++++ 6 files changed, 170 insertions(+), 24 deletions(-) diff --git a/src/box/sql.c b/src/box/sql.c index ae12cae..789681e 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -840,6 +840,49 @@ rename_fail: return SQL_TARANTOOL_ERROR; } +int +sql_index_update_table_name(struct index_def *def, const char *new_tbl_name, + char **sql_stmt) +{ + assert(new_tbl_name != NULL); + + bool is_quoted = false; + *sql_stmt = rename_table(db, def->opts.sql, new_tbl_name, &is_quoted); + if (*sql_stmt == NULL) + return -1; + + uint32_t key_len = mp_sizeof_uint(def->space_id) + + mp_sizeof_uint(def->iid) + mp_sizeof_array(2); + struct index_opts opts = def->opts; + opts.sql = *sql_stmt; + uint32_t new_opts_sz = sql_encode_index_opts(&opts, 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 + op_sz, "region_alloc", + "key_begin"); + return -1; + } + char *key = mp_encode_array(key_begin, 2); + key = mp_encode_uint(key, def->space_id); + key = mp_encode_uint(key, def->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, BOX_INDEX_FIELD_OPTS); + op += sql_encode_index_opts(&opts, op); + + if (box_update(BOX_INDEX_ID, 0, key_begin, key, op_begin, op, + 0, NULL) != 0) + return -1; + + return 0; +} + int tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor, struct UnpackedRecord *unpacked) @@ -1484,23 +1527,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 +sql_encode_index_opts(struct index_opts *opts, 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,9 +1543,9 @@ 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, opts->is_unique); p = enc->encode_str(p, "sql", 3); - p = enc->encode_str(p, zSql, zSql ? strlen(zSql) : 0); + p = enc->encode_str(p, opts->sql, opts->sql ? strlen(opts->sql) : 0); return (int)(p - base); } diff --git a/src/box/sql/build.c b/src/box/sql/build.c index dddeb12..da7668b 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -1245,14 +1245,15 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId, int zOptsSz, zPartsSz; /* Format "opts" and "parts" for _index entry. */ + struct index_opts opts = pIndex->def->opts; + opts.sql = (char *)zSql; zOpts = sqlite3DbMallocRaw(pParse->db, - tarantoolSqlite3MakeIdxOpts(pIndex, zSql, - NULL) + + sql_encode_index_opts(&opts, NULL) + tarantoolSqlite3MakeIdxParts(pIndex, NULL) + 2); if (!zOpts) return; - zOptsSz = tarantoolSqlite3MakeIdxOpts(pIndex, zSql, zOpts); + zOptsSz = sql_encode_index_opts(&opts, 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..a3ad52a 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 0 on success, -1 otherwise. + */ +int +sql_index_update_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,15 @@ 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 opts Options to encode. + * @param[out] buf destination buffer. + * @retval Result size or size estimation if buf == NULL. */ -int tarantoolSqlite3MakeIdxOpts(Index * index, const char *zSql, void *buf); +int +sql_encode_index_opts(struct index_opts *opts, void *buf); /** * Extract next id from _sequence space. diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index dc5146f..821c7a9 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4625,13 +4625,35 @@ 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_index_update_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, + 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..ac62b5f --- /dev/null +++ b/test/sql/gh-3613-idx-alter-update.result @@ -0,0 +1,48 @@ +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') +--- +... +-- Make sure that no artifacts remain after restart. +box.snapshot() +--- +- ok +... +test_run:cmd('restart server default') +box.sql.execute('DROP INDEX i ON j3') +--- +- error: 'no such index: J3.I' +... +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..49b81d7 --- /dev/null +++ b/test/sql/gh-3613-idx-alter-update.test.lua @@ -0,0 +1,26 @@ +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') + +-- Make sure that no artifacts remain after restart. +box.snapshot() +test_run:cmd('restart server default') +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] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 1/2] sql: after table rename properly update indexes 2018-08-21 12:20 ` Kirill Yukhin @ 2018-08-21 20:38 ` n.pettik 2018-08-22 6:39 ` Kirill Yukhin 0 siblings, 1 reply; 11+ messages in thread From: n.pettik @ 2018-08-21 20:38 UTC (permalink / raw) To: tarantool-patches; +Cc: Kirill Yukhin [-- Attachment #1: Type: text/plain, Size: 1289 bytes --] > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index dc5146f..821c7a9 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -4625,13 +4625,35 @@ 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_index_update_table_name(def, zNewTableName, &sql_stmt); Where do you free memory for sql_stmt? table_rename allocates new string using sqlite3DbMalloc AFAIK. > + if (rc != SQLITE_OK) rc != 0 (or better: if (sql_index_update_table_name() != 0)). > + 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) { rc != 0 (or better: if (init.rc != 0) The rest seems to be ok. [-- Attachment #2: Type: text/html, Size: 41498 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 1/2] sql: after table rename properly update indexes 2018-08-21 20:38 ` n.pettik @ 2018-08-22 6:39 ` Kirill Yukhin 0 siblings, 0 replies; 11+ messages in thread From: Kirill Yukhin @ 2018-08-22 6:39 UTC (permalink / raw) To: n.pettik; +Cc: tarantool-patches Hello Nikita, My answers are inlined. Updated patch in the bottom. Branch force-pushed. On 21 авг 23:38, n.pettik wrote: > > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > > index dc5146f..821c7a9 100644 > > --- a/src/box/sql/vdbe.c > > +++ b/src/box/sql/vdbe.c > > @@ -4625,13 +4625,35 @@ 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_index_update_table_name(def, zNewTableName, &sql_stmt); > > Where do you free memory for sql_stmt? table_rename allocates new > string using sqlite3DbMalloc AFAIK. Yes, it is. This is a clear leak. Fixed. > > + if (rc != SQLITE_OK) > > rc != 0 (or better: if (sql_index_update_table_name() != 0)). > > > + 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) { > > rc != 0 (or better: if (init.rc != 0) Fixed. Here's iterative diff: 1 file changed, 4 insertions(+), 3 deletions(-) src/box/sql/vdbe.c | 7 ++++--- modified src/box/sql/vdbe.c @@ -4663,14 +4663,15 @@ case OP_RenameTable: { continue; char *sql_stmt; rc = sql_index_update_table_name(def, zNewTableName, &sql_stmt); - if (rc != SQLITE_OK) + if (rc != 0) 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) { + sqlite3DbFree(db, sql_stmt); + if (init.rc != 0) { sqlite3CommitInternalChanges(); + rc = init.rc; db->init.busy = 0; goto abort_due_to_error; } > > The rest seems to be ok. > Thanks! I'll check updated patch into 2.0 branch. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] [PATCH 2/2] sql: take sql field in index_opts_cmp 2018-08-20 16:49 [tarantool-patches] [PATCH 0/2] sql: update inexes after table rename Kirill Yukhin 2018-08-20 16:49 ` [tarantool-patches] [PATCH 1/2] sql: after table rename properly update indexes Kirill Yukhin @ 2018-08-20 16:49 ` Kirill Yukhin 2018-08-21 10:26 ` [tarantool-patches] " n.pettik 2018-08-22 6:47 ` [tarantool-patches] Re: [PATCH 0/2] sql: update inexes after table rename Kirill Yukhin 2 siblings, 1 reply; 11+ messages in thread From: Kirill Yukhin @ 2018-08-20 16:49 UTC (permalink / raw) To: korablev; +Cc: tarantool-patches, Kirill Yukhin After gh-3613 is imlemented a hidden bug was uncovered: during, e.g. recovery opts.sql field didn't take into account marking corresponding xlog entry useless. Fix that by comparing mentioned field of new and old entries. Follow up of #3613 --- src/box/index_def.h | 8 ++++++++ test/sql/gh-3613-idx-alter-update-2.result | 28 ++++++++++++++++++++++++++++ test/sql/gh-3613-idx-alter-update-2.test.lua | 16 ++++++++++++++++ 3 files changed, 52 insertions(+) create mode 100644 test/sql/gh-3613-idx-alter-update-2.result create mode 100644 test/sql/gh-3613-idx-alter-update-2.test.lua diff --git a/src/box/index_def.h b/src/box/index_def.h index 48a7820..8960e3d 100644 --- a/src/box/index_def.h +++ b/src/box/index_def.h @@ -212,6 +212,14 @@ index_opts_cmp(const struct index_opts *o1, const struct index_opts *o2) return o1->run_size_ratio < o2->run_size_ratio ? -1 : 1; if (o1->bloom_fpr != o2->bloom_fpr) return o1->bloom_fpr < o2->bloom_fpr ? -1 : 1; + + if ((o1->sql == NULL) != (o2->sql == NULL)) + return 1; + if ((o1->sql != NULL) && (o2->sql != NULL)) { + int rc = strcmp(o1->sql, o2->sql); + if (rc != 0) + return rc; + } return 0; } diff --git a/test/sql/gh-3613-idx-alter-update-2.result b/test/sql/gh-3613-idx-alter-update-2.result new file mode 100644 index 0000000..234336c --- /dev/null +++ b/test/sql/gh-3613-idx-alter-update-2.result @@ -0,0 +1,28 @@ +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') +--- +... +-- After gh-3613 fix, bug in cmp_def was discovered. +-- Comparison didn't take .opts.sql into account. +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-2.test.lua b/test/sql/gh-3613-idx-alter-update-2.test.lua new file mode 100644 index 0000000..e2beb7a --- /dev/null +++ b/test/sql/gh-3613-idx-alter-update-2.test.lua @@ -0,0 +1,16 @@ +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') + +-- After gh-3613 fix, bug in cmp_def was discovered. +-- Comparison didn't take .opts.sql into account. +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] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: take sql field in index_opts_cmp 2018-08-20 16:49 ` [tarantool-patches] [PATCH 2/2] sql: take sql field in index_opts_cmp Kirill Yukhin @ 2018-08-21 10:26 ` n.pettik 2018-08-21 12:30 ` Kirill Yukhin 0 siblings, 1 reply; 11+ messages in thread From: n.pettik @ 2018-08-21 10:26 UTC (permalink / raw) To: tarantool-patches; +Cc: Kirill Yukhin > diff --git a/src/box/index_def.h b/src/box/index_def.h > index 48a7820..8960e3d 100644 > --- a/src/box/index_def.h > +++ b/src/box/index_def.h > @@ -212,6 +212,14 @@ index_opts_cmp(const struct index_opts *o1, const struct index_opts *o2) > return o1->run_size_ratio < o2->run_size_ratio ? -1 : 1; > if (o1->bloom_fpr != o2->bloom_fpr) > return o1->bloom_fpr < o2->bloom_fpr ? -1 : 1; > + Nitpicking: don’t put extra new line: it breaks whole function look. > + if ((o1->sql == NULL) != (o2->sql == NULL)) > + return 1; > + if ((o1->sql != NULL) && (o2->sql != NULL)) { Nitpicking: after previous check o1->sql and o2->sql are both NULL or not. So I guess only one check is enough: if (o1->sql != NULL). > + int rc = strcmp(o1->sql, o2->sql); > + if (rc != 0) > + return rc; Why not simply return strcmp()? > + } > return 0; > } > > diff --git a/test/sql/gh-3613-idx-alter-update-2.test.lua b/test/sql/gh-3613-idx-alter-update-2.test.lua > new file mode 100644 > index 0000000..e2beb7a > --- /dev/null > +++ b/test/sql/gh-3613-idx-alter-update-2.test.lua Can you merge this test file with previous one? I strongly dislike the fact that such simple tests are put in separate files. Even though we already have sql-tap/alter.test.lua. > @@ -0,0 +1,16 @@ > +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') > + > +-- After gh-3613 fix, bug in cmp_def was discovered. > +-- Comparison didn't take .opts.sql into account. > +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] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: take sql field in index_opts_cmp 2018-08-21 10:26 ` [tarantool-patches] " n.pettik @ 2018-08-21 12:30 ` Kirill Yukhin 2018-08-21 20:41 ` n.pettik 0 siblings, 1 reply; 11+ messages in thread From: Kirill Yukhin @ 2018-08-21 12:30 UTC (permalink / raw) To: n.pettik; +Cc: tarantool-patches Hello Nikita, Thanks for review! My answers inlined, updated patch in the bottom of the messaga, branch force-pushed. On 21 авг 13:26, n.pettik wrote: > > diff --git a/src/box/index_def.h b/src/box/index_def.h > > index 48a7820..8960e3d 100644 > > --- a/src/box/index_def.h > > +++ b/src/box/index_def.h > > @@ -212,6 +212,14 @@ index_opts_cmp(const struct index_opts *o1, const struct index_opts *o2) > > return o1->run_size_ratio < o2->run_size_ratio ? -1 : 1; > > if (o1->bloom_fpr != o2->bloom_fpr) > > return o1->bloom_fpr < o2->bloom_fpr ? -1 : 1; > > + > > Nitpicking: don’t put extra new line: it breaks whole function look. Removed. > > + if ((o1->sql == NULL) != (o2->sql == NULL)) > > + return 1; > > + if ((o1->sql != NULL) && (o2->sql != NULL)) { > > Nitpicking: after previous check o1->sql and o2->sql are both NULL or not. > So I guess only one check is enough: if (o1->sql != NULL). Good catch. Fixed. > > + int rc = strcmp(o1->sql, o2->sql); > > + if (rc != 0) > > + return rc; > > Why not simply return strcmp()? Done. > > + } > > return 0; > > } > > > > diff --git a/test/sql/gh-3613-idx-alter-update-2.test.lua b/test/sql/gh-3613-idx-alter-update-2.test.lua > > new file mode 100644 > > index 0000000..e2beb7a > > --- /dev/null > > +++ b/test/sql/gh-3613-idx-alter-update-2.test.lua > > Can you merge this test file with previous one? > I strongly dislike the fact that such simple tests are put in > separate files. Even though we already have sql-tap/alter.test.lua. No. There's no reason for economy in number of files. There're tons of pros when each and every regression test lives in separate file, or even files. I'll update SOP stating that. -- Regards, Kirill Yukhin commit a2c7fb02f330819d31591273dbf9ec9e60a5f5df Author: Kirill Yukhin <kyukhin@tarantool.org> Date: Mon Aug 20 19:38:06 2018 +0300 sql: take sql field in index_opts_cmp After gh-3613 is imlemented a hidden bug was uncovered: during, e.g. recovery opts.sql field didn't take into account marking corresponding xlog entry useless. Fix that by comparing mentioned field of new and old entries. Follow up of #3613 --- src/box/index_def.h | 4 ++++ test/sql/gh-3613-idx-alter-update-2.result | 28 ++++++++++++++++++++++++++++ test/sql/gh-3613-idx-alter-update-2.test.lua | 16 ++++++++++++++++ 3 files changed, 48 insertions(+) diff --git a/src/box/index_def.h b/src/box/index_def.h index 48a7820..273b8cb 100644 --- a/src/box/index_def.h +++ b/src/box/index_def.h @@ -212,6 +212,10 @@ index_opts_cmp(const struct index_opts *o1, const struct index_opts *o2) return o1->run_size_ratio < o2->run_size_ratio ? -1 : 1; if (o1->bloom_fpr != o2->bloom_fpr) return o1->bloom_fpr < o2->bloom_fpr ? -1 : 1; + if ((o1->sql == NULL) != (o2->sql == NULL)) + return 1; + if (o1->sql != NULL) + return strcmp(o1->sql, o2->sql); return 0; } diff --git a/test/sql/gh-3613-idx-alter-update-2.result b/test/sql/gh-3613-idx-alter-update-2.result new file mode 100644 index 0000000..234336c --- /dev/null +++ b/test/sql/gh-3613-idx-alter-update-2.result @@ -0,0 +1,28 @@ +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') +--- +... +-- After gh-3613 fix, bug in cmp_def was discovered. +-- Comparison didn't take .opts.sql into account. +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-2.test.lua b/test/sql/gh-3613-idx-alter-update-2.test.lua new file mode 100644 index 0000000..e2beb7a --- /dev/null +++ b/test/sql/gh-3613-idx-alter-update-2.test.lua @@ -0,0 +1,16 @@ +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') + +-- After gh-3613 fix, bug in cmp_def was discovered. +-- Comparison didn't take .opts.sql into account. +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] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: take sql field in index_opts_cmp 2018-08-21 12:30 ` Kirill Yukhin @ 2018-08-21 20:41 ` n.pettik 0 siblings, 0 replies; 11+ messages in thread From: n.pettik @ 2018-08-21 20:41 UTC (permalink / raw) To: tarantool-patches; +Cc: Kirill Yukhin LGTM. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 0/2] sql: update inexes after table rename 2018-08-20 16:49 [tarantool-patches] [PATCH 0/2] sql: update inexes after table rename Kirill Yukhin 2018-08-20 16:49 ` [tarantool-patches] [PATCH 1/2] sql: after table rename properly update indexes Kirill Yukhin 2018-08-20 16:49 ` [tarantool-patches] [PATCH 2/2] sql: take sql field in index_opts_cmp Kirill Yukhin @ 2018-08-22 6:47 ` Kirill Yukhin 2 siblings, 0 replies; 11+ messages in thread From: Kirill Yukhin @ 2018-08-22 6:47 UTC (permalink / raw) To: korablev; +Cc: tarantool-patches Hello, On 20 авг 19:49, Kirill Yukhin wrote: > First patch in the patchset fixes indexes update, absense of which caused > gh-3613 to fail. After that, another issue was uncovered. It seems that > during comparing of index defs, opts.sql field was ignored, which in turn > leads to ifnorance of updated indexes from xlog. Second patch takes ops.sql > into account while comparing. > > Issue: https://github.com/tarantool/tarantool/issues/3613 > Branch: https://github.com/tarantool/tarantool/commits/kyukhin/gh-3613-update-idx-afer-alter I've pushed the patchset in to 2.0 branch. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-08-22 6:47 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-20 16:49 [tarantool-patches] [PATCH 0/2] sql: update inexes after table rename Kirill Yukhin 2018-08-20 16:49 ` [tarantool-patches] [PATCH 1/2] sql: after table rename properly update indexes Kirill Yukhin 2018-08-21 10:26 ` [tarantool-patches] " n.pettik 2018-08-21 12:20 ` Kirill Yukhin 2018-08-21 20:38 ` n.pettik 2018-08-22 6:39 ` Kirill Yukhin 2018-08-20 16:49 ` [tarantool-patches] [PATCH 2/2] sql: take sql field in index_opts_cmp Kirill Yukhin 2018-08-21 10:26 ` [tarantool-patches] " n.pettik 2018-08-21 12:30 ` Kirill Yukhin 2018-08-21 20:41 ` n.pettik 2018-08-22 6:47 ` [tarantool-patches] Re: [PATCH 0/2] sql: update inexes after table rename Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox