From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 3BEA5280B2 for ; Tue, 21 Aug 2018 08:20:41 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id lBUa6UZdF3uN for ; Tue, 21 Aug 2018 08:20:41 -0400 (EDT) Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id C307D277D0 for ; Tue, 21 Aug 2018 08:20:40 -0400 (EDT) Date: Tue, 21 Aug 2018 15:20:38 +0300 From: Kirill Yukhin Subject: [tarantool-patches] Re: [PATCH 1/2] sql: after table rename properly update indexes Message-ID: <20180821122038.zlyjqxk5nzpj755l@tarantool.org> References: <8A23ACEC-DCAA-47A2-8311-F46D75C9F9C0@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <8A23ACEC-DCAA-47A2-8311-F46D75C9F9C0@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" Cc: tarantool-patches@freelists.org 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 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')