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

[Nikita Pettik <korablev@tarantool.org>]

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(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index 7b41c9926..e9fa89df9 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -697,119 +697,6 @@ rename_fail:
 	return SQL_TARANTOOL_ERROR;
 }
 
-int
-sql_rename_table(uint32_t space_id, const char *new_name, char **sql_stmt)
-{
-	assert(space_id != 0);
-	assert(new_name != NULL);
-	assert(sql_stmt != NULL);
-
-	box_tuple_t *tuple;
-	uint32_t key_len = mp_sizeof_uint(space_id) + mp_sizeof_array(1);
-	char *key_begin = (char*) region_alloc(&fiber()->gc, key_len);
-	if (key_begin == NULL) {
-		diag_set(OutOfMemory, key_len, "region_alloc", "key_begin");
-		return SQL_TARANTOOL_ERROR;
-	}
-	char *key = mp_encode_array(key_begin, 1);
-	key = mp_encode_uint(key, space_id);
-	if (box_index_get(BOX_SPACE_ID, 0, key_begin, key, &tuple) != 0)
-		return SQL_TARANTOOL_ERROR;
-	assert(tuple != NULL);
-
-	/* Code below relies on format of _space. If number of fields or their
-	 * order will ever change, this code should be changed too.
-	 */
-	assert(tuple_field_count(tuple) == 7);
-	const char *sql_stmt_map = box_tuple_field(tuple, 5);
-
-	if (sql_stmt_map == NULL || mp_typeof(*sql_stmt_map) != MP_MAP)
-		goto rename_fail;
-	uint32_t map_size = mp_decode_map(&sql_stmt_map);
-	if (map_size != 1)
-		goto rename_fail;
-	const char *sql_str = mp_decode_str(&sql_stmt_map, &key_len);
-
-	/* If this table hasn't been created via SQL facilities,
-	 * we can't do anything yet.
-	 */
-	if (sqlite3StrNICmp(sql_str, "sql", 3) != 0)
-		goto rename_fail;
-	uint32_t sql_stmt_decoded_len;
-	const char *sql_stmt_old = mp_decode_str(&sql_stmt_map,
-						 &sql_stmt_decoded_len);
-	uint32_t old_name_len;
-	const char *old_name = box_tuple_field(tuple, 2);
-	old_name = mp_decode_str(&old_name, &old_name_len);
-	uint32_t new_name_len = strlen(new_name);
-
-	*sql_stmt = (char*)region_alloc(&fiber()->gc, sql_stmt_decoded_len + 1);
-	if (*sql_stmt == NULL) {
-		diag_set(OutOfMemory, sql_stmt_decoded_len + 1, "region_alloc",
-			 "sql_stmt");
-		return SQL_TARANTOOL_ERROR;
-	}
-	memcpy(*sql_stmt, sql_stmt_old, sql_stmt_decoded_len);
-	*(*sql_stmt + sql_stmt_decoded_len) = '\0';
-	bool is_quoted = false;
-	*sql_stmt = rename_table(db, *sql_stmt, new_name, &is_quoted);
-	if (*sql_stmt == NULL)
-		goto rename_fail;
-
-	/* If old table name isn't quoted, then need to reserve space for quotes. */
-	uint32_t  sql_stmt_len = sql_stmt_decoded_len +
-				 new_name_len - old_name_len +
-				 2 * (!is_quoted);
-
-	assert(sql_stmt_len > 0);
-	/* Construct new msgpack to insert to _space.
-	 * Since we have changed only name of table and create statement,
-	 * there is no need to decode/encode other fields of tuple,
-	 * just memcpy constant parts.
-	 */
-	char *new_tuple = (char*)region_alloc(&fiber()->gc, tuple->bsize +
-					      mp_sizeof_str(sql_stmt_len));
-	if (new_tuple == NULL) {
-		free(*sql_stmt);
-		*sql_stmt = NULL;
-		diag_set(OutOfMemory,
-			 tuple->bsize + mp_sizeof_str(sql_stmt_len),
-			 "region_alloc", "new_tuple");
-		return SQL_TARANTOOL_ERROR;
-	}
-
-	char *new_tuple_end = new_tuple;
-	const char *data_begin = tuple_data(tuple);
-	const char *data_end = tuple_field(tuple, 2);
-	uint32_t data_size = data_end - data_begin;
-	memcpy(new_tuple, data_begin, data_size);
-	new_tuple_end += data_size;
-	new_tuple_end = mp_encode_str(new_tuple_end, new_name, new_name_len);
-	data_begin = tuple_field(tuple, 3);
-	data_end = tuple_field(tuple, 5);
-	data_size = data_end - data_begin;
-	memcpy(new_tuple_end, data_begin, data_size);
-	new_tuple_end += data_size;
-	new_tuple_end = mp_encode_map(new_tuple_end, 1);
-	new_tuple_end = mp_encode_str(new_tuple_end, "sql", 3);
-	new_tuple_end = mp_encode_str(new_tuple_end, *sql_stmt, sql_stmt_len);
-	data_begin = tuple_field(tuple, 6);
-	data_end = (char*) tuple + tuple_size(tuple);
-	data_size = data_end - data_begin;
-	memcpy(new_tuple_end, data_begin, data_size);
-	new_tuple_end += data_size;
-
-	if (box_replace(BOX_SPACE_ID, new_tuple, new_tuple_end, NULL) != 0)
-		return SQL_TARANTOOL_ERROR;
-	else
-		return SQLITE_OK;
-
-rename_fail:
-	diag_set(ClientError, ER_SQL_EXECUTE, "can't modify name of space "
-		"created not via SQL facilities");
-	return SQL_TARANTOOL_ERROR;
-}
-
 /** Callback to forward and error from mpstream methods. */
 static void
 set_encode_error(void *error_ctx)
@@ -817,74 +704,64 @@ set_encode_error(void *error_ctx)
 	*(bool *)error_ctx = true;
 }
 
-/**
- * Encode index options @opts into message pack with @stream.
- * @param stream Message Pack Stream to use on encode.
- * @param opts Index options to encode.
- */
-static void
-mpstream_encode_index_opts(struct mpstream *stream,
-			   const struct index_opts *opts)
-{
-	mpstream_encode_map(stream, 2);
-	mpstream_encode_str(stream, "unique");
-	mpstream_encode_bool(stream, opts->is_unique);
-	mpstream_encode_str(stream, "sql");
-	mpstream_encode_strn(stream, opts->sql,
-			     opts->sql != NULL ? strlen(opts->sql) : 0);
-}
-
 int
-sql_index_update_table_name(struct index_def *def, const char *new_tbl_name,
-			    char **sql_stmt)
+sql_rename_table(uint32_t space_id, const char *new_name)
 {
-	assert(new_tbl_name != NULL);
-
-	bool is_quoted = false;
-	*sql_stmt = rename_table(db, def->opts.sql, new_tbl_name, &is_quoted);
-	if (*sql_stmt == NULL)
-		return -1;
-
+	assert(space_id != 0);
+	assert(new_name != NULL);
 	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, 2);
-	mpstream_encode_uint(&stream, def->space_id);
-	mpstream_encode_uint(&stream, def->iid);
+	mpstream_encode_array(&stream, 1);
+	mpstream_encode_uint(&stream, space_id);
 
-	/* Encode op. */
+	/* 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_INDEX_FIELD_OPTS);
-
-	/* Encode index opts. */
-	struct index_opts opts = def->opts;
-	opts.sql = *sql_stmt;
-	mpstream_encode_index_opts(&stream, &opts);
-
+	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 -1;
+		return SQL_TARANTOOL_ERROR;
 	}
 	size_t sz = region_used(region) - used;
 	char *raw = region_join(region, sz);
 	if (raw == NULL) {
 		diag_set(OutOfMemory, sz, "region_join", "raw");
-		return -1;
+		return SQL_TARANTOOL_ERROR;
 	}
