[tarantool-patches] Re: [PATCH 2/6] sql: don't update SQL string during renaming

[n.pettik]

> On 12 Dec 2018, at 15:36, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
> On 11/12/2018 21:29, n.pettik wrote:
>>> On 10 Dec 2018, at 17:16, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
>>> Thanks for the patch!
>>> On 10/12/2018 00:30, Nikita Pettik wrote:
>>>> Since SQL string containing "CREATE TABLE ..." statement is not used
>>>> anymore for ordinary tables/space, it makes no sense to modify it during
>>>> renaming. Hence, now rename routine needs only to update name in _space,
>>>> so it can be done using simple update operation.
>>>> Moreover, now we are able to rename spaces created from Lua-land.
>>>> Part of #2647
>>>> ---
>>>>  src/box/sql.c               | 187 ++++++++------------------------------------
>>>>  src/box/sql/tarantoolInt.h  |   3 +-
>>>>  src/box/sql/vdbe.c          |   4 +-
>>>>  test/sql-tap/alter.test.lua |  32 +++++++-
>>>>  4 files changed, 63 insertions(+), 163 deletions(-)
>>> 
>>> You forgot to remove some code and comments. My
>>> review fixes here and on the branch.
>> Thx, applied.
> 
> Sorry, I've found another space for optimizations. See my
> new review fixes below and on the branch. This time I've
> removed mpstream from sql_rename_table(). It'd used mpstream
> only because of mpstream_encode_index_opts, which was removed
> from there by your commit.
> 
> After these changes are applied, the patchset is LGTM.
> 

Ok, I’ve applied your fixes. Nevertheless, I don’t think that such
nano “optimizations” worth it IMHO. At least due to the fact that
rename is assumed to be called rarely, so it is not hot path.

> ====================================================================
> 
> commit 1d280e440cb3049bd45bfffa4243d8d1fb5d1c3d
> Author: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> Date:   Wed Dec 12 15:27:12 2018 +0300
> 
>    Review fix
> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 0a10f4d8c..18e8f2507 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -709,37 +709,27 @@ sql_rename_table(uint32_t space_id, const char *new_name)
> {
> 	assert(space_id != 0);
> 	assert(new_name != NULL);
> +	int name_len = strlen(new_name);
> 	struct region *region = &fiber()->gc;
> -	size_t used = region_used(region);
> -	struct mpstream stream;
> -	bool is_error = false;
> -	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
> -		      set_encode_error, &is_error);
> -	/* Encode key. */
> -	mpstream_encode_array(&stream, 1);
> -	mpstream_encode_uint(&stream, space_id);
> -
> -	/* Encode op and new name. */
> -	uint32_t op_offset = stream.pos - stream.buf;
> -	mpstream_encode_array(&stream, 1);
> -	mpstream_encode_array(&stream, 3);
> -	mpstream_encode_str(&stream, "=");
> -	mpstream_encode_uint(&stream, BOX_SPACE_FIELD_NAME);
> -	mpstream_encode_str(&stream, new_name);
> -	mpstream_flush(&stream);
> -	if (is_error) {
> -		diag_set(OutOfMemory, stream.pos - stream.buf,
> -			 "mpstream_flush", "stream");
> -		return SQL_TARANTOOL_ERROR;
> -	}
> -	size_t sz = region_used(region) - used;
> -	char *raw = region_join(region, sz);
> +	/* 32 + name_len is enough to encode one update op. */
> +	size_t size = 32 + name_len;
> +	char *raw = (char *) region_alloc(region, size);
> 	if (raw == NULL) {
> -		diag_set(OutOfMemory, sz, "region_join", "raw");
> +		diag_set(OutOfMemory, size, "region_alloc", "raw");
> 		return SQL_TARANTOOL_ERROR;
> 	}
> -	if (box_update(BOX_SPACE_ID, 0, raw, raw + op_offset, raw + op_offset,
> -		       raw + sz, 0, NULL) != 0)
> +	/* Encode key. */
> +	char *pos = mp_encode_array(raw, 1);
> +	pos = mp_encode_uint(pos, space_id);
> +
> +	/* Encode op and new name. */
> +	char *ops = pos;
> +	pos = mp_encode_array(pos, 1);
> +	pos = mp_encode_array(pos, 3);
> +	pos = mp_encode_str(pos, "=", 1);
> +	pos = mp_encode_uint(pos, BOX_SPACE_FIELD_NAME);
> +	pos = mp_encode_str(pos, new_name, name_len);
> +	if (box_update(BOX_SPACE_ID, 0, raw, ops, ops, pos, 0, NULL) != 0)
> 		return SQL_TARANTOOL_ERROR;
> 	return 0;
> }