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

[Vladislav Shpilevoy]

Hi! Thanks for the fixes! Unfortunately you have ignored some of
my fixes pushed on the branch, so I pushed them again on a
separate commit. Please, be careful doing a force push.

>>> diff --git a/src/box/sql.c b/src/box/sql.c
>>> index ae12cae..e2d3cc1 100644
>>> --- a/src/box/sql.c
>>> +++ b/src/box/sql.c
>>> @@ -840,6 +840,55 @@ rename_fail:
>>>    	return SQL_TARANTOOL_ERROR;
>>>    }
>>> +int
>>> +sql_update_index_table_name(uint32_t space_id, uint32_t iid,
>>> +			    const char *new_tbl_name, char **sql_stmt)
>>
>> 4. I think, it would be enough to pass index_def (that you have
>> on the stack above). It has both index id and space id. And here
>> you use space_cache_find and struct space only to get index_def.
> Done. This fix cures assert in p 3

1. Unfortunately, not done.

> Regards, Kirill Yukhin
> 
> commit 4cf29331f3730658351701122d8714e157e0d77b
> 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                              | 49 +++++++++++++++++++++++++++---
>   src/box/sql/build.c                        |  6 ++--
>   src/box/sql/tarantoolInt.h                 | 25 +++++++++++++--
>   src/box/sql/vdbe.c                         | 27 ++++++++++++++--
>   test/sql/gh-3613-idx-alter-update.result   | 38 +++++++++++++++++++++++
>   test/sql/gh-3613-idx-alter-update.test.lua | 21 +++++++++++++
>   6 files changed, 156 insertions(+), 10 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index cdf2bfc..6247f18 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1245,13 +1245,15 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId,
>   
>   	/* Format "opts" and "parts" for _index entry. */
>   	zOpts = sqlite3DbMallocRaw(pParse->db,
> -				   tarantoolSqlite3MakeIdxOpts(pIndex, zSql,
> +				   tarantoolSqlite3MakeIdxOpts(IsUniqueIndex(pIndex),
> +							       zSql,
>   							       NULL) +

2. Looks like you has ignored my fixes here and in most other
places. Why?

>   				   tarantoolSqlite3MakeIdxParts(pIndex,
>   								NULL) + 2);
>   	if (!zOpts)
> diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
> index 94517f6..234c94b 100644
> --- a/src/box/sql/tarantoolInt.h
> +++ b/src/box/sql/tarantoolInt.h
> @@ -89,6 +89,23 @@ 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 space_id Table's space identifier.
> + * @param idef Index definition.
> + * @param new_tbl_name new name of table
> + * @param[out] sql_stmt CREATE INDEX statement for new name table,
> + * can be NULL.
> + *
> + * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise.
> + */
> +int
> +sql_update_index_table_name(uint32_t space_id, struct index_def *idef,
> +			    const char *new_tbl_name, char **sql_stmt);
> +
> +

3. Redundant empty line.

>   /* Alter trigger statement after rename table. */
>   int tarantoolSqlite3RenameTrigger(const char *zTriggerName,
>   				  const char *zOldName, const char *zNewName);
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index dc5146f..1166ddb 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4625,13 +4625,36 @@ 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) {
> +		char *sql_stmt = space->index[i]->def->opts.sql;
> +		if (sql_stmt == NULL)
> +			continue;
> +		rc = sql_update_index_table_name(space_id, space->index[i]->def,
> +						 zNewTableName, &sql_stmt);
> +		if (rc != SQLITE_OK)
> +			goto abort_due_to_error;
> +
> +		space = space_by_id(space_id);

4. The same space is looked up before the cycle.

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