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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

See 6 comments below and a patch on the branch.

On 13/08/2018 07:54, Kirill Yukhin wrote:
> After table was altered due to rename, indexes were not updated.
> Re-create all table's indexes (in legacy SQL DD), also perform
> udate of corresponding entry in _index for each index to fix table

1. Typo: 'udate'.

> name in create statement.
> 
> Closes #3613
> ---

2. Please, put here the links to the issue and branch.

>   src/box/sql.c                              | 55 +++++++++++++++++++++++++++---
>   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   | 37 ++++++++++++++++++++
>   test/sql/gh-3613-idx-alter-update.test.lua | 21 ++++++++++++
>   6 files changed, 161 insertions(+), 10 deletions(-)
>   create mode 100644 test/sql/gh-3613-idx-alter-update.result
>   create mode 100644 test/sql/gh-3613-idx-alter-update.test.lua
> 

3. Running the tests I caught an assertion:

[010] sql-tap/alter.test.lua                          vinyl           [ fail ]
[010] Test failed! Output from reject file sql-tap/alter.reject:
[010] TAP version 13
[010] 1..41
[010] ok - alter-1.1
[010] ok - alter-1.2
[010]
[010] Last 15 lines of Tarantool Log file [Instance "app_server"][/Users/v.shpilevoy/Work/Repositories/tarantool/test/var/010_sql-tap/alter.test.lua:vinyl.tarantool.log]:
[010] ls: */: No such file or directory
[010] Assertion failed: (iid < space->index_count), function sql_update_index_table_name, file /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql.c, line 851.

Also just failing test:

[016] sql/gh-3613-idx-alter-update.test.lua           vinyl           [ fail ]
[016]
[016] Test failed! Result content mismatch:
[016] --- sql/gh-3613-idx-alter-update.result	Wed Aug 15 00:41:34 2018
[016] +++ sql/gh-3613-idx-alter-update.reject	Wed Aug 15 00:42:19 2018
[016] @@ -32,6 +32,7 @@
[016]  box.sql.execute('DROP INDEX i ON j3')
[016]  ---
[016]  ...
[016] +-- Cleanup
[016]  box.sql.execute('DROP TABLE j3')
[016]  ---
[016]  ...
[016]


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

> +{
> +	assert(space_id != 0);
> +	assert(new_tbl_name != NULL);
> +	struct space *space = space_cache_find(space_id);
> +	assert(space != NULL);
> +	assert(iid < space->index_count);
> +	struct index_def *def = space->index[iid]->def;
> +
> +	bool is_quoted = false;
> +	*sql_stmt = rename_table(db, *sql_stmt, new_tbl_name, &is_quoted);
> +
> +	uint32_t key_len = mp_sizeof_uint(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, 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);
> +
> +	box_tuple_t *res = NULL;

5. You do not need this dummy res. box_update (as all other DML
C API functions) is tolerant to NULL in the last parameter.

> +
> +	if (box_update(BOX_INDEX_ID, 0, key_begin, key, op_begin, op,
> +		       0, &res) != 0) {
> +		diag_set(ClientError, ER_UPDATE_FIELD, 4,
> +			 "while updating SQL field");

6. box_update has already set diag.

> +		return SQL_TARANTOOL_ERROR;
> +	}
> +
> +	return SQLITE_OK;
> +}
> +
>   int
>   tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor,
>   			      struct UnpackedRecord *unpacked)