From ce19dcbce456c18f906a709800b8828c3e3dca99 Mon Sep 17 00:00:00 2001
From: Nikita Pettik <korablev@tarantool.org>
Date: Wed, 5 Dec 2018 17:30:00 +0300
Subject: [PATCH] sql: don't update SQL string during renaming
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 | 189 ++++++------------------------------
src/box/sql/alter.c | 72 --------------
src/box/sql/sqliteInt.h | 1 -
src/box/sql/tarantoolInt.h | 29 +-----
src/box/sql/vdbe.c | 4 +-
test/sql-tap/alter.test.lua | 32 +++++-
6 files changed, 61 insertions(+), 266 deletions(-)
diff --git a/src/box/sql.c b/src/box/sql.c
index 240b3e0b91..713e5956ee 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -697,124 +697,41 @@ int tarantoolSqlite3RenameTrigger(const char *trig_name,
return SQL_TARANTOOL_ERROR;
}
+/** Callback to forward and error from mpstream methods. */
+static void
+set_encode_error(void *error_ctx)
+{
+ *(bool *)error_ctx = true;
+}
+
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)
{
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");
+ int name_len = strlen(new_name);
+ struct region *region = &fiber()->gc;
+ /* 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, size, "region_alloc", "raw");
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)
+ /* 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;
- 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)
-{
- *(bool *)error_ctx = true;
+ return 0;
}
/**
@@ -834,56 +751,6 @@ mpstream_encode_index_opts(struct mpstream *stream,
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)
-{
- 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;
-
- 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);
-
- /* Encode op. */
- 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_flush(&stream);
- if (is_error) {
- diag_set(OutOfMemory, stream.pos - stream.buf,
- "mpstream_flush", "stream");
- return -1;
- }
- 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 box_update(BOX_INDEX_ID, 0, raw, raw + op_offset,
- raw + op_offset, raw + sz, 0, NULL);
-}
int
tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor,
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 89e51eb30d..d0ce9d8938 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -77,78 +77,6 @@ sql_alter_table_rename(struct Parse *parse, struct SrcList *src_tab,
goto exit_rename_table;
}
-/*
- * This function implements part of the ALTER TABLE command.
- * The first argument is the text of a CREATE TABLE or CREATE INDEX command.
- * The second is a table name. The table name in the CREATE TABLE or
- * CREATE INDEX statement is replaced with the second argument and
- * the result returned. There is no need to deallocate returned memory, since
- * it will be doe automatically by VDBE. Note that new statement always
- * contains quoted new table name.
- *
- * Examples:
- *
- * sqlite_rename_table('CREATE TABLE abc(a, b, c)', 'def')
- * -> 'CREATE TABLE "def"(a, b, c)'
- *
- * sqlite_rename_table('CREATE INDEX i ON abc(a)', 'def')
- * -> 'CREATE INDEX i ON "def"(a, b, c)'
- *
- * @param sql_stmt text of a CREATE TABLE or CREATE INDEX statement
- * @param table_name new table name
- * @param[out] is_quoted true if statement to be modified contains quoted name
- *
- * @retval new SQL statement on success, NULL otherwise.
- */
-char*
-rename_table(sqlite3 *db, const char *sql_stmt, const char *table_name,
- bool *is_quoted)
-{
- assert(sql_stmt);
- assert(table_name);
- assert(is_quoted);
-
- int token;
- Token old_name;
- const char *csr = sql_stmt;
- int len = 0;
- char *new_sql_stmt;
- bool unused;
-
- /* The principle used to locate the table name in the CREATE TABLE
- * statement is that the table name is the first non-space token that
- * is immediately followed by a TK_LP or TK_USING token.
- */
- do {
- if (!*csr) {
- /* Ran out of input before finding a bracket. */
- return NULL;
- }
- /* Store the token that zCsr points to in tname. */
- old_name.z = (char *)csr;
- old_name.n = len;
- /* Advance zCsr to the next token.
- * Store that token type in 'token', and its length
- * in 'len' (to be used next iteration of this loop).
- */
- do {
- csr += len;
- len = sql_token(csr, &token, &unused);
- } while (token == TK_SPACE);
- assert(len > 0);
- } while (token != TK_LP && token != TK_USING);
-
- if (*old_name.z == '"')
- *is_quoted = true;
- /* No need to care about deallocating zRet, since its memory
- * will be automatically freed by VDBE.
- */
- new_sql_stmt = sqlite3MPrintf(db, "%.*s\"%w\"%s",
- (int)((old_name.z) - sql_stmt), sql_stmt,
- table_name, old_name.z + old_name.n);
- return new_sql_stmt;
-}
-
/* This function is used to implement the ALTER TABLE command.
* The table name in the CREATE TRIGGER statement is replaced with the third
* argument and the result returned. This is analagous to rename_table()
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 1ec52b875b..8675958f94 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4461,7 +4461,6 @@ int sqlite3ResolveOrderGroupBy(Parse *, Select *, ExprList *, const char *);
void
sqlite3ColumnDefault(Vdbe *v, struct space_def *def, int i, int ireg);
-char* rename_table(sqlite3 *, const char *, const char *, bool *);
char* rename_trigger(sqlite3 *, char const *, char const *, bool *);
/**
* Find a collation by name. Set error in @a parser if not found.
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index 3ff14d53ac..0aa31bdd63 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -61,37 +61,14 @@ sql_delete_by_key(struct space *space, uint32_t iid, char *key,
int tarantoolSqlite3ClearTable(struct space *space, uint32_t *tuple_count);
/**
- * Rename the table in _space. Update tuple with corresponding id
- * with new name and statement fields and insert back. If sql_stmt
- * is NULL, then return from function after getting length of new
- * statement: it is the way how to dynamically allocate memory for
- * new statement in VDBE. So basically this function should be
- * called twice: firstly to get length of CREATE TABLE statement,
- * and secondly to make routine of replacing tuple and filling out
- * param sql_stmt with new CREATE TABLE statement.
- *
+ * Rename the table in _space.
* @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);
-
-/**
- * Update CREATE INDEX field (def->opt.sql) replacing table name
- * w/ new one in _index space.
- *
- * @param idef Index definition.
- * @param new_tbl_name new name of table
- * @param[out] sql_stmt New CREATE INDEX statement.
- *
- * @retval 0 on success, -1 otherwise.
+ * @retval 0 on success, SQL_TARANTOOL_ERROR otherwise.
*/
int
-sql_index_update_table_name(struct index_def *idef, const char *new_tbl_name,
- char **sql_stmt);
+sql_rename_table(uint32_t space_id, const char *new_name);
/* Alter trigger statement after rename table. */
int tarantoolSqlite3RenameTrigger(const char *zTriggerName,
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index bf6f4cdd12..e6b413c709 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 7402526d96..1aad555c07 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",
[[
--
GitLab