-	return box_update(BOX_INDEX_ID, 0, raw, raw + op_offset,
-			  raw + op_offset, raw + sz, 0, NULL);
+	if (box_update(BOX_SPACE_ID, 0, raw, raw + op_offset, raw + op_offset,
+		       raw + sz, 0, NULL) != 0)
+		return SQL_TARANTOOL_ERROR;
+	return 0;
+}
+
+/**
+ * Encode index options @opts into message pack with @stream.
+ * @param stream Message Pack Stream to use on encode.
+ * @param opts Index options to encode.
+ */
+static void
+mpstream_encode_index_opts(struct mpstream *stream,
+			   const struct index_opts *opts)
+{
+	mpstream_encode_map(stream, 2);
+	mpstream_encode_str(stream, "unique");
+	mpstream_encode_bool(stream, opts->is_unique);
+	mpstream_encode_str(stream, "sql");
+	mpstream_encode_strn(stream, opts->sql,
+			     opts->sql != NULL ? strlen(opts->sql) : 0);
 }
 
+
 int
 tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor,
 			      struct UnpackedRecord *unpacked)
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index 3ff14d53a..e963132fe 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -72,12 +72,11 @@ int tarantoolSqlite3ClearTable(struct space *space, uint32_t *tuple_count);
  *
  * @param space_id Table's space identifier.
  * @param new_name new name of table
- * @param[out] sql_stmt CREATE TABLE statement for new name table, can be NULL.
  *
  * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise.
  */
 int
-sql_rename_table(uint32_t space_id, const char *new_name, char **sql_stmt);
+sql_rename_table(uint32_t space_id, const char *new_name);
 
 /**
  * Update CREATE INDEX field (def->opt.sql) replacing table name
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index bf6f4cdd1..e6b413c70 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4713,7 +4713,6 @@ case OP_RenameTable: {
 	struct space *space;
 	const char *zOldTableName;
 	const char *zNewTableName;
-	char *zSqlStmt;
 
 	space_id = pOp->p1;
 	space = space_by_id(space_id);
@@ -4725,7 +4724,7 @@ case OP_RenameTable: {
 	zNewTableName = pOp->p4.z;
 	zOldTableName = sqlite3DbStrNDup(db, zOldTableName,
 					 sqlite3Strlen30(zOldTableName));
-	rc = sql_rename_table(space_id, zNewTableName, &zSqlStmt);
+	rc = sql_rename_table(space_id, zNewTableName);
 	if (rc) goto abort_due_to_error;
 	/*
 	 * Rebuild 'CREATE TRIGGER' expressions of all triggers
@@ -4753,7 +4752,6 @@ case OP_RenameTable: {
 		trigger = next_trigger;
 	}
 	sqlite3DbFree(db, (void*)zOldTableName);
-	sqlite3DbFree(db, (void*)zSqlStmt);
 	break;
 }
 
diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua
index 7402526d9..1aad555c0 100755
--- a/test/sql-tap/alter.test.lua
+++ b/test/sql-tap/alter.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(41)
+test:plan(43)
 
 test:do_execsql_test(
     "alter-1.1",
@@ -87,16 +87,42 @@ test:do_catchsql_test(
         -- </alter-2.2>
     })
 
+test:do_test(
+    "alter-2.3.prepare",
+    function()
+        format = {}
+        format[1] = { name = 'id', type = 'integer'}
+        format[2] = { name = 'f2', type = 'number'}
+        s = box.schema.create_space('t', {format = format})
+        i = s:create_index('i', {parts= {1, 'integer'}})
+
+        s:replace{1, 4}
+        s:replace{2, 2}
+        s:replace{3, 3}
+        s:replace{4, 3}
+    end,
+    {})
+
 test:do_catchsql_test(
     "alter-2.3",
     [[
-        ALTER TABLE "_space" RENAME TO space;
+        ALTER TABLE "t" RENAME TO "new_lua_space";
     ]], {
         -- <alter-2.3>
-        1, "Failed to execute SQL statement: can't modify name of space created not via SQL facilities"
+        0
         -- </alter-2.3>
     })
 
+test:do_execsql_test(
+    "alter-2.4",
+    [[
+        SELECT "f2" from "new_lua_space" WHERE "f2" >= 3 ORDER BY "id";
+    ]], {
+    -- <alter-2.4>
+        4, 3, 3
+    -- </alter-2.4>
+})
+
 test:do_execsql_test(
     "alter-3.1",
     [[
-- 
2.15.1