[tarantool-patches] [PATCH 0/2] sql: update inexes after table rename

[tarantool-patches] [PATCH 0/2] sql: update inexes after table rename

[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

[tarantool-patches] [PATCH 1/2] sql: after table rename properly update indexes

[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

[tarantool-patches] Re: [PATCH 1/2] sql: after table rename properly update indexes

[n.pettik]

> 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.

[tarantool-patches] Re: [PATCH 1/2] sql: after table rename properly update indexes

[Kirill Yukhin]

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')

[tarantool-patches] Re: [PATCH 1/2] sql: after table rename properly update indexes

[n.pettik]

[-- 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 --]

[tarantool-patches] Re: [PATCH 1/2] sql: after table rename properly update indexes

[Kirill Yukhin]

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

[tarantool-patches] [PATCH 2/2] sql: take sql field in index_opts_cmp

[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

[tarantool-patches] Re: [PATCH 2/2] sql: take sql field in index_opts_cmp

[n.pettik]

> 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
> 
> 

[tarantool-patches] Re: [PATCH 2/2] sql: take sql field in index_opts_cmp

[Kirill Yukhin]

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')

[tarantool-patches] Re: [PATCH 2/2] sql: take sql field in index_opts_cmp

[n.pettik]

LGTM.

[tarantool-patches] Re: [PATCH 0/2] sql: update inexes after table rename

[Kirill Yukhin]

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