[tarantool-patches] Re: [PATCH 1/2] sql: after table rename properly update indexes
Kirill Yukhin
kyukhin at tarantool.org
Tue Aug 21 15:20:38 MSK 2018
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 at 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')
More information about the Tarantool-patches
mailing list