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

[Vladislav Shpilevoy]

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.

====================================================================

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