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