* [tarantool-patches] [PATCH 0/6] Remove string of SQL statement from opts
@ 2018-12-09 21:30 Nikita Pettik
2018-12-09 21:30 ` [tarantool-patches] [PATCH 1/6] sql: avoid calling sql_encode_table_opts() during trigger creation Nikita Pettik
` (6 more replies)
0 siblings, 7 replies; 20+ messages in thread
From: Nikita Pettik @ 2018-12-09 21:30 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Branch: https://github.com/tarantool/tarantool/tree/np/gh-2647-remove-sql-from-opts
Issue: https://github.com/tarantool/tarantool/issues/2647
After DD integration was finished, there is no need to add string
of SQL statement to space or index opts (except for VIEWs). So,
current patch-set completely removes SQL from index opts, and
removes SQL from space opts for ordinary spaces.
Nikita Pettik (6):
sql: avoid calling sql_encode_table_opts() during trigger creation
sql: don't update SQL string during renaming
test: fix sqltester methods to drop all tables/views
sql: don't add string of 'CREATE TABLE...' to space opts
sql: don't add string of 'CREATE INDEX ...' to index opts
Remove SQL string from index opts
src/box/alter.cc | 9 --
src/box/index_def.c | 20 ---
src/box/index_def.h | 9 --
src/box/sql.c | 181 +++-------------------
src/box/sql/analyze.c | 5 +-
src/box/sql/build.c | 277 ++--------------------------------
src/box/sql/pragma.c | 2 -
src/box/sql/sqliteInt.h | 1 -
src/box/sql/tarantoolInt.h | 3 +-
src/box/sql/trigger.c | 20 ++-
src/box/sql/vdbe.c | 4 +-
src/box/sql/where.c | 1 -
test/sql-tap/alter.test.lua | 32 +++-
test/sql-tap/drop_all.test.lua | 4 +-
test/sql-tap/identifier_case.test.lua | 6 +-
test/sql-tap/lua/sqltester.lua | 38 ++---
test/sql/upgrade.result | 8 +-
test/sql/view.result | 3 +
test/sql/view.test.lua | 1 +
19 files changed, 100 insertions(+), 524 deletions(-)
--
2.15.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] [PATCH 1/6] sql: avoid calling sql_encode_table_opts() during trigger creation
2018-12-09 21:30 [tarantool-patches] [PATCH 0/6] Remove string of SQL statement from opts Nikita Pettik
@ 2018-12-09 21:30 ` Nikita Pettik
2018-12-10 14:17 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-09 21:30 ` [tarantool-patches] [PATCH 2/6] sql: don't update SQL string during renaming Nikita Pettik
` (5 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Nikita Pettik @ 2018-12-09 21:30 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
At the last stage of trigger creation, trigger's create statement
("CREATE TRIGGER ...") is encoded to msgpack. Since only this string is
only member of a map to be encoded, is makes no sense to call whole
sql_encode_table_opts() function, which in turn processes table's
checks, opts for VIEW etc.
Needed for #2647
---
src/box/sql/trigger.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index c38f9cd9d..c7d22b9d0 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -212,19 +212,17 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
parse->nMem += 3;
int record = ++parse->nMem;
- uint32_t opts_buff_sz = 0;
- char *data = sql_encode_table_opts(&fiber()->gc, NULL, sql_str,
- &opts_buff_sz);
- sqlite3DbFree(db, sql_str);
- if (data == NULL) {
- parse->nErr++;
- parse->rc = SQL_TARANTOOL_ERROR;
- goto cleanup;
- }
- char *opts_buff = sqlite3DbMallocRaw(db, opts_buff_sz);
+ uint32_t opts_buff_sz = mp_sizeof_map(1) +
+ mp_sizeof_str(strlen("sql")) +
+ mp_sizeof_str(strlen(sql_str));
+ char *opts_buff = (char *) sqlite3DbMallocRaw(db, opts_buff_sz);
if (opts_buff == NULL)
goto cleanup;
- memcpy(opts_buff, data, opts_buff_sz);
+
+ char *data = mp_encode_map(opts_buff, 1);
+ data = mp_encode_str(data, "sql", strlen("sql"));
+ data = mp_encode_str(data, sql_str, strlen(sql_str));
+ sqlite3DbFree(db, sql_str);
trigger_name = sqlite3DbStrDup(db, trigger_name);
if (trigger_name == NULL) {
--
2.15.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] [PATCH 2/6] sql: don't update SQL string during renaming
2018-12-09 21:30 [tarantool-patches] [PATCH 0/6] Remove string of SQL statement from opts Nikita Pettik
2018-12-09 21:30 ` [tarantool-patches] [PATCH 1/6] sql: avoid calling sql_encode_table_opts() during trigger creation Nikita Pettik
@ 2018-12-09 21:30 ` Nikita Pettik
2018-12-10 14:16 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-09 21:30 ` [tarantool-patches] [PATCH 3/6] test: fix sqltester methods to drop all tables/views Nikita Pettik
` (4 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Nikita Pettik @ 2018-12-09 21:30 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] [PATCH 3/6] test: fix sqltester methods to drop all tables/views
2018-12-09 21:30 [tarantool-patches] [PATCH 0/6] Remove string of SQL statement from opts Nikita Pettik
2018-12-09 21:30 ` [tarantool-patches] [PATCH 1/6] sql: avoid calling sql_encode_table_opts() during trigger creation Nikita Pettik
2018-12-09 21:30 ` [tarantool-patches] [PATCH 2/6] sql: don't update SQL string during renaming Nikita Pettik
@ 2018-12-09 21:30 ` Nikita Pettik
2018-12-10 14:16 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-09 21:30 ` [tarantool-patches] [PATCH 4/6] sql: don't add string of 'CREATE TABLE...' to space opts Nikita Pettik
` (3 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Nikita Pettik @ 2018-12-09 21:30 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Since we are going to remove string of SQL "CREATE TABLE ..." statement
from space's opts, lets rework methods in sqltester to drop all tables
and views in order to avoid relying on this parameter.
Part of #2647
---
test/sql-tap/drop_all.test.lua | 4 ++--
test/sql-tap/identifier_case.test.lua | 6 +++---
test/sql-tap/lua/sqltester.lua | 38 ++++++++++++++---------------------
3 files changed, 20 insertions(+), 28 deletions(-)
diff --git a/test/sql-tap/drop_all.test.lua b/test/sql-tap/drop_all.test.lua
index 86a2587eb..b08daf87c 100755
--- a/test/sql-tap/drop_all.test.lua
+++ b/test/sql-tap/drop_all.test.lua
@@ -22,7 +22,7 @@ test:do_test(
test:do_test(
prefix.."-1.1",
function()
- return #test:drop_all_views()
+ return test:drop_all_views()
end,
N
)
@@ -30,7 +30,7 @@ test:do_test(
test:do_test(
prefix.."-1.1",
function()
- return #test:drop_all_tables()
+ return test:drop_all_tables()
end,
N
)
diff --git a/test/sql-tap/identifier_case.test.lua b/test/sql-tap/identifier_case.test.lua
index 9e8107aef..1d248f9dc 100755
--- a/test/sql-tap/identifier_case.test.lua
+++ b/test/sql-tap/identifier_case.test.lua
@@ -64,7 +64,7 @@ test:do_test(
test:do_test(
test_prefix.."1.3.3",
function ()
- return #test:drop_all_tables()
+ return test:drop_all_tables()
end,
3)
@@ -112,7 +112,7 @@ end
test:do_test(
test_prefix.."2.3.1",
function ()
- return #test:drop_all_tables()
+ return test:drop_all_tables()
end,
6)
@@ -152,7 +152,7 @@ end
test:do_test(
test_prefix.."3.2.1",
function ()
- return #test:drop_all_tables()
+ return test:drop_all_tables()
end,
1)
diff --git a/test/sql-tap/lua/sqltester.lua b/test/sql-tap/lua/sqltester.lua
index 8751ef820..672d2e67d 100644
--- a/test/sql-tap/lua/sqltester.lua
+++ b/test/sql-tap/lua/sqltester.lua
@@ -277,7 +277,7 @@ end
test.catchsql2 = catchsql2
-- Show the VDBE program for an SQL statement but omit the Trace
--- opcode at the beginning. This procedure can be used to prove
+-- opcode at the beginning. This procedure can be used to PROVE
-- that different SQL statements generate exactly the same VDBE code.
local function explain_no_trace(self, sql)
tr = execsql(self, "EXPLAIN "..sql)
@@ -288,35 +288,27 @@ local function explain_no_trace(self, sql)
end
test.explain_no_trace = explain_no_trace
json = require("json")
-function test.find_spaces_by_sql_statement(self, statement)
- local spaces = box.space._space:select{}
- local res = {}
- for _, space in ipairs(spaces) do
- local name_num = 3-- [3] is space name
- local options_num = 6-- [6] is space options
- if not space[name_num]:startswith("_") and space[options_num]["sql"] then
- if string.find(space[options_num]["sql"], statement) then
- table.insert(res, space[name_num])
- end
- end
- end
- return res
-end
function test.drop_all_tables(self)
- local tables = test:find_spaces_by_sql_statement("CREATE TABLE")
- for _, table in ipairs(tables) do
- test:execsql(string.format([[DROP TABLE "%s";]], table))
+ local entry_count = 0
+ for _, v in box.space._space:pairs() do
+ if v[1] >= 512 then
+ test:execsql(string.format([[DROP TABLE "%s";]], v[3]))
+ entry_count = entry_count + 1
+ end
end
- return tables
+ return entry_count
end
function test.drop_all_views(self)
- local views = test:find_spaces_by_sql_statement("CREATE VIEW")
- for _, view in ipairs(views) do
- test:execsql(string.format([[DROP VIEW "%s";]], view))
+ local entry_count = 0
+ for _, v in box.space._space:pairs() do
+ if v[1] > 512 and v[6]["view"] == true then
+ test:execsql(string.format([[DROP VIEW "%s";]], v[3]))
+ entry_count = entry_count + 1
+ end
end
- return views
+ return entry_count
end
function test.do_select_tests(self, label, tests)
--
2.15.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] [PATCH 4/6] sql: don't add string of 'CREATE TABLE...' to space opts
2018-12-09 21:30 [tarantool-patches] [PATCH 0/6] Remove string of SQL statement from opts Nikita Pettik
` (2 preceding siblings ...)
2018-12-09 21:30 ` [tarantool-patches] [PATCH 3/6] test: fix sqltester methods to drop all tables/views Nikita Pettik
@ 2018-12-09 21:30 ` Nikita Pettik
2018-12-10 14:17 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-09 21:30 ` [tarantool-patches] [PATCH 5/6] sql: don't add string of 'CREATE INDEX ...' to index opts Nikita Pettik
` (2 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Nikita Pettik @ 2018-12-09 21:30 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
We don't rely no more on this string anymore and it can be removed for
ordinary tables. However, it is still used to hold SELECT body for view.
Part of #2647
---
src/box/sql.c | 8 +-
src/box/sql/analyze.c | 2 +-
src/box/sql/build.c | 242 ++----------------------------------------------
src/box/sql/pragma.c | 2 -
src/box/sql/sqliteInt.h | 1 -
test/sql/upgrade.result | 8 +-
test/sql/view.result | 3 +
test/sql/view.test.lua | 1 +
8 files changed, 19 insertions(+), 248 deletions(-)
diff --git a/src/box/sql.c b/src/box/sql.c
index e9fa89df9..e79265823 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1070,7 +1070,6 @@ char *
sql_encode_table_opts(struct region *region, struct Table *table,
const char *sql, uint32_t *size)
{
- assert(sql != NULL);
size_t used = region_used(region);
struct mpstream stream;
bool is_error = false;
@@ -1087,11 +1086,12 @@ sql_encode_table_opts(struct region *region, struct Table *table,
a = checks->a;
}
}
- mpstream_encode_map(&stream, 1 + is_view + (checks_cnt > 0));
+ assert(is_view || sql == NULL);
+ mpstream_encode_map(&stream, 2 * is_view + (checks_cnt > 0));
- mpstream_encode_str(&stream, "sql");
- mpstream_encode_str(&stream, sql);
if (is_view) {
+ mpstream_encode_str(&stream, "sql");
+ mpstream_encode_str(&stream, sql);
mpstream_encode_str(&stream, "view");
mpstream_encode_bool(&stream, true);
}
diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 3f492800c..8f8b6eb5d 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1056,7 +1056,7 @@ loadAnalysis(Parse * pParse)
static int
sql_space_foreach_analyze(struct space *space, void *data)
{
- if (space->def->opts.sql == NULL || space->def->opts.is_view)
+ if (space->def->opts.is_view)
return 0;
vdbe_emit_analyze_space((struct Parse*)data, space);
return 0;
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 52f0bde15..fd1666c6d 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -595,102 +595,6 @@ sql_column_add_nullable_action(struct Parse *parser,
field->is_nullable = action_is_nullable(nullable_action);
}
-/*
- * Scan the column type name zType (length nType) and return the
- * associated affinity type.
- *
- * This routine does a case-independent search of zType for the
- * substrings in the following table. If one of the substrings is
- * found, the corresponding affinity is returned. If zType contains
- * more than one of the substrings, entries toward the top of
- * the table take priority. For example, if zType is 'BLOBINT',
- * AFFINITY_INTEGER is returned.
- *
- * Substring | Affinity
- * --------------------------------
- * 'INT' | AFFINITY_INTEGER
- * 'CHAR' | AFFINITY_TEXT
- * 'CLOB' | AFFINITY_TEXT
- * 'TEXT' | AFFINITY_TEXT
- * 'BLOB' | AFFINITY_BLOB
- * 'REAL' | AFFINITY_REAL
- * 'FLOA' | AFFINITY_REAL
- * 'DOUB' | AFFINITY_REAL
- *
- * If none of the substrings in the above table are found,
- * AFFINITY_NUMERIC is returned.
- */
-char
-sqlite3AffinityType(const char *zIn, u8 * pszEst)
-{
- u32 h = 0;
- char aff = AFFINITY_NUMERIC;
- const char *zChar = 0;
-
- assert(zIn != 0);
- while (zIn[0]) {
- h = (h << 8) + sqlite3UpperToLower[(*zIn) & 0xff];
- zIn++;
- if (h == (('c' << 24) + ('h' << 16) + ('a' << 8) + 'r')) { /* CHAR */
- aff = AFFINITY_TEXT;
- zChar = zIn;
- } else if (h == (('c' << 24) + ('l' << 16) + ('o' << 8) + 'b')) { /* CLOB */
- aff = AFFINITY_TEXT;
- } else if (h == (('t' << 24) + ('e' << 16) + ('x' << 8) + 't')) { /* TEXT */
- aff = AFFINITY_TEXT;
- } else if (h == (('b' << 24) + ('l' << 16) + ('o' << 8) + 'b') /* BLOB */
- &&(aff == AFFINITY_NUMERIC
- || aff == AFFINITY_REAL)) {
- aff = AFFINITY_BLOB;
- if (zIn[0] == '(')
- zChar = zIn;
-#ifndef SQLITE_OMIT_FLOATING_POINT
- } else if (h == (('r' << 24) + ('e' << 16) + ('a' << 8) + 'l') /* REAL */
- &&aff == AFFINITY_NUMERIC) {
- aff = AFFINITY_REAL;
- } else if (h == (('f' << 24) + ('l' << 16) + ('o' << 8) + 'a') /* FLOA */
- &&aff == AFFINITY_NUMERIC) {
- aff = AFFINITY_REAL;
- } else if (h == (('d' << 24) + ('o' << 16) + ('u' << 8) + 'b') /* DOUB */
- &&aff == AFFINITY_NUMERIC) {
- aff = AFFINITY_REAL;
-#endif
- } else if ((h & 0x00FFFFFF) == (('i' << 16) + ('n' << 8) + 't')) { /* INT */
- aff = AFFINITY_INTEGER;
- break;
- }
- }
-
- /* If pszEst is not NULL, store an estimate of the field size. The
- * estimate is scaled so that the size of an integer is 1.
- */
- if (pszEst) {
- *pszEst = 1; /* default size is approx 4 bytes
- */
- if (aff < AFFINITY_NUMERIC) {
- if (zChar) {
- while (zChar[0]) {
- if (sqlite3Isdigit(zChar[0])) {
- int v = 0;
- sqlite3GetInt32(zChar, &v);
- v = v / 4 + 1;
- if (v > 255)
- v = 255;
- *pszEst = v; /* BLOB(k), VARCHAR(k), CHAR(k) -> r=(k/4+1)
- */
- break;
- }
- zChar++;
- }
- } else {
- *pszEst = 5; /* BLOB, TEXT, CLOB -> r=5 (approx 20 bytes)
- */
- }
- }
- }
- return aff;
-}
-
/*
* The expression is the default value for the most recently added column
* of the table currently under construction.
@@ -959,136 +863,6 @@ vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
index_id, 0, (void *) space, P4_SPACEPTR);
}
-/*
- * Measure the number of characters needed to output the given
- * identifier. The number returned includes any quotes used
- * but does not include the null terminator.
- *
- * The estimate is conservative. It might be larger that what is
- * really needed.
- */
-static int
-identLength(const char *z)
-{
- int n;
- for (n = 0; *z; n++, z++) {
- if (*z == '"') {
- n++;
- }
- }
- return n + 2;
-}
-
-/*
- * The first parameter is a pointer to an output buffer. The second
- * parameter is a pointer to an integer that contains the offset at
- * which to write into the output buffer. This function copies the
- * nul-terminated string pointed to by the third parameter, zSignedIdent,
- * to the specified offset in the buffer and updates *pIdx to refer
- * to the first byte after the last byte written before returning.
- *
- * If the string zSignedIdent consists entirely of alpha-numeric
- * characters, does not begin with a digit and is not an SQL keyword,
- * then it is copied to the output buffer exactly as it is. Otherwise,
- * it is quoted using double-quotes.
- */
-static void
-identPut(char *z, int *pIdx, char *zSignedIdent)
-{
- unsigned char *zIdent = (unsigned char *)zSignedIdent;
- int i, j, needQuote;
- i = *pIdx;
-
- for (j = 0; zIdent[j]; j++) {
- if (!sqlite3Isalnum(zIdent[j]) && zIdent[j] != '_')
- break;
- }
- needQuote = sqlite3Isdigit(zIdent[0])
- || sqlite3KeywordCode(zIdent, j) != TK_ID
- || zIdent[j] != 0 || j == 0;
-
- if (needQuote)
- z[i++] = '"';
- for (j = 0; zIdent[j]; j++) {
- z[i++] = zIdent[j];
- if (zIdent[j] == '"')
- z[i++] = '"';
- }
- if (needQuote)
- z[i++] = '"';
- z[i] = 0;
- *pIdx = i;
-}
-
-/*
- * Generate a CREATE TABLE statement appropriate for the given
- * table. Memory to hold the text of the statement is obtained
- * from sqliteMalloc() and must be freed by the calling function.
- */
-static char *
-createTableStmt(sqlite3 * db, Table * p)
-{
- char *zStmt;
- char *zSep, *zSep2, *zEnd;
- int n = 0;
- for (uint32_t i = 0; i < p->def->field_count; i++)
- n += identLength(p->def->fields[i].name) + 5;
- n += identLength(p->def->name);
- if (n < 50) {
- zSep = "";
- zSep2 = ",";
- zEnd = ")";
- } else {
- zSep = "\n ";
- zSep2 = ",\n ";
- zEnd = "\n)";
- }
- n += 35 + 6 * p->def->field_count;
- zStmt = sqlite3DbMallocRaw(0, n);
- if (zStmt == 0) {
- sqlite3OomFault(db);
- return 0;
- }
- sqlite3_snprintf(n, zStmt, "CREATE TABLE ");
- int k = sqlite3Strlen30(zStmt);
- identPut(zStmt, &k, p->def->name);
- zStmt[k++] = '(';
- for (uint32_t i = 0; i < p->def->field_count; i++) {
- static const char *const azType[] = {
- /* AFFINITY_BLOB */ "",
- /* AFFINITY_TEXT */ " TEXT",
- /* AFFINITY_NUMERIC */ " NUM",
- /* AFFINITY_INTEGER */ " INT",
- /* AFFINITY_REAL */ " REAL"
- };
- int len;
- const char *zType;
-
- sqlite3_snprintf(n - k, &zStmt[k], zSep);
- k += sqlite3Strlen30(&zStmt[k]);
- zSep = zSep2;
- identPut(zStmt, &k, p->def->fields[i].name);
- char affinity = p->def->fields[i].affinity;
- assert(affinity - AFFINITY_BLOB >= 0);
- assert(affinity - AFFINITY_BLOB < ArraySize(azType));
- testcase(affinity == AFFINITY_BLOB);
- testcase(affinity == AFFINITY_TEXT);
- testcase(affinity == AFFINITY_NUMERIC);
- testcase(affinity == AFFINITY_INTEGER);
- testcase(affinity == AFFINITY_REAL);
-
- zType = azType[affinity - AFFINITY_BLOB];
- len = sqlite3Strlen30(zType);
- assert(affinity == AFFINITY_BLOB ||
- affinity == sqlite3AffinityType(zType, 0));
- memcpy(&zStmt[k], zType, len);
- k += len;
- assert(k <= n);
- }
- sqlite3_snprintf(n - k, &zStmt[k], "%s", zEnd);
- return zStmt;
-}
-
/*
* Generate code to determine the new space Id.
* Fetch the max space id seen so far from _schema and increment it.
@@ -1524,19 +1298,15 @@ sqlite3EndTable(Parse * pParse, /* Parse context */
if (NEVER(v == 0))
return;
- /* Text of the CREATE TABLE or CREATE VIEW statement. */
- char *stmt;
- if (pSelect) {
- stmt = createTableStmt(db, p);
- } else {
- struct Token *pEnd2 = p->def->opts.is_view ?
- &pParse->sLastToken : pEnd;
+ /* Text of the CREATE VIEW statement. */
+ char *stmt = NULL;
+ if (p->def->opts.is_view) {
+ struct Token *pEnd2 = &pParse->sLastToken;
int n = pEnd2->z - pParse->sNameToken.z;
if (pEnd2->z[0] != ';')
n += pEnd2->n;
- stmt = sqlite3MPrintf(db, "CREATE %s %.*s",
- p->def->opts.is_view ? "VIEW" : "TABLE",
- n, pParse->sNameToken.z);
+ stmt = sqlite3MPrintf(db, "CREATE VIEW %.*s", n,
+ pParse->sNameToken.z);
}
int reg_space_id = getNewSpaceId(pParse);
createSpace(pParse, reg_space_id, stmt);
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 5c3501702..325c272b0 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -347,8 +347,6 @@ sql_pragma_index_info(struct Parse *parse,
struct space *space = space_by_name(tbl_name);
if (space == NULL)
return;
- if (space->def->opts.sql == NULL)
- return;
uint32_t iid = box_index_id_by_name(space->def->id, idx_name,
strlen(idx_name));
if (iid == BOX_ID_NIL)
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index dbf58d967..ae6886685 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4474,7 +4474,6 @@ char* rename_trigger(sqlite3 *, char const *, char const *, bool *);
*/
struct coll *
sql_get_coll_seq(Parse *parser, const char *name, uint32_t *coll_id);
-char sqlite3AffinityType(const char *, u8 *);
void sqlite3Analyze(Parse *, Token *);
/**
diff --git a/test/sql/upgrade.result b/test/sql/upgrade.result
index 5e7d85109..79c7eb245 100644
--- a/test/sql/upgrade.result
+++ b/test/sql/upgrade.result
@@ -80,13 +80,13 @@ box.sql.execute("CREATE TRIGGER t2t AFTER INSERT ON t BEGIN INSERT INTO t_out VA
...
box.space._space.index['name']:get('T')
---
-- [513, 1, 'T', 'memtx', 1, {'sql': 'CREATE TABLE t(x INTEGER PRIMARY KEY)'}, [{'affinity': 68,
- 'type': 'integer', 'nullable_action': 'abort', 'name': 'X', 'is_nullable': false}]]
+- [513, 1, 'T', 'memtx', 1, {}, [{'affinity': 68, 'type': 'integer', 'nullable_action': 'abort',
+ 'name': 'X', 'is_nullable': false}]]
...
box.space._space.index['name']:get('T_OUT')
---
-- [514, 1, 'T_OUT', 'memtx', 1, {'sql': 'CREATE TABLE t_out(x INTEGER PRIMARY KEY)'},
- [{'affinity': 68, 'type': 'integer', 'nullable_action': 'abort', 'name': 'X', 'is_nullable': false}]]
+- [514, 1, 'T_OUT', 'memtx', 1, {}, [{'affinity': 68, 'type': 'integer', 'nullable_action': 'abort',
+ 'name': 'X', 'is_nullable': false}]]
...
t1t = box.space._trigger:get('T1T')
---
diff --git a/test/sql/view.result b/test/sql/view.result
index b211bcb2e..3d2569524 100644
--- a/test/sql/view.result
+++ b/test/sql/view.result
@@ -49,6 +49,9 @@ t1 = box.space._space.index[2]:select('T1')[1]:totable();
t1[6]['view'] = true;
---
...
+t1[6]['sql'] = 'SELECT * FROM t1;'
+---
+...
box.space._space:replace(t1);
---
- error: 'Can''t modify space ''T1'': can not convert a space to a view and vice versa'
diff --git a/test/sql/view.test.lua b/test/sql/view.test.lua
index a6269a1bf..239e4ce5e 100644
--- a/test/sql/view.test.lua
+++ b/test/sql/view.test.lua
@@ -23,6 +23,7 @@ box.space._space:replace(v1);
t1 = box.space._space.index[2]:select('T1')[1]:totable();
t1[6]['view'] = true;
+t1[6]['sql'] = 'SELECT * FROM t1;'
box.space._space:replace(t1);
-- View can't exist without SQL statement.
--
2.15.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] [PATCH 5/6] sql: don't add string of 'CREATE INDEX ...' to index opts
2018-12-09 21:30 [tarantool-patches] [PATCH 0/6] Remove string of SQL statement from opts Nikita Pettik
` (3 preceding siblings ...)
2018-12-09 21:30 ` [tarantool-patches] [PATCH 4/6] sql: don't add string of 'CREATE TABLE...' to space opts Nikita Pettik
@ 2018-12-09 21:30 ` Nikita Pettik
2018-12-10 14:18 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-09 21:30 ` [tarantool-patches] [PATCH 6/6] Remove SQL string from " Nikita Pettik
2018-12-25 13:45 ` [tarantool-patches] Re: [PATCH 0/6] Remove string of SQL statement from opts Kirill Yukhin
6 siblings, 1 reply; 20+ messages in thread
From: Nikita Pettik @ 2018-12-09 21:30 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Part of #2647
---
src/box/sql.c | 22 +++-------------------
src/box/sql/build.c | 35 +++--------------------------------
2 files changed, 6 insertions(+), 51 deletions(-)
diff --git a/src/box/sql.c b/src/box/sql.c
index e79265823..45c539ec7 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -744,24 +744,6 @@ sql_rename_table(uint32_t space_id, const char *new_name)
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)
@@ -1214,7 +1196,9 @@ sql_encode_index_opts(struct region *region, const struct index_opts *opts,
bool is_error = false;
mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
set_encode_error, &is_error);
- mpstream_encode_index_opts(&stream, opts);
+ mpstream_encode_map(&stream, 1);
+ mpstream_encode_str(&stream, "unique");
+ mpstream_encode_bool(&stream, opts->is_unique);;
mpstream_flush(&stream);
if (is_error) {
diag_set(OutOfMemory, stream.pos - stream.buf, "mpstream_flush",
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index fd1666c6d..ab2a56420 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -947,12 +947,6 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def,
SQL_SUBTYPE_MSGPACK, index_parts, P4_STATIC);
sqlite3VdbeAddOp3(v, OP_MakeRecord, entry_reg, 6, tuple_reg);
sqlite3VdbeAddOp3(v, OP_SInsert, BOX_INDEX_ID, 0, tuple_reg);
- /*
- * Non-NULL value means that index has been created via
- * separate CREATE INDEX statement.
- */
- if (idx_def->opts.sql != NULL)
- sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1);
return;
error:
@@ -2138,12 +2132,11 @@ static int
index_fill_def(struct Parse *parse, struct index *index,
struct space_def *space_def, uint32_t iid, const char *name,
uint32_t name_len, struct ExprList *expr_list,
- enum sql_index_type idx_type, char *sql_stmt)
+ enum sql_index_type idx_type)
{
struct index_opts opts;
index_opts_create(&opts);
opts.is_unique = idx_type != SQL_INDEX_TYPE_NON_UNIQUE;
- opts.sql = sql_stmt;
index->def = NULL;
int rc = -1;
@@ -2400,25 +2393,13 @@ sql_create_index(struct Parse *parse, struct Token *token,
* TODO: Issue a warning if the table primary key is used
* as part of the index key.
*/
- char *sql_stmt = NULL;
- if (tbl_name != NULL) {
- int n = (int) (parse->sLastToken.z - token->z) +
- parse->sLastToken.n;
- if (token->z[n - 1] == ';')
- n--;
- sql_stmt = sqlite3MPrintf(db, "CREATE%s INDEX %.*s",
- idx_type == SQL_INDEX_TYPE_NON_UNIQUE ?
- "" : " UNIQUE", n, token->z);
- if (sql_stmt == NULL)
- goto exit_create_index;
- }
uint32_t iid;
if (idx_type != SQL_INDEX_TYPE_CONSTRAINT_PK)
iid = space->index_id_max + 1;
else
iid = 0;
if (index_fill_def(parse, index, def, iid, name, strlen(name),
- col_list, idx_type, sql_stmt) != 0)
+ col_list, idx_type) != 0)
goto exit_create_index;
/*
* Remove all redundant columns from the PRIMARY KEY.
@@ -2546,6 +2527,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
sqlite3VdbeAddOp1(vdbe, OP_Close, cursor);
vdbe_emit_create_index(parse, def, index->def,
def->id, index_id);
+ sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
sqlite3VdbeAddOp0(vdbe, OP_Expire);
}
@@ -2598,17 +2580,6 @@ sql_drop_index(struct Parse *parse_context, struct SrcList *index_name_list,
}
struct index *index = space_index(space, index_id);
assert(index != NULL);
- /*
- * If index has been created by user, it has its SQL
- * statement. Otherwise (i.e. PK and UNIQUE indexes,
- * which are created alongside with table) it is NULL.
- */
- if (index->def->opts.sql == NULL) {
- sqlite3ErrorMsg(parse_context, "index associated with UNIQUE "
- "or PRIMARY KEY constraint cannot be dropped",
- 0);
- goto exit_drop_index;
- }
/*
* Generate code to remove entry from _index space
--
2.15.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] [PATCH 6/6] Remove SQL string from index opts
2018-12-09 21:30 [tarantool-patches] [PATCH 0/6] Remove string of SQL statement from opts Nikita Pettik
` (4 preceding siblings ...)
2018-12-09 21:30 ` [tarantool-patches] [PATCH 5/6] sql: don't add string of 'CREATE INDEX ...' to index opts Nikita Pettik
@ 2018-12-09 21:30 ` Nikita Pettik
2018-12-25 13:45 ` [tarantool-patches] Re: [PATCH 0/6] Remove string of SQL statement from opts Kirill Yukhin
6 siblings, 0 replies; 20+ messages in thread
From: Nikita Pettik @ 2018-12-09 21:30 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Closes #2647
---
src/box/alter.cc | 9 ---------
src/box/index_def.c | 20 --------------------
src/box/index_def.h | 9 ---------
src/box/sql/analyze.c | 3 +--
src/box/sql/where.c | 1 -
5 files changed, 1 insertion(+), 41 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 029da029e..c6ee025f2 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -187,15 +187,6 @@ index_opts_decode(struct index_opts *opts, const char *map,
BOX_INDEX_FIELD_OPTS, "distance must be either "\
"'euclid' or 'manhattan'");
}
- if (opts->sql != NULL) {
- char *sql = strdup(opts->sql);
- if (sql == NULL) {
- opts->sql = NULL;
- tnt_raise(OutOfMemory, strlen(opts->sql) + 1, "strdup",
- "sql");
- }
- opts->sql = sql;
- }
if (opts->range_size <= 0) {
tnt_raise(ClientError, ER_WRONG_INDEX_OPTIONS,
BOX_INDEX_FIELD_OPTS,
diff --git a/src/box/index_def.c b/src/box/index_def.c
index 45c74d9ec..2ba57ee9d 100644
--- a/src/box/index_def.c
+++ b/src/box/index_def.c
@@ -46,7 +46,6 @@ const struct index_opts index_opts_default = {
/* .run_size_ratio = */ 3.5,
/* .bloom_fpr = */ 0.05,
/* .lsn = */ 0,
- /* .sql = */ NULL,
/* .stat = */ NULL,
};
@@ -61,7 +60,6 @@ const struct opt_def index_opts_reg[] = {
OPT_DEF("run_size_ratio", OPT_FLOAT, struct index_opts, run_size_ratio),
OPT_DEF("bloom_fpr", OPT_FLOAT, struct index_opts, bloom_fpr),
OPT_DEF("lsn", OPT_INT64, struct index_opts, lsn),
- OPT_DEF("sql", OPT_STRPTR, struct index_opts, sql),
OPT_END,
};
@@ -109,15 +107,6 @@ index_def_new(uint32_t space_id, uint32_t iid, const char *name,
def->space_id = space_id;
def->iid = iid;
def->opts = *opts;
- if (opts->sql != NULL) {
- def->opts.sql = strdup(opts->sql);
- if (def->opts.sql == NULL) {
- diag_set(OutOfMemory, strlen(opts->sql) + 1, "strdup",
- "def->opts.sql");
- index_def_delete(def);
- return NULL;
- }
- }
/* Statistics are initialized separately. */
assert(opts->stat == NULL);
return def;
@@ -148,15 +137,6 @@ index_def_dup(const struct index_def *def)
}
rlist_create(&dup->link);
dup->opts = def->opts;
- if (def->opts.sql != NULL) {
- dup->opts.sql = strdup(def->opts.sql);
- if (dup->opts.sql == NULL) {
- diag_set(OutOfMemory, strlen(def->opts.sql) + 1,
- "strdup", "dup->opts.sql");
- index_def_delete(dup);
- return NULL;
- }
- }
if (def->opts.stat != NULL) {
dup->opts.stat = index_stat_dup(def->opts.stat);
if (dup->opts.stat == NULL) {
diff --git a/src/box/index_def.h b/src/box/index_def.h
index 273b8cb53..7717ecd64 100644
--- a/src/box/index_def.h
+++ b/src/box/index_def.h
@@ -157,10 +157,6 @@ struct index_opts {
* LSN from the time of index creation.
*/
int64_t lsn;
- /**
- * SQL statement that produced this index.
- */
- char *sql;
/**
* SQL specific statistics concerning tuples
* distribution for query planer. It is automatically
@@ -187,7 +183,6 @@ index_opts_create(struct index_opts *opts)
static inline void
index_opts_destroy(struct index_opts *opts)
{
- free(opts->sql);
free(opts->stat);
TRASH(opts);
}
@@ -212,10 +207,6 @@ index_opts_cmp(const struct index_opts *o1, const struct index_opts *o2)
return o1->run_size_ratio < o2->run_size_ratio ? -1 : 1;
if (o1->bloom_fpr != o2->bloom_fpr)
return o1->bloom_fpr < o2->bloom_fpr ? -1 : 1;
- if ((o1->sql == NULL) != (o2->sql == NULL))
- return 1;
- if (o1->sql != NULL)
- return strcmp(o1->sql, o2->sql);
return 0;
}
diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 8f8b6eb5d..51c63fa7a 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1606,8 +1606,7 @@ index_field_tuple_est(const struct index_def *idx_def, uint32_t field)
{
assert(idx_def != NULL);
struct space *space = space_by_id(idx_def->space_id);
- if (space == NULL || (idx_def->opts.sql != NULL &&
- strcmp(idx_def->opts.sql, "fake_autoindex") == 0))
+ if (space == NULL || strcmp(idx_def->name, "fake_autoindex") == 0)
return idx_def->opts.stat->tuple_log_est[field];
assert(field <= idx_def->key_def->part_count);
/* Statistics is held only in real indexes. */
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 9c3462bc0..571b5af78 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -2818,7 +2818,6 @@ tnt_error:
struct index_opts opts;
index_opts_create(&opts);
- opts.sql = "fake_autoindex";
fake_index = index_def_new(pTab->def->id, 0,"fake_autoindex",
sizeof("fake_autoindex") - 1,
TREE, &opts, key_def, NULL);
--
2.15.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] Re: [PATCH 2/6] sql: don't update SQL string during renaming
2018-12-09 21:30 ` [tarantool-patches] [PATCH 2/6] sql: don't update SQL string during renaming Nikita Pettik
@ 2018-12-10 14:16 ` Vladislav Shpilevoy
2018-12-11 18:29 ` n.pettik
0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-10 14:16 UTC (permalink / raw)
To: tarantool-patches, Nikita Pettik
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.
========================================================
commit 3eeb01aa01766001fb313eaf26d539273100b71b
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Mon Dec 10 16:27:04 2018 +0300
Review fixes
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 89e51eb30..d0ce9d893 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -77,78 +77,6 @@ tnt_error:
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 dbf58d967..85adcdb18 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4462,7 +4462,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 e963132fe..0aa31bdd6 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -61,37 +61,15 @@ 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
*
- * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise.
+ * @retval 0 on success, SQL_TARANTOOL_ERROR otherwise.
*/
int
sql_rename_table(uint32_t space_id, const char *new_name);
-/**
- * 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.
- */
-int
-sql_index_update_table_name(struct index_def *idef, const char *new_tbl_name,
- char **sql_stmt);
-
/* Alter trigger statement after rename table. */
int tarantoolSqlite3RenameTrigger(const char *zTriggerName,
const char *zOldName, const char *zNewName);
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] Re: [PATCH 3/6] test: fix sqltester methods to drop all tables/views
2018-12-09 21:30 ` [tarantool-patches] [PATCH 3/6] test: fix sqltester methods to drop all tables/views Nikita Pettik
@ 2018-12-10 14:16 ` Vladislav Shpilevoy
2018-12-11 18:29 ` n.pettik
0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-10 14:16 UTC (permalink / raw)
To: tarantool-patches, Nikita Pettik
Thanks for the patch! See 3 comments below.
On 10/12/2018 00:30, Nikita Pettik wrote:
> Since we are going to remove string of SQL "CREATE TABLE ..." statement
> from space's opts, lets rework methods in sqltester to drop all tables
> and views in order to avoid relying on this parameter.
>
> Part of #2647
> ---
> test/sql-tap/drop_all.test.lua | 4 ++--
> test/sql-tap/identifier_case.test.lua | 6 +++---
> test/sql-tap/lua/sqltester.lua | 38 ++++++++++++++---------------------
> 3 files changed, 20 insertions(+), 28 deletions(-)
>
> diff --git a/test/sql-tap/lua/sqltester.lua b/test/sql-tap/lua/sqltester.lua
> index 8751ef820..672d2e67d 100644
> --- a/test/sql-tap/lua/sqltester.lua
> +++ b/test/sql-tap/lua/sqltester.lua
> @@ -277,7 +277,7 @@ end
> test.catchsql2 = catchsql2
>
> -- Show the VDBE program for an SQL statement but omit the Trace
> --- opcode at the beginning. This procedure can be used to prove
> +-- opcode at the beginning. This procedure can be used to PROVE
1. ???
> -- that different SQL statements generate exactly the same VDBE code.
> local function explain_no_trace(self, sql)
> tr = execsql(self, "EXPLAIN "..sql)
> @@ -288,35 +288,27 @@ local function explain_no_trace(self, sql)
> end
> test.explain_no_trace = explain_no_trace
> json = require("json")
> -function test.find_spaces_by_sql_statement(self, statement)
> - local spaces = box.space._space:select{}
> - local res = {}
> - for _, space in ipairs(spaces) do
> - local name_num = 3-- [3] is space name
> - local options_num = 6-- [6] is space options
> - if not space[name_num]:startswith("_") and space[options_num]["sql"] then
> - if string.find(space[options_num]["sql"], statement) then
> - table.insert(res, space[name_num])
> - end
> - end
> - end
> - return res
> -end
>
> function test.drop_all_tables(self)
> - local tables = test:find_spaces_by_sql_statement("CREATE TABLE")
> - for _, table in ipairs(tables) do
> - test:execsql(string.format([[DROP TABLE "%s";]], table))
> + local entry_count = 0
> + for _, v in box.space._space:pairs() do
> + if v[1] >= 512 then
> + test:execsql(string.format([[DROP TABLE "%s";]], v[3]))
> + entry_count = entry_count + 1
2. Since DD integration is finished, can we do box.space[v3]:drop()
instead of executing SQL? I think it can help us to find more bugs.
And it is faster.
> + end
> end
> - return tables
> + return entry_count
> end
>
> function test.drop_all_views(self)
> - local views = test:find_spaces_by_sql_statement("CREATE VIEW")
> - for _, view in ipairs(views) do
> - test:execsql(string.format([[DROP VIEW "%s";]], view))
> + local entry_count = 0
> + for _, v in box.space._space:pairs() do
> + if v[1] > 512 and v[6]["view"] == true then
3. IMO .view looks better than ["view"], but I do not insist. Up to
you.
> + test:execsql(string.format([[DROP VIEW "%s";]], v[3]))
> + entry_count = entry_count + 1
> + end
> end
> - return views
> + return entry_count
> end
>
> function test.do_select_tests(self, label, tests)
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] Re: [PATCH 1/6] sql: avoid calling sql_encode_table_opts() during trigger creation
2018-12-09 21:30 ` [tarantool-patches] [PATCH 1/6] sql: avoid calling sql_encode_table_opts() during trigger creation Nikita Pettik
@ 2018-12-10 14:17 ` Vladislav Shpilevoy
2018-12-11 18:29 ` n.pettik
0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-10 14:17 UTC (permalink / raw)
To: tarantool-patches, Nikita Pettik
Thanks for the patch! See 2 comments and review fixes below.
On 10/12/2018 00:30, Nikita Pettik wrote:
> At the last stage of trigger creation, trigger's create statement
> ("CREATE TRIGGER ...") is encoded to msgpack. Since only this string is
> only member of a map to be encoded, is makes no sense to call whole
> sql_encode_table_opts() function, which in turn processes table's
> checks, opts for VIEW etc.
>
> Needed for #2647
> ---
> src/box/sql/trigger.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index c38f9cd9d..c7d22b9d0 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -212,19 +212,17 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
> parse->nMem += 3;
> int record = ++parse->nMem;
>
> - uint32_t opts_buff_sz = 0;
> - char *data = sql_encode_table_opts(&fiber()->gc, NULL, sql_str,
> - &opts_buff_sz);
> - sqlite3DbFree(db, sql_str);
> - if (data == NULL) {
> - parse->nErr++;
> - parse->rc = SQL_TARANTOOL_ERROR;
> - goto cleanup;
> - }
> - char *opts_buff = sqlite3DbMallocRaw(db, opts_buff_sz);
> + uint32_t opts_buff_sz = mp_sizeof_map(1) +
> + mp_sizeof_str(strlen("sql")) +
> + mp_sizeof_str(strlen(sql_str));
> + char *opts_buff = (char *) sqlite3DbMallocRaw(db, opts_buff_sz);
> if (opts_buff == NULL)
> goto cleanup;
> - memcpy(opts_buff, data, opts_buff_sz);
> +
> + char *data = mp_encode_map(opts_buff, 1);
> + data = mp_encode_str(data, "sql", strlen("sql"));
> + data = mp_encode_str(data, sql_str, strlen(sql_str));
1. strlen(sql_str) is called twice. Foreboding your claim that
it will be optimized to 1 call - no, it won't. I disassembled this
function and saw double strlen(sql_str). Only strlen("sql") is
optimized to constant 3.
2. After you've deleted this invocation of sql_encode_table_opts(),
struct Table *table in its second parameter became not NULL always.
So I simplified this function a bit.
> + sqlite3DbFree(db, sql_str);
>
> trigger_name = sqlite3DbStrDup(db, trigger_name);
> if (trigger_name == NULL) {
>
My review fixes here and on the branch:
==================================================================
commit c287163409a6a542273d07cb0602834dfa66df88
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Mon Dec 10 16:12:23 2018 +0300
Review fixes
diff --git a/src/box/sql.c b/src/box/sql.c
index 7b41c9926..f43387f18 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1199,16 +1199,13 @@ sql_encode_table_opts(struct region *region, struct Table *table,
bool is_error = false;
mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
set_encode_error, &is_error);
- bool is_view = false;
int checks_cnt = 0;
struct ExprList_item *a;
- if (table != NULL) {
- is_view = table->def->opts.is_view;
- struct ExprList *checks = table->def->opts.checks;
- if (checks != NULL) {
- checks_cnt = checks->nExpr;
- a = checks->a;
- }
+ bool is_view = table->def->opts.is_view;
+ struct ExprList *checks = table->def->opts.checks;
+ if (checks != NULL) {
+ checks_cnt = checks->nExpr;
+ a = checks->a;
}
mpstream_encode_map(&stream, 1 + is_view + (checks_cnt > 0));
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index c7d22b9d0..418dc9147 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -211,17 +211,19 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
int first_col = parse->nMem + 1;
parse->nMem += 3;
int record = ++parse->nMem;
+ int sql_str_len = strlen(sql_str);
+ int sql_len = strlen("sql");
uint32_t opts_buff_sz = mp_sizeof_map(1) +
- mp_sizeof_str(strlen("sql")) +
- mp_sizeof_str(strlen(sql_str));
+ mp_sizeof_str(sql_len) +
+ mp_sizeof_str(sql_str_len);
char *opts_buff = (char *) sqlite3DbMallocRaw(db, opts_buff_sz);
if (opts_buff == NULL)
goto cleanup;
char *data = mp_encode_map(opts_buff, 1);
- data = mp_encode_str(data, "sql", strlen("sql"));
- data = mp_encode_str(data, sql_str, strlen(sql_str));
+ data = mp_encode_str(data, "sql", sql_len);
+ data = mp_encode_str(data, sql_str, sql_str_len);
sqlite3DbFree(db, sql_str);
trigger_name = sqlite3DbStrDup(db, trigger_name);
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] Re: [PATCH 4/6] sql: don't add string of 'CREATE TABLE...' to space opts
2018-12-09 21:30 ` [tarantool-patches] [PATCH 4/6] sql: don't add string of 'CREATE TABLE...' to space opts Nikita Pettik
@ 2018-12-10 14:17 ` Vladislav Shpilevoy
2018-12-11 18:29 ` n.pettik
0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-10 14:17 UTC (permalink / raw)
To: tarantool-patches, Nikita Pettik
Thanks for the patch! See 1 comment and review fixes below.
On 10/12/2018 00:30, Nikita Pettik wrote:
> We don't rely no more on this string anymore and it can be removed for
'no more on this anymore' - please, rephrase. Sounds a little
tautological.
> ordinary tables. However, it is still used to hold SELECT body for view.
>
> Part of #2647
> ---
> src/box/sql.c | 8 +-
> src/box/sql/analyze.c | 2 +-
> src/box/sql/build.c | 242 ++----------------------------------------------
> src/box/sql/pragma.c | 2 -
> src/box/sql/sqliteInt.h | 1 -
> test/sql/upgrade.result | 8 +-
> test/sql/view.result | 3 +
> test/sql/view.test.lua | 1 +
> 8 files changed, 19 insertions(+), 248 deletions(-)
>
My review fix here and on the branch:
====================================================================
commit e4ea3a20c71e73426a9ea2478b55c346b0ca3a41
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Mon Dec 10 16:57:18 2018 +0300
Review fix
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 325c272b0..ce1f4aed7 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -294,7 +294,7 @@ sql_pragma_table_info(struct Parse *parse, const char *tbl_name)
static int
sql_pragma_table_stats(struct space *space, void *data)
{
- if (space->def->opts.sql == NULL || space->def->opts.is_view)
+ if (space->def->opts.is_view)
return 0;
struct Parse *parse = (struct Parse *) data;
struct index *pk = space_index(space, 0);
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] Re: [PATCH 5/6] sql: don't add string of 'CREATE INDEX ...' to index opts
2018-12-09 21:30 ` [tarantool-patches] [PATCH 5/6] sql: don't add string of 'CREATE INDEX ...' to index opts Nikita Pettik
@ 2018-12-10 14:18 ` Vladislav Shpilevoy
2018-12-11 18:29 ` n.pettik
0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-10 14:18 UTC (permalink / raw)
To: tarantool-patches, Nikita Pettik
Thanks for the patch! See 2 comments and review fixes below.
On 10/12/2018 00:30, Nikita Pettik wrote:
> Part of #2647
> ---
> src/box/sql.c | 22 +++-------------------
> src/box/sql/build.c | 35 +++--------------------------------
> 2 files changed, 6 insertions(+), 51 deletions(-)
>
> diff --git a/src/box/sql.c b/src/box/sql.c
> index e79265823..45c539ec7 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1214,7 +1196,9 @@ sql_encode_index_opts(struct region *region, const struct index_opts *opts,
> bool is_error = false;
> mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
> set_encode_error, &is_error);
> - mpstream_encode_index_opts(&stream, opts);
> + mpstream_encode_map(&stream, 1);
> + mpstream_encode_str(&stream, "unique");
> + mpstream_encode_bool(&stream, opts->is_unique);;
> mpstream_flush(&stream);
1. We do not need mpstream for this tiny piece of code. I
refactored it to simple region_alloc.
> if (is_error) {
> diag_set(OutOfMemory, stream.pos - stream.buf, "mpstream_flush",
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index fd1666c6d..ab2a56420 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2546,6 +2527,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
> sqlite3VdbeAddOp1(vdbe, OP_Close, cursor);
> vdbe_emit_create_index(parse, def, index->def,
> def->id, index_id);
> + sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
2. Are you sure that you should increment change counter here?
As I know, it should be incremented once on a DDL, but this
increment is under 'if (tbl_name != NULL)' which means that it
is not a separate index, but part of CREATE TABLE, it is not?
CREATE TABLE increments change counter on insertion into _space
only.
> sqlite3VdbeAddOp0(vdbe, OP_Expire);
> }
>
My review fixes here and on the branch:
=================================================================
commit 65020d36bb3a9c8058789c90ed95665bd847a732
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Mon Dec 10 17:11:26 2018 +0300
Review fixes
diff --git a/src/box/sql.c b/src/box/sql.c
index d270529ae..dd7a7ca4b 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1188,25 +1188,18 @@ char *
sql_encode_index_opts(struct region *region, const struct index_opts *opts,
uint32_t *size)
{
- 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);
- mpstream_encode_map(&stream, 1);
- mpstream_encode_str(&stream, "unique");
- mpstream_encode_bool(&stream, opts->is_unique);;
- mpstream_flush(&stream);
- if (is_error) {
- diag_set(OutOfMemory, stream.pos - stream.buf, "mpstream_flush",
- "stream");
+ int unique_len = strlen("unique");
+ *size = mp_sizeof_map(1) + mp_sizeof_str(unique_len) +
+ mp_sizeof_bool(opts->is_unique);
+ char *mem = (char *) region_alloc(region, *size);
+ if (mem == NULL) {
+ diag_set(OutOfMemory, *size, "region_alloc", "mem");
return NULL;
}
- *size = region_used(region) - used;
- char *raw = region_join(region, *size);
- if (raw == NULL)
- diag_set(OutOfMemory, *size, "region_join", "raw");
- return raw;
+ char *pos = mp_encode_map(mem, 1);
+ pos = mp_encode_str(pos, "unique", unique_len);
+ pos = mp_encode_bool(pos, opts->is_unique);
+ return mem;
}
void
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index ab2a56420..50227232c 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2125,7 +2125,6 @@ table_add_index(struct space *space, struct index *index)
* @param idx_type Index type: non-unique index, unique index,
* index implementing UNIQUE constraint or
* index implementing PK constraint.
- * @param sql_stmt SQL statement, which creates the index.
* @retval 0 on success, -1 on error.
*/
static int
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] Re: [PATCH 1/6] sql: avoid calling sql_encode_table_opts() during trigger creation
2018-12-10 14:17 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-12-11 18:29 ` n.pettik
0 siblings, 0 replies; 20+ messages in thread
From: n.pettik @ 2018-12-11 18:29 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
[-- Attachment #1: Type: text/plain, Size: 2479 bytes --]
> On 10 Dec 2018, at 17:17, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Thanks for the patch! See 2 comments and review fixes below.
>
> On 10/12/2018 00:30, Nikita Pettik wrote:
>> At the last stage of trigger creation, trigger's create statement
>> ("CREATE TRIGGER ...") is encoded to msgpack. Since only this string is
>> only member of a map to be encoded, is makes no sense to call whole
>> sql_encode_table_opts() function, which in turn processes table's
>> checks, opts for VIEW etc.
>> Needed for #2647
>> ---
>> src/box/sql/trigger.c | 20 +++++++++-----------
>> 1 file changed, 9 insertions(+), 11 deletions(-)
>> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
>> index c38f9cd9d..c7d22b9d0 100644
>> --- a/src/box/sql/trigger.c
>> +++ b/src/box/sql/trigger.c
>> @@ -212,19 +212,17 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
>> parse->nMem += 3;
>> int record = ++parse->nMem;
>> - uint32_t opts_buff_sz = 0;
>> - char *data = sql_encode_table_opts(&fiber()->gc, NULL, sql_str,
>> - &opts_buff_sz);
>> - sqlite3DbFree(db, sql_str);
>> - if (data == NULL) {
>> - parse->nErr++;
>> - parse->rc = SQL_TARANTOOL_ERROR;
>> - goto cleanup;
>> - }
>> - char *opts_buff = sqlite3DbMallocRaw(db, opts_buff_sz);
>> + uint32_t opts_buff_sz = mp_sizeof_map(1) +
>> + mp_sizeof_str(strlen("sql")) +
>> + mp_sizeof_str(strlen(sql_str));
>> + char *opts_buff = (char *) sqlite3DbMallocRaw(db, opts_buff_sz);
>> if (opts_buff == NULL)
>> goto cleanup;
>> - memcpy(opts_buff, data, opts_buff_sz);
>> +
>> + char *data = mp_encode_map(opts_buff, 1);
>> + data = mp_encode_str(data, "sql", strlen("sql"));
>> + data = mp_encode_str(data, sql_str, strlen(sql_str));
>
> 1. strlen(sql_str) is called twice. Foreboding your claim that
> it will be optimized to 1 call - no, it won't. I disassembled this
> function and saw double strlen(sql_str). Only strlen("sql") is
> optimized to constant 3.
I’m too lazy to check it, so let it be.
>
> 2. After you've deleted this invocation of sql_encode_table_opts(),
> struct Table *table in its second parameter became not NULL always.
> So I simplified this function a bit.
>
>> + sqlite3DbFree(db, sql_str);
>> trigger_name = sqlite3DbStrDup(db, trigger_name);
>> if (trigger_name == NULL) {
>
> My review fixes here and on the branch:
Ok, applied.
[-- Attachment #2: Type: text/html, Size: 16753 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] Re: [PATCH 2/6] sql: don't update SQL string during renaming
2018-12-10 14:16 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-12-11 18:29 ` n.pettik
2018-12-12 12:36 ` Vladislav Shpilevoy
0 siblings, 1 reply; 20+ messages in thread
From: n.pettik @ 2018-12-11 18:29 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
> On 10 Dec 2018, at 17:16, Vladislav Shpilevoy <v.shpilevoy@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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] Re: [PATCH 3/6] test: fix sqltester methods to drop all tables/views
2018-12-10 14:16 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-12-11 18:29 ` n.pettik
0 siblings, 0 replies; 20+ messages in thread
From: n.pettik @ 2018-12-11 18:29 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
> On 10 Dec 2018, at 17:16, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Thanks for the patch! See 3 comments below.
>
> On 10/12/2018 00:30, Nikita Pettik wrote:
>> Since we are going to remove string of SQL "CREATE TABLE ..." statement
>> from space's opts, lets rework methods in sqltester to drop all tables
>> and views in order to avoid relying on this parameter.
>> Part of #2647
>> ---
>> test/sql-tap/drop_all.test.lua | 4 ++--
>> test/sql-tap/identifier_case.test.lua | 6 +++---
>> test/sql-tap/lua/sqltester.lua | 38 ++++++++++++++---------------------
>> 3 files changed, 20 insertions(+), 28 deletions(-)
>> diff --git a/test/sql-tap/lua/sqltester.lua b/test/sql-tap/lua/sqltester.lua
>> index 8751ef820..672d2e67d 100644
>> --- a/test/sql-tap/lua/sqltester.lua
>> +++ b/test/sql-tap/lua/sqltester.lua
>> @@ -277,7 +277,7 @@ end
>> test.catchsql2 = catchsql2
>> -- Show the VDBE program for an SQL statement but omit the Trace
>> --- opcode at the beginning. This procedure can be used to prove
>> +-- opcode at the beginning. This procedure can be used to PROVE
>
> 1. ???
Oops, fixed:
diff --git a/test/sql-tap/lua/sqltester.lua b/test/sql-tap/lua/sqltester.lua
index 672d2e67d..076510932 100644
--- a/test/sql-tap/lua/sqltester.lua
+++ b/test/sql-tap/lua/sqltester.lua
@@ -277,7 +277,7 @@ end
test.catchsql2 = catchsql2
-- Show the VDBE program for an SQL statement but omit the Trace
--- opcode at the beginning. This procedure can be used to PROVE
+-- opcode at the beginning. This procedure can be used to prove
>
>> -- that different SQL statements generate exactly the same VDBE code.
>> local function explain_no_trace(self, sql)
>> tr = execsql(self, "EXPLAIN "..sql)
>> @@ -288,35 +288,27 @@ local function explain_no_trace(self, sql)
>> end
>> test.explain_no_trace = explain_no_trace
>> json = require("json")
>> -function test.find_spaces_by_sql_statement(self, statement)
>> - local spaces = box.space._space:select{}
>> - local res = {}
>> - for _, space in ipairs(spaces) do
>> - local name_num = 3-- [3] is space name
>> - local options_num = 6-- [6] is space options
>> - if not space[name_num]:startswith("_") and space[options_num]["sql"] then
>> - if string.find(space[options_num]["sql"], statement) then
>> - table.insert(res, space[name_num])
>> - end
>> - end
>> - end
>> - return res
>> -end
>> function test.drop_all_tables(self)
>> - local tables = test:find_spaces_by_sql_statement("CREATE TABLE")
>> - for _, table in ipairs(tables) do
>> - test:execsql(string.format([[DROP TABLE "%s";]], table))
>> + local entry_count = 0
>> + for _, v in box.space._space:pairs() do
>> + if v[1] >= 512 then
>> + test:execsql(string.format([[DROP TABLE "%s";]], v[3]))
>> + entry_count = entry_count + 1
>
> 2. Since DD integration is finished, can we do box.space[v3]:drop()
> instead of executing SQL? I think it can help us to find more bugs.
> And it is faster.
Seems like:
@@ -293,7 +293,7 @@ function test.drop_all_tables(self)
local entry_count = 0
for _, v in box.space._space:pairs() do
if v[1] >= 512 then
- test:execsql(string.format([[DROP TABLE "%s";]], v[3]))
+ box.space[v[3]]:drop()
entry_count = entry_count + 1
end
end
@@ -303,8 +303,8 @@ end
function test.drop_all_views(self)
local entry_count = 0
for _, v in box.space._space:pairs() do
- if v[1] > 512 and v[6]["view"] == true then
- test:execsql(string.format([[DROP VIEW "%s";]], v[3]))
+ if v[1] > 512 and v[6].view == true then
+ box.space[v[3]]:drop()
entry_count = entry_count + 1
end
>
>> + end
>> end
>> - return tables
>> + return entry_count
>> end
>> function test.drop_all_views(self)
>> - local views = test:find_spaces_by_sql_statement("CREATE VIEW")
>> - for _, view in ipairs(views) do
>> - test:execsql(string.format([[DROP VIEW "%s";]], view))
>> + local entry_count = 0
>> + for _, v in box.space._space:pairs() do
>> + if v[1] > 512 and v[6]["view"] == true then
>
> 3. IMO .view looks better than ["view"], but I do not insist. Up to
> you.
I don’t really care, so let's use your approach (see above).
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] Re: [PATCH 4/6] sql: don't add string of 'CREATE TABLE...' to space opts
2018-12-10 14:17 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-12-11 18:29 ` n.pettik
0 siblings, 0 replies; 20+ messages in thread
From: n.pettik @ 2018-12-11 18:29 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
> On 10 Dec 2018, at 17:17, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Thanks for the patch! See 1 comment and review fixes below.
>
> On 10/12/2018 00:30, Nikita Pettik wrote:
>> We don't rely no more on this string anymore and it can be removed for
>
> 'no more on this anymore' - please, rephrase. Sounds a little
> tautological.
Rephrased:
We don't rely on this string anymore and it can be removed for ordinary
tables. However, it is still used to hold SELECT body for view.
>
>> ordinary tables. However, it is still used to hold SELECT body for view.
>> Part of #2647
>> ---
>> src/box/sql.c | 8 +-
>> src/box/sql/analyze.c | 2 +-
>> src/box/sql/build.c | 242 ++----------------------------------------------
>> src/box/sql/pragma.c | 2 -
>> src/box/sql/sqliteInt.h | 1 -
>> test/sql/upgrade.result | 8 +-
>> test/sql/view.result | 3 +
>> test/sql/view.test.lua | 1 +
>> 8 files changed, 19 insertions(+), 248 deletions(-)
>
> My review fix here and on the branch:
Applied.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] Re: [PATCH 5/6] sql: don't add string of 'CREATE INDEX ...' to index opts
2018-12-10 14:18 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-12-11 18:29 ` n.pettik
0 siblings, 0 replies; 20+ messages in thread
From: n.pettik @ 2018-12-11 18:29 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
> On 10 Dec 2018, at 17:18, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Thanks for the patch! See 2 comments and review fixes below.
>
> On 10/12/2018 00:30, Nikita Pettik wrote:
>> Part of #2647
>> ---
>> src/box/sql.c | 22 +++-------------------
>> src/box/sql/build.c | 35 +++--------------------------------
>> 2 files changed, 6 insertions(+), 51 deletions(-)
>> diff --git a/src/box/sql.c b/src/box/sql.c
>> index e79265823..45c539ec7 100644
>> --- a/src/box/sql.c
>> +++ b/src/box/sql.c
>> @@ -1214,7 +1196,9 @@ sql_encode_index_opts(struct region *region, const struct index_opts *opts,
>> bool is_error = false;
>> mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
>> set_encode_error, &is_error);
>> - mpstream_encode_index_opts(&stream, opts);
>> + mpstream_encode_map(&stream, 1);
>> + mpstream_encode_str(&stream, "unique");
>> + mpstream_encode_bool(&stream, opts->is_unique);;
>> mpstream_flush(&stream);
>
> 1. We do not need mpstream for this tiny piece of code. I
> refactored it to simple region_alloc.
Ok, I don’t mind.
>> if (is_error) {
>> diag_set(OutOfMemory, stream.pos - stream.buf, "mpstream_flush",
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index fd1666c6d..ab2a56420 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -2546,6 +2527,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
>> sqlite3VdbeAddOp1(vdbe, OP_Close, cursor);
>> vdbe_emit_create_index(parse, def, index->def,
>> def->id, index_id);
>> + sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
>
> 2. Are you sure that you should increment change counter here?
> As I know, it should be incremented once on a DDL, but this
> increment is under 'if (tbl_name != NULL)' which means that it
> is not a separate index, but part of CREATE TABLE, it is not?
> CREATE TABLE increments change counter on insertion into _space
> only.
tbl_name != NULL means that currently we are processing
CREATE INDEX statement, not CREATE TABLE. So, its ok.
>
>> sqlite3VdbeAddOp0(vdbe, OP_Expire);
>> }
>>
>
> My review fixes here and on the branch:
Applied.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] Re: [PATCH 2/6] sql: don't update SQL string during renaming
2018-12-11 18:29 ` n.pettik
@ 2018-12-12 12:36 ` Vladislav Shpilevoy
2018-12-13 12:42 ` n.pettik
0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-12 12:36 UTC (permalink / raw)
To: n.pettik, tarantool-patches
On 11/12/2018 21:29, n.pettik wrote:
>
>
>> On 10 Dec 2018, at 17:16, Vladislav Shpilevoy <v.shpilevoy@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@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;
}
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] Re: [PATCH 2/6] sql: don't update SQL string during renaming
2018-12-12 12:36 ` Vladislav Shpilevoy
@ 2018-12-13 12:42 ` n.pettik
0 siblings, 0 replies; 20+ messages in thread
From: n.pettik @ 2018-12-13 12:42 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
> On 12 Dec 2018, at 15:36, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> On 11/12/2018 21:29, n.pettik wrote:
>>> On 10 Dec 2018, at 17:16, Vladislav Shpilevoy <v.shpilevoy@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@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;
> }
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tarantool-patches] Re: [PATCH 0/6] Remove string of SQL statement from opts
2018-12-09 21:30 [tarantool-patches] [PATCH 0/6] Remove string of SQL statement from opts Nikita Pettik
` (5 preceding siblings ...)
2018-12-09 21:30 ` [tarantool-patches] [PATCH 6/6] Remove SQL string from " Nikita Pettik
@ 2018-12-25 13:45 ` Kirill Yukhin
6 siblings, 0 replies; 20+ messages in thread
From: Kirill Yukhin @ 2018-12-25 13:45 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Hello,
On 10 Dec 00:30, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/tree/np/gh-2647-remove-sql-from-opts
> Issue: https://github.com/tarantool/tarantool/issues/2647
>
> After DD integration was finished, there is no need to add string
> of SQL statement to space or index opts (except for VIEWs). So,
> current patch-set completely removes SQL from index opts, and
> removes SQL from space opts for ordinary spaces.
>
> Nikita Pettik (6):
> sql: avoid calling sql_encode_table_opts() during trigger creation
> sql: don't update SQL string during renaming
> test: fix sqltester methods to drop all tables/views
> sql: don't add string of 'CREATE TABLE...' to space opts
> sql: don't add string of 'CREATE INDEX ...' to index opts
> Remove SQL string from index opts
I've checked your patch set into 2.1 branch.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-12-25 13:45 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-09 21:30 [tarantool-patches] [PATCH 0/6] Remove string of SQL statement from opts Nikita Pettik
2018-12-09 21:30 ` [tarantool-patches] [PATCH 1/6] sql: avoid calling sql_encode_table_opts() during trigger creation Nikita Pettik
2018-12-10 14:17 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-11 18:29 ` n.pettik
2018-12-09 21:30 ` [tarantool-patches] [PATCH 2/6] sql: don't update SQL string during renaming Nikita Pettik
2018-12-10 14:16 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-11 18:29 ` n.pettik
2018-12-12 12:36 ` Vladislav Shpilevoy
2018-12-13 12:42 ` n.pettik
2018-12-09 21:30 ` [tarantool-patches] [PATCH 3/6] test: fix sqltester methods to drop all tables/views Nikita Pettik
2018-12-10 14:16 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-11 18:29 ` n.pettik
2018-12-09 21:30 ` [tarantool-patches] [PATCH 4/6] sql: don't add string of 'CREATE TABLE...' to space opts Nikita Pettik
2018-12-10 14:17 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-11 18:29 ` n.pettik
2018-12-09 21:30 ` [tarantool-patches] [PATCH 5/6] sql: don't add string of 'CREATE INDEX ...' to index opts Nikita Pettik
2018-12-10 14:18 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-11 18:29 ` n.pettik
2018-12-09 21:30 ` [tarantool-patches] [PATCH 6/6] Remove SQL string from " Nikita Pettik
2018-12-25 13:45 ` [tarantool-patches] Re: [PATCH 0/6] Remove string of SQL statement from opts Kirill Yukhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox