* [tarantool-patches] [PATCH v1 1/1] sql: remove struct Enc
@ 2018-08-17 12:24 Kirill Shcherbatov
2018-08-17 12:40 ` [tarantool-patches] " Vladislav Shpilevoy
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Kirill Shcherbatov @ 2018-08-17 12:24 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov
We shouldn't use legacy Enc structure in SQL as we have
featured class mpstream in tarantool core. With this path
all allocations temporally commited with mpstream initialized
on region and then duplicated in dynamic persistent memory via
sqlite3DbMallocRaw.
Closes #3545.
---
Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3545-remove-enc-struct
Issue: https://github.com/tarantool/tarantool/issues/3545
src/box/sql.c | 412 ++++++++++++++++++++++++---------------------
src/box/sql/build.c | 146 +++++++++-------
src/box/sql/tarantoolInt.h | 82 ++++++---
src/box/sql/trigger.c | 24 +--
src/lua/msgpack.c | 18 ++
5 files changed, 391 insertions(+), 291 deletions(-)
diff --git a/src/box/sql.c b/src/box/sql.c
index ae12cae..f1e860d 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -56,6 +56,8 @@
#include "xrow.h"
#include "iproto_constants.h"
#include "fkey.h"
+#include "lua/msgpack.h"
+#include "small/region.h"
static sqlite3 *db = NULL;
@@ -1190,66 +1192,6 @@ void tarantoolSqlite3LoadSchema(struct init_data *init)
*/
/*
- * Resulting data is of the variable length. Routines are called twice:
- * 1. with a NULL buffer, yielding result size estimation;
- * 2. with a buffer of the estimated size, rendering the result.
- *
- * For convenience, formatting routines use Enc structure to call
- * Enc is either configured to perform size estimation
- * or to render the result.
- */
-struct Enc {
- char *(*encode_uint)(char *data, uint64_t num);
- char *(*encode_str)(char *data, const char *str, uint32_t len);
- char *(*encode_bool)(char *data, bool v);
- char *(*encode_array)(char *data, uint32_t len);
- char *(*encode_map)(char *data, uint32_t len);
-};
-
-/* no_encode_XXX functions estimate result size */
-
-static char *no_encode_uint(char *data, uint64_t num)
-{
- /* MsgPack UINT is encoded in 9 bytes or less */
- (void)num; return data + 9;
-}
-
-static char *no_encode_str(char *data, const char *str, uint32_t len)
-{
- /* MsgPack STR header is encoded in 5 bytes or less, followed by
- * the string data. */
- (void)str; return data + 5 + len;
-}
-
-static char *no_encode_bool(char *data, bool v)
-{
- /* MsgPack BOOL is encoded in 1 byte. */
- (void)v; return data + 1;
-}
-
-static char *no_encode_array_or_map(char *data, uint32_t len)
-{
- /* MsgPack ARRAY or MAP header is encoded in 5 bytes or less. */
- (void)len; return data + 5;
-}
-
-/*
- * If buf==NULL, return Enc that will perform size estimation;
- * otherwize, return Enc that renders results in the provided buf.
- */
-static const struct Enc *get_enc(void *buf)
-{
- static const struct Enc mp_enc = {
- mp_encode_uint, mp_encode_str, mp_encode_bool,
- mp_encode_array, mp_encode_map
- }, no_enc = {
- no_encode_uint, no_encode_str, no_encode_bool,
- no_encode_array_or_map, no_encode_array_or_map
- };
- return buf ? &mp_enc : &no_enc;
-}
-
-/*
* Convert SQLite affinity value to the corresponding Tarantool type
* string which is suitable for _index.parts field.
*/
@@ -1278,34 +1220,38 @@ static const char *convertSqliteAffinity(int affinity, bool allow_nulls)
}
}
-/*
- * Render "format" array for _space entry.
- * Returns result size.
- * If buf==NULL estimate result size.
- *
- * Ex: [{"name": "col1", "type": "integer"}, ... ]
- */
-int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
+static void
+set_encode_error(void *error_ctx)
{
- const struct Enc *enc = get_enc(buf);
- const struct space_def *def = pTable->def;
- assert(def != NULL);
- struct SqliteIndex *pk_idx = sqlite3PrimaryKeyIndex(pTable);
- int pk_forced_int = -1;
- char *base = buf, *p;
- int i, n = def->field_count;
+ bool *is_error = error_ctx;
+ *is_error = true;
+}
- p = enc->encode_array(base, n);
+char *
+sql_encode_table(struct region *region, struct Table *table, 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);
- /* If table's PK is single column which is INTEGER, then
- * treat it as strict type, not affinity. */
+ const struct space_def *def = table->def;
+ assert(def != NULL);
+ /*
+ * If table's PK is single column which is INTEGER, then
+ * treat it as strict type, not affinity.
+ */
+ struct SqliteIndex *pk_idx = sqlite3PrimaryKeyIndex(table);
+ uint32_t pk_forced_int = UINT32_MAX;
if (pk_idx != NULL && pk_idx->def->key_def->part_count == 1) {
int pk = pk_idx->def->key_def->parts[0].fieldno;
if (def->fields[pk].type == FIELD_TYPE_INTEGER)
pk_forced_int = pk;
}
-
- for (i = 0; i < n; i++) {
+ uint32_t field_count = def->field_count;
+ luamp_encode_array(luaL_msgpack_default, &stream, field_count);
+ for (uint32_t i = 0; i < field_count && !is_error; i++) {
const char *t;
uint32_t cid = def->fields[i].coll_id;
struct field_def *field = &def->fields[i];
@@ -1315,10 +1261,13 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
base_len += 1;
if (default_str != NULL)
base_len += 1;
- p = enc->encode_map(p, base_len);
- p = enc->encode_str(p, "name", 4);
- p = enc->encode_str(p, field->name, strlen(field->name));
- p = enc->encode_str(p, "type", 4);
+ luamp_encode_map(luaL_msgpack_default, &stream, base_len);
+ luamp_encode_str(luaL_msgpack_default, &stream, "name",
+ strlen("name"));
+ luamp_encode_str(luaL_msgpack_default, &stream, field->name,
+ strlen(field->name));
+ luamp_encode_str(luaL_msgpack_default, &stream, "type",
+ strlen("type"));
if (i == pk_forced_int) {
t = "integer";
} else {
@@ -1329,108 +1278,164 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
}
assert(def->fields[i].is_nullable ==
- action_is_nullable(def->fields[i].nullable_action));
- p = enc->encode_str(p, t, strlen(t));
- p = enc->encode_str(p, "affinity", 8);
- p = enc->encode_uint(p, def->fields[i].affinity);
- p = enc->encode_str(p, "is_nullable", 11);
- p = enc->encode_bool(p, def->fields[i].is_nullable);
- p = enc->encode_str(p, "nullable_action", 15);
+ action_is_nullable(def->fields[i].
+ nullable_action));
+ luamp_encode_str(luaL_msgpack_default, &stream, t, strlen(t));
+ luamp_encode_str(luaL_msgpack_default, &stream, "affinity",
+ strlen("affinity"));
+ luamp_encode_uint(luaL_msgpack_default, &stream,
+ def->fields[i].affinity);
+ luamp_encode_str(luaL_msgpack_default, &stream, "is_nullable",
+ strlen("is_nullable"));
+ luamp_encode_bool(luaL_msgpack_default, &stream,
+ def->fields[i].is_nullable);
+ luamp_encode_str(luaL_msgpack_default, &stream,
+ "nullable_action", strlen("nullable_action"));
assert(def->fields[i].nullable_action < on_conflict_action_MAX);
const char *action =
on_conflict_action_strs[def->fields[i].nullable_action];
- p = enc->encode_str(p, action, strlen(action));
+ luamp_encode_str(luaL_msgpack_default, &stream, action,
+ strlen(action));
if (cid != COLL_NONE) {
- p = enc->encode_str(p, "collation", strlen("collation"));
- p = enc->encode_uint(p, cid);
+ luamp_encode_str(luaL_msgpack_default, &stream,
+ "collation", strlen("collation"));
+ luamp_encode_uint(luaL_msgpack_default, &stream, cid);
}
if (default_str != NULL) {
- p = enc->encode_str(p, "default", strlen("default"));
- p = enc->encode_str(p, default_str, strlen(default_str));
+ luamp_encode_str(luaL_msgpack_default, &stream,
+ "default", strlen("default"));
+ luamp_encode_str(luaL_msgpack_default, &stream,
+ default_str, strlen(default_str));
}
}
- return (int)(p - base);
+ mpstream_flush(&stream);
+ if (is_error)
+ return NULL;
+ uint32_t sz = region_used(region) - used;
+ char *raw = region_join(region, sz);
+ if (raw == NULL)
+ diag_set(OutOfMemory, sz, "region_join", "raw");
+ else
+ *size = sz;
+ return raw;
}
-/*
- * Format "opts" dictionary for _space entry.
- * Returns result size.
- * If buf==NULL estimate result size.
- *
- * Ex: {"temporary": true, "sql": "CREATE TABLE student (name, grade)"}
- */
-int
-tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, char *buf)
+char *
+sql_encode_table_opts(struct region *region, struct Table *table,
+ const char *sql, uint32_t *size)
{
- const struct Enc *enc = get_enc(buf);
- bool is_view = pTable != NULL && pTable->def->opts.is_view;
- bool has_checks = pTable != NULL && pTable->def->opts.checks != NULL;
- int checks_cnt = has_checks ? pTable->def->opts.checks->nExpr : 0;
- char *p = enc->encode_map(buf, 1 + is_view + (checks_cnt > 0));
-
- p = enc->encode_str(p, "sql", 3);
- p = enc->encode_str(p, zSql, strlen(zSql));
+ assert(sql != NULL);
+ 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);
+ bool is_view = false;
+ bool has_checks = false;
+ if (table != NULL) {
+ is_view = table->def->opts.is_view;
+ has_checks = table->def->opts.checks != NULL;
+ }
+ uint32_t checks_cnt = has_checks ? table->def->opts.checks->nExpr : 0;
+ luamp_encode_map(luaL_msgpack_default, &stream,
+ 1 + is_view + (checks_cnt > 0));
+
+ luamp_encode_str(luaL_msgpack_default, &stream, "sql", strlen("sql"));
+ luamp_encode_str(luaL_msgpack_default, &stream, sql, strlen(sql));
if (is_view) {
- p = enc->encode_str(p, "view", 4);
- p = enc->encode_bool(p, true);
+ luamp_encode_str(luaL_msgpack_default, &stream, "view",
+ strlen("view"));
+ luamp_encode_bool(luaL_msgpack_default, &stream, true);
}
if (checks_cnt == 0)
- return p - buf;
+ goto finish;
+
/* Encode checks. */
- struct ExprList_item *a = pTable->def->opts.checks->a;
- p = enc->encode_str(p, "checks", 6);
- p = enc->encode_array(p, checks_cnt);
- for (int i = 0; i < checks_cnt; ++i) {
+ struct ExprList_item *a = table->def->opts.checks->a;
+ luamp_encode_str(luaL_msgpack_default, &stream, "checks",
+ strlen("checks"));
+ luamp_encode_array(luaL_msgpack_default, &stream, checks_cnt);
+ for (uint32_t i = 0; i < checks_cnt && !is_error; ++i) {
int items = (a[i].pExpr != NULL) + (a[i].zName != NULL);
- p = enc->encode_map(p, items);
+ luamp_encode_map(luaL_msgpack_default, &stream, items);
/*
* a[i].pExpr could be NULL for VIEW column names
* represented as checks.
*/
if (a[i].pExpr != NULL) {
- Expr *pExpr = a[i].pExpr;
+ struct Expr *pExpr = a[i].pExpr;
assert(pExpr->u.zToken != NULL);
- p = enc->encode_str(p, "expr", 4);
- p = enc->encode_str(p, pExpr->u.zToken,
- strlen(pExpr->u.zToken));
+ luamp_encode_str(luaL_msgpack_default, &stream, "expr",
+ strlen("expr"));
+ luamp_encode_str(luaL_msgpack_default, &stream,
+ pExpr->u.zToken,
+ strlen(pExpr->u.zToken));
}
if (a[i].zName != NULL) {
- p = enc->encode_str(p, "name", 4);
- p = enc->encode_str(p, a[i].zName, strlen(a[i].zName));
+ luamp_encode_str(luaL_msgpack_default, &stream, "name",
+ strlen("name"));
+ luamp_encode_str(luaL_msgpack_default, &stream,
+ a[i].zName, strlen(a[i].zName));
}
}
- return p - buf;
+
+finish:
+ mpstream_flush(&stream);
+ if (is_error)
+ return NULL;
+ uint32_t sz = region_used(region) - used;
+ char *raw = region_join(region, sz);
+ if (raw == NULL)
+ diag_set(OutOfMemory, sz, "region_join", "raw");
+ else
+ *size = sz;
+ return raw;
}
-int
-fkey_encode_links(const struct fkey_def *def, int type, char *buf)
+char *
+fkey_encode_links(struct region *region, const struct fkey_def *def, int type,
+ uint32_t *size)
{
- const struct Enc *enc = get_enc(buf);
- char *p = enc->encode_array(buf, def->field_count);
- for (uint32_t i = 0; i < def->field_count; ++i)
- p = enc->encode_uint(p, def->links[i].fields[type]);
- return p - buf;
+ 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);
+ uint32_t field_count = def->field_count;
+ luamp_encode_array(luaL_msgpack_default, &stream, field_count);
+ for (uint32_t i = 0; i < field_count && !is_error; ++i) {
+ luamp_encode_uint(luaL_msgpack_default, &stream,
+ def->links[i].fields[type]);
+ }
+ mpstream_flush(&stream);
+ if (is_error)
+ return NULL;
+ uint32_t sz = region_used(region) - used;
+ char *raw = region_join(region, sz);
+ if (raw == NULL)
+ diag_set(OutOfMemory, sz, "region_join", "raw");
+ else
+ *size = sz;
+ return raw;
}
-/*
- * Format "parts" array for _index entry.
- * Returns result size.
- * If buf==NULL estimate result size.
- *
- * Ex: [[0, "integer"]]
- */
-int tarantoolSqlite3MakeIdxParts(SqliteIndex *pIndex, void *buf)
+char *
+sql_encode_index_parts(struct region *region, struct SqliteIndex *index,
+ uint32_t *size)
{
- struct field_def *fields = pIndex->pTable->def->fields;
- struct key_def *key_def = pIndex->def->key_def;
- const struct Enc *enc = get_enc(buf);
- char *base = buf;
+ 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);
+ /*
+ * If table's PK is single column which is INTEGER, then
+ * treat it as strict type, not affinity.
+ */
uint32_t pk_forced_int = UINT32_MAX;
struct SqliteIndex *primary_index =
- sqlite3PrimaryKeyIndex(pIndex->pTable);
-
- /* If table's PK is single column which is INTEGER, then
- * treat it as strict type, not affinity. */
+ sqlite3PrimaryKeyIndex(index->pTable);
+ struct field_def *fields = index->pTable->def->fields;
if (primary_index->def->key_def->part_count == 1) {
int pk = primary_index->def->key_def->parts[0].fieldno;
if (fields[pk].type == FIELD_TYPE_INTEGER)
@@ -1443,9 +1448,11 @@ int tarantoolSqlite3MakeIdxParts(SqliteIndex *pIndex, void *buf)
* primary key columns. Query planner depends on this particular
* data layout.
*/
+ struct key_def *key_def = index->def->key_def;
+ uint32_t part_count = key_def->part_count;
struct key_part *part = key_def->parts;
- char *p = enc->encode_array(base, key_def->part_count);
- for (uint32_t i = 0; i < key_def->part_count; ++i, ++part) {
+ luamp_encode_array(luaL_msgpack_default, &stream, part_count);
+ for (uint32_t i = 0; i < part_count; ++i, ++part) {
uint32_t col = part->fieldno;
assert(fields[col].is_nullable ==
action_is_nullable(fields[col].nullable_action));
@@ -1456,54 +1463,65 @@ int tarantoolSqlite3MakeIdxParts(SqliteIndex *pIndex, void *buf)
t = convertSqliteAffinity(fields[col].affinity,
fields[col].is_nullable);
}
- /* do not decode default collation */
+ /* Do not decode default collation. */
uint32_t cid = part->coll_id;
- p = enc->encode_map(p, cid == COLL_NONE ? 5 : 6);
- p = enc->encode_str(p, "type", sizeof("type")-1);
- p = enc->encode_str(p, t, strlen(t));
- p = enc->encode_str(p, "field", sizeof("field")-1);
- p = enc->encode_uint(p, col);
+ luamp_encode_map(luaL_msgpack_default, &stream,
+ 5 + (cid != COLL_NONE));
+ luamp_encode_str(luaL_msgpack_default, &stream, "type",
+ strlen("type"));
+ luamp_encode_str(luaL_msgpack_default, &stream, t, strlen(t));
+ luamp_encode_str(luaL_msgpack_default, &stream, "field",
+ strlen("field"));
+ luamp_encode_uint(luaL_msgpack_default, &stream, col);
if (cid != COLL_NONE) {
- p = enc->encode_str(p, "collation",
- sizeof("collation") - 1);
- p = enc->encode_uint(p, cid);
+ luamp_encode_str(luaL_msgpack_default, &stream,
+ "collation", strlen("collation"));
+ luamp_encode_uint(luaL_msgpack_default, &stream, cid);
}
- p = enc->encode_str(p, "is_nullable", 11);
- p = enc->encode_bool(p, fields[col].is_nullable);
- p = enc->encode_str(p, "nullable_action", 15);
+ luamp_encode_str(luaL_msgpack_default, &stream, "is_nullable",
+ strlen("is_nullable"));
+ luamp_encode_bool(luaL_msgpack_default, &stream,
+ fields[col].is_nullable);
+ luamp_encode_str(luaL_msgpack_default, &stream,
+ "nullable_action", strlen("nullable_action"));
const char *action_str =
on_conflict_action_strs[fields[col].nullable_action];
- p = enc->encode_str(p, action_str, strlen(action_str));
+ luamp_encode_str(luaL_msgpack_default, &stream, action_str,
+ strlen(action_str));
- p = enc->encode_str(p, "sort_order", 10);
+ luamp_encode_str(luaL_msgpack_default, &stream, "sort_order",
+ strlen("sort_order"));
enum sort_order sort_order = part->sort_order;
assert(sort_order < sort_order_MAX);
const char *sort_order_str = sort_order_strs[sort_order];
- p = enc->encode_str(p, sort_order_str, strlen(sort_order_str));
+ luamp_encode_str(luaL_msgpack_default, &stream, sort_order_str,
+ strlen(sort_order_str));
}
- return p - base;
+ mpstream_flush(&stream);
+ if (is_error)
+ return NULL;
+ uint32_t sz = region_used(region) - used;
+ char *raw = region_join(region, sz);
+ if (raw == NULL)
+ diag_set(OutOfMemory, sz, "region_join", "raw");
+ else
+ *size = sz;
+ return raw;
}
-/*
- * Format "opts" dictionary for _index entry.
- * Returns result size.
- * If buf==NULL estimate result size.
- *
- * Ex: {
- * "unique": "true",
- * "sql": "CREATE INDEX student_by_name ON students(name)"
- * }
- */
-int tarantoolSqlite3MakeIdxOpts(SqliteIndex *index, const char *zSql, void *buf)
+char *
+sql_encode_index_opts(struct region *region, struct SqliteIndex *index,
+ const char *sql, uint32_t *size)
{
- const struct Enc *enc = get_enc(buf);
- char *base = buf, *p;
-
- (void)index;
-
- p = enc->encode_map(base, 2);
+ 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);
+ luamp_encode_map(luaL_msgpack_default, &stream, 2);
/* Mark as unique pk and unique indexes */
- p = enc->encode_str(p, "unique", 6);
+ luamp_encode_str(luaL_msgpack_default, &stream, "unique",
+ strlen("unique"));
/* If user didn't defined ON CONFLICT OPTIONS, all uniqueness checks
* will be made by Tarantool. However, Tarantool doesn't have ON
* CONFLIT option, so in that case (except ON CONFLICT ABORT, which is
@@ -1511,10 +1529,20 @@ int tarantoolSqlite3MakeIdxOpts(SqliteIndex *index, const char *zSql, void *buf)
* INSERT OR REPLACE/IGNORE uniqueness checks will be also done by
* Tarantool.
*/
- p = enc->encode_bool(p, IsUniqueIndex(index));
- p = enc->encode_str(p, "sql", 3);
- p = enc->encode_str(p, zSql, zSql ? strlen(zSql) : 0);
- return (int)(p - base);
+ luamp_encode_bool(luaL_msgpack_default, &stream, IsUniqueIndex(index));
+ luamp_encode_str(luaL_msgpack_default, &stream, "sql", strlen("sql"));
+ luamp_encode_str(luaL_msgpack_default, &stream, sql,
+ sql != NULL ? strlen(sql) : 0);
+ mpstream_flush(&stream);
+ if (is_error)
+ return NULL;
+ uint32_t sz = region_used(region) - used;
+ char *raw = region_join(region, sz);
+ if (raw == NULL)
+ diag_set(OutOfMemory, sz, "region_join", "raw");
+ else
+ *size = sz;
+ return raw;
}
void
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index dddeb12..ca4ba44 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1241,25 +1241,30 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId,
Vdbe *v = sqlite3GetVdbe(pParse);
int iFirstCol = ++pParse->nMem;
int iRecord = (pParse->nMem += 6); /* 6 total columns */
- char *zOpts, *zParts;
- int zOptsSz, zPartsSz;
-
- /* Format "opts" and "parts" for _index entry. */
- zOpts = sqlite3DbMallocRaw(pParse->db,
- tarantoolSqlite3MakeIdxOpts(pIndex, zSql,
- NULL) +
- tarantoolSqlite3MakeIdxParts(pIndex,
- NULL) + 2);
- if (!zOpts)
+ char *index_opts = NULL, *index_parts = NULL;
+ uint32_t index_opts_sz = 0, index_parts_sz = 0;
+ struct region *region = &fiber()->gc;
+ index_opts =
+ sql_encode_index_opts(region, pIndex, zSql, &index_opts_sz);
+ if (index_opts != NULL) {
+ index_parts =
+ sql_encode_index_parts(region, pIndex, &index_parts_sz);
+ }
+ if (index_opts == NULL || index_parts == NULL) {
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
return;
- zOptsSz = tarantoolSqlite3MakeIdxOpts(pIndex, zSql, zOpts);
- zParts = zOpts + zOptsSz + 1;
- zPartsSz = tarantoolSqlite3MakeIdxParts(pIndex, zParts);
-#if SQLITE_DEBUG
- /* NUL-termination is necessary for VDBE trace facility only */
- zOpts[zOptsSz] = 0;
- zParts[zPartsSz] = 0;
-#endif
+ }
+ char *raw = sqlite3DbMallocRaw(pParse->db,
+ index_opts_sz + index_parts_sz);
+ if (raw == NULL)
+ return;
+
+ memcpy(raw, index_opts, index_opts_sz);
+ index_opts = raw;
+ raw += index_opts_sz;
+ memcpy(raw, index_parts, index_parts_sz);
+ index_parts = raw;
if (pParse->pNewTable) {
int reg;
@@ -1295,11 +1300,11 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId,
P4_DYNAMIC);
sqlite3VdbeAddOp4(v, OP_String8, 0, iFirstCol + 3, 0, "tree",
P4_STATIC);
- sqlite3VdbeAddOp4(v, OP_Blob, zOptsSz, iFirstCol + 4,
- SQL_SUBTYPE_MSGPACK, zOpts, P4_DYNAMIC);
+ sqlite3VdbeAddOp4(v, OP_Blob, index_opts_sz, iFirstCol + 4,
+ SQL_SUBTYPE_MSGPACK, index_opts, P4_DYNAMIC);
/* zOpts and zParts are co-located, hence STATIC */
- sqlite3VdbeAddOp4(v, OP_Blob, zPartsSz, iFirstCol + 5,
- SQL_SUBTYPE_MSGPACK,zParts, P4_STATIC);
+ sqlite3VdbeAddOp4(v, OP_Blob, index_parts_sz, iFirstCol + 5,
+ SQL_SUBTYPE_MSGPACK, index_parts, P4_STATIC);
sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 6, iRecord);
sqlite3VdbeAddOp2(v, OP_SInsert, BOX_INDEX_ID, iRecord);
if (pIndex->index_type == SQL_INDEX_TYPE_NON_UNIQUE ||
@@ -1359,47 +1364,50 @@ makeIndexSchemaRecord(Parse * pParse,
static void
createSpace(Parse * pParse, int iSpaceId, char *zStmt)
{
- Table *p = pParse->pNewTable;
+ struct Table *table = pParse->pNewTable;
Vdbe *v = sqlite3GetVdbe(pParse);
int iFirstCol = ++pParse->nMem;
int iRecord = (pParse->nMem += 7);
- char *zOpts, *zFormat;
- int zOptsSz, zFormatSz;
-
- zOpts = sqlite3DbMallocRaw(pParse->db,
- tarantoolSqlite3MakeTableFormat(p, NULL) +
- tarantoolSqlite3MakeTableOpts(p, zStmt,
- NULL) + 2);
- if (!zOpts) {
- zOptsSz = 0;
- zFormat = NULL;
- zFormatSz = 0;
- } else {
- zOptsSz = tarantoolSqlite3MakeTableOpts(p, zStmt, zOpts);
- zFormat = zOpts + zOptsSz + 1;
- zFormatSz = tarantoolSqlite3MakeTableFormat(p, zFormat);
-#if SQLITE_DEBUG
- /* NUL-termination is necessary for VDBE-tracing facility only */
- zOpts[zOptsSz] = 0;
- zFormat[zFormatSz] = 0;
-#endif
+ uint32_t table_stmt_sz = 0, table_opts_stmt_sz = 0;
+ char *table_stmt = NULL, *table_opts_stmt = NULL;
+
+ struct region *region = &fiber()->gc;
+ table_opts_stmt =
+ sql_encode_table_opts(region, table, zStmt, &table_opts_stmt_sz);
+ if (table_opts_stmt != NULL)
+ table_stmt = sql_encode_table(region, table, &table_stmt_sz);
+ if (table_opts_stmt == NULL || table_stmt == NULL) {
+ pParse->nErr++;
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ return;
}
+ char *raw = sqlite3DbMallocRaw(pParse->db,
+ table_stmt_sz + table_opts_stmt_sz);
+ if (raw == NULL)
+ return;
+
+ memcpy(raw, table_opts_stmt, table_opts_stmt_sz);
+ table_opts_stmt = raw;
+ raw += table_opts_stmt_sz;
+ memcpy(raw, table_stmt, table_stmt_sz);
+ table_stmt = raw;
sqlite3VdbeAddOp2(v, OP_SCopy, iSpaceId, iFirstCol /* spaceId */ );
sqlite3VdbeAddOp2(v, OP_Integer, effective_user()->uid,
iFirstCol + 1 /* owner */ );
sqlite3VdbeAddOp4(v, OP_String8, 0, iFirstCol + 2 /* name */ , 0,
- sqlite3DbStrDup(pParse->db, p->def->name), P4_DYNAMIC);
+ sqlite3DbStrDup(pParse->db, table->def->name),
+ P4_DYNAMIC);
sqlite3VdbeAddOp4(v, OP_String8, 0, iFirstCol + 3 /* engine */ , 0,
- sqlite3DbStrDup(pParse->db, p->def->engine_name),
+ sqlite3DbStrDup(pParse->db, table->def->engine_name),
P4_DYNAMIC);
- sqlite3VdbeAddOp2(v, OP_Integer, p->def->field_count,
+ sqlite3VdbeAddOp2(v, OP_Integer, table->def->field_count,
iFirstCol + 4 /* field_count */ );
- sqlite3VdbeAddOp4(v, OP_Blob, zOptsSz, iFirstCol + 5,
- SQL_SUBTYPE_MSGPACK, zOpts, P4_DYNAMIC);
+ sqlite3VdbeAddOp4(v, OP_Blob, table_opts_stmt_sz, iFirstCol + 5,
+ SQL_SUBTYPE_MSGPACK, table_opts_stmt, P4_DYNAMIC);
/* zOpts and zFormat are co-located, hence STATIC */
- sqlite3VdbeAddOp4(v, OP_Blob, zFormatSz, iFirstCol + 6,
- SQL_SUBTYPE_MSGPACK, zFormat, P4_STATIC);
+ sqlite3VdbeAddOp4(v, OP_Blob, table_stmt_sz, iFirstCol + 6,
+ SQL_SUBTYPE_MSGPACK, table_stmt, P4_STATIC);
sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, iRecord);
sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, iRecord);
sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
@@ -1603,26 +1611,42 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
fkey_action_strs[fk->on_delete], P4_STATIC);
sqlite3VdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 6, 0,
fkey_action_strs[fk->on_update], P4_STATIC);
- size_t parent_size = fkey_encode_links(fk, FIELD_LINK_PARENT, NULL);
- size_t child_size = fkey_encode_links(fk, FIELD_LINK_CHILD, NULL);
+ struct region *region = &fiber()->gc;
+ uint32_t parent_links_size = 0, child_links_size = 0;
+ char *parent_links = NULL, *child_links = NULL;
+ parent_links = fkey_encode_links(region, fk, FIELD_LINK_PARENT,
+ &parent_links_size);
+ if (parent_links != NULL) {
+ child_links = fkey_encode_links(region, fk, FIELD_LINK_CHILD,
+ &child_links_size);
+ }
+ if (parent_links == NULL || child_links == NULL) {
+ parse_context->nErr++;
+ parse_context->rc = SQL_TARANTOOL_ERROR;
+ sqlite3DbFree(parse_context->db, name_copy);
+ return;
+ }
/*
* We are allocating memory for both parent and child
* arrays in the same chunk. Thus, first OP_Blob opcode
* interprets it as static memory, and the second one -
* as dynamic and releases memory.
*/
- char *parent_links = sqlite3DbMallocRaw(parse_context->db,
- parent_size + child_size);
- if (parent_links == NULL) {
- sqlite3DbFree(parse_context->db, (void *) name_copy);
+ char *raw = sqlite3DbMallocRaw(parse_context->db,
+ parent_links_size + child_links_size);
+ if (raw == NULL) {
+ sqlite3DbFree(parse_context->db, name_copy);
return;
}
- parent_size = fkey_encode_links(fk, FIELD_LINK_PARENT, parent_links);
- char *child_links = parent_links + parent_size;
- child_size = fkey_encode_links(fk, FIELD_LINK_CHILD, child_links);
- sqlite3VdbeAddOp4(vdbe, OP_Blob, child_size, constr_tuple_reg + 7,
+ memcpy(raw, parent_links, parent_links_size);
+ parent_links = raw;
+ raw += parent_links_size;
+ memcpy(raw, child_links, child_links_size);
+ child_links = raw;
+
+ sqlite3VdbeAddOp4(vdbe, OP_Blob, child_links_size, constr_tuple_reg + 7,
SQL_SUBTYPE_MSGPACK, child_links, P4_STATIC);
- sqlite3VdbeAddOp4(vdbe, OP_Blob, parent_size, constr_tuple_reg + 8,
+ sqlite3VdbeAddOp4(vdbe, OP_Blob, parent_links_size, constr_tuple_reg + 8,
SQL_SUBTYPE_MSGPACK, parent_links, P4_DYNAMIC);
sqlite3VdbeAddOp3(vdbe, OP_MakeRecord, constr_tuple_reg, 9,
constr_tuple_reg + 9);
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index 94517f6..b158cc7 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -137,46 +137,74 @@ tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor,
int
tarantoolSqlite3IncrementMaxid(uint64_t *space_max_id);
-/*
- * Render "format" array for _space entry.
- * Returns result size.
- * If buf==NULL estimate result size.
+/**
+ * Encode Table AST to msgpack on @region.
+ *
+ * @param region to use.
+ * @param table AST to encode.
+ * @param[out] size of result allocation.
+ * @retval NULL on error.
+ * @retval not NULL msgpack pointer on success.
*/
-int tarantoolSqlite3MakeTableFormat(Table * pTable, void *buf);
+char *
+sql_encode_table(struct region *region, struct Table *table, uint32_t *size);
-/*
- * Format "opts" dictionary for _space entry.
- * Returns result size.
- * If buf==NULL estimate result size.
+/**
+ * Encode "opts" dictionary for _space entry on @region.
+ *
+ * @param region to use.
+ * @param table AST containing opts to encode.
+ * @param sql source request to encode.
+ * @param[out] size of result allocation.
+ * @retval NULL on error.
+ * @retval not NULL msgpack pointer on success.
*/
-int tarantoolSqlite3MakeTableOpts(Table * pTable, const char *zSql, char *buf);
+char *
+sql_encode_table_opts(struct region *region, struct Table *table,
+ const char *sql, uint32_t *size);
/**
- * Encode links of given foreign key constraint into MsgPack.
+ * Encode links of given foreign key constraint into MsgPack on
+ * @region.
*
+ * @param region to use.
* @param def FK def to encode links of.
* @param type Links type to encode.
- * @param buf Buffer to hold encoded links. Can be NULL. In this
- * case function would simply calculate memory required
- * for such buffer.
- * @retval Length of encoded array.
+ * @param[out] size of result allocation.
+ * @retval NULL on error.
+ * @retval not NULL msgpack pointer on success.
*/
-int
-fkey_encode_links(const struct fkey_def *def, int type, char *buf);
+char *
+fkey_encode_links(struct region *region, const struct fkey_def *def, int type,
+ uint32_t *size);
-/*
- * Format "parts" array for _index entry.
- * Returns result size.
- * If buf==NULL estimate result size.
+/**
+ * Encode links of given foreign key constraint into MsgPack on
+ * @region.
+ *
+ * @param region to use.
+ * @param index to encode.
+ * @param[out] size of result allocation.
+ * @retval NULL on error.
+ * @retval not NULL msgpack pointer on success.
*/
-int tarantoolSqlite3MakeIdxParts(Index * index, void *buf);
+char *
+sql_encode_index_parts(struct region *region, struct Index *index,
+ uint32_t *size);
-/*
- * Format "opts" dictionary for _index entry.
- * Returns result size.
- * If buf==NULL estimate result size.
+/**
+ * Encode "opts" dictionary for _index entry on @region.
+ *
+ * @param region to use.
+ * @param index to encode.
+ * @param sql source request to encode.
+ * @param[out] size of result allocation.
+ * @retval NULL on error.
+ * @retval not NULL msgpack pointer on success.
*/
-int tarantoolSqlite3MakeIdxOpts(Index * index, const char *zSql, void *buf);
+char *
+sql_encode_index_opts(struct region *region, struct Index *index,
+ const char *sql, uint32_t *size);
/**
* Extract next id from _sequence space.
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index bd730c4..6a07081 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -225,20 +225,22 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
parse->nMem += 3;
int record = ++parse->nMem;
- opts_buff =
- sqlite3DbMallocRaw(parse->db,
- tarantoolSqlite3MakeTableOpts(0,
- sql_str,
- NULL) +
- 1);
- if (db->mallocFailed)
+ uint32_t opts_buff_sz = 0;
+ char *data = sql_encode_table_opts(&fiber()->gc, NULL, sql_str,
+ &opts_buff_sz);
+ if (data != NULL) {
+ opts_buff = sqlite3DbMallocRaw(parse->db, opts_buff_sz);
+ if (opts_buff != NULL)
+ memcpy(opts_buff, data, opts_buff_sz);
+ } else {
+ parse->nErr++;
+ parse->rc = SQL_TARANTOOL_ERROR;
+ }
+ if (opts_buff == NULL)
goto triggerfinish_cleanup;
- int opts_buff_sz =
- tarantoolSqlite3MakeTableOpts(0, sql_str, opts_buff);
-
trigger_name = sqlite3DbStrDup(parse->db, trigger_name);
- if (db->mallocFailed)
+ if (trigger_name == NULL)
goto triggerfinish_cleanup;
sqlite3VdbeAddOp4(v,
diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
index acd860a..a757e22 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -114,6 +114,8 @@ luamp_encode_array(struct luaL_serializer *cfg, struct mpstream *stream,
(void) cfg;
assert(mp_sizeof_array(size) <= 5);
char *data = mpstream_reserve(stream, 5);
+ if (data == NULL)
+ return;
char *pos = mp_encode_array(data, size);
mpstream_advance(stream, pos - data);
}
@@ -125,6 +127,8 @@ luamp_encode_map(struct luaL_serializer *cfg, struct mpstream *stream,
(void) cfg;
assert(mp_sizeof_map(size) <= 5);
char *data = mpstream_reserve(stream, 5);
+ if (data == NULL)
+ return;
char *pos = mp_encode_map(data, size);
mpstream_advance(stream, pos - data);
}
@@ -136,6 +140,8 @@ luamp_encode_uint(struct luaL_serializer *cfg, struct mpstream *stream,
(void) cfg;
assert(mp_sizeof_uint(num) <= 9);
char *data = mpstream_reserve(stream, 9);
+ if (data == NULL)
+ return;
char *pos = mp_encode_uint(data, num);
mpstream_advance(stream, pos - data);
}
@@ -147,6 +153,8 @@ luamp_encode_int(struct luaL_serializer *cfg, struct mpstream *stream,
(void) cfg;
assert(mp_sizeof_int(num) <= 9);
char *data = mpstream_reserve(stream, 9);
+ if (data == NULL)
+ return;
char *pos = mp_encode_int(data, num);
mpstream_advance(stream, pos - data);
}
@@ -158,6 +166,8 @@ luamp_encode_float(struct luaL_serializer *cfg, struct mpstream *stream,
(void) cfg;
assert(mp_sizeof_float(num) <= 5);
char *data = mpstream_reserve(stream, 5);
+ if (data == NULL)
+ return;
char *pos = mp_encode_float(data, num);
mpstream_advance(stream, pos - data);
}
@@ -169,6 +179,8 @@ luamp_encode_double(struct luaL_serializer *cfg, struct mpstream *stream,
(void) cfg;
assert(mp_sizeof_double(num) <= 9);
char *data = mpstream_reserve(stream, 9);
+ if (data == NULL)
+ return;
char *pos = mp_encode_double(data, num);
mpstream_advance(stream, pos - data);
}
@@ -180,6 +192,8 @@ luamp_encode_str(struct luaL_serializer *cfg, struct mpstream *stream,
(void) cfg;
assert(mp_sizeof_str(len) <= 5 + len);
char *data = mpstream_reserve(stream, 5 + len);
+ if (data == NULL)
+ return;
char *pos = mp_encode_str(data, str, len);
mpstream_advance(stream, pos - data);
}
@@ -190,6 +204,8 @@ luamp_encode_nil(struct luaL_serializer *cfg, struct mpstream *stream)
(void) cfg;
assert(mp_sizeof_nil() <= 1);
char *data = mpstream_reserve(stream, 1);
+ if (data == NULL)
+ return;
char *pos = mp_encode_nil(data);
mpstream_advance(stream, pos - data);
}
@@ -201,6 +217,8 @@ luamp_encode_bool(struct luaL_serializer *cfg, struct mpstream *stream,
(void) cfg;
assert(mp_sizeof_bool(val) <= 1);
char *data = mpstream_reserve(stream, 1);
+ if (data == NULL)
+ return;
char *pos = mp_encode_bool(data, val);
mpstream_advance(stream, pos - data);
}
--
2.7.4
^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: remove struct Enc
2018-08-17 12:24 [tarantool-patches] [PATCH v1 1/1] sql: remove struct Enc Kirill Shcherbatov
@ 2018-08-17 12:40 ` Vladislav Shpilevoy
2018-08-17 14:04 ` Kirill Shcherbatov
[not found] ` <cover.1534514520.git.kshcherbatov@tarantool.org>
2018-08-21 12:09 ` [tarantool-patches] Re: [PATCH v1 1/1] sql: remove struct Enc n.pettik
2 siblings, 1 reply; 8+ messages in thread
From: Vladislav Shpilevoy @ 2018-08-17 12:40 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov; +Cc: korablev
Hi! Please, do not use any Lua things in SQL.
You should not even include this: #include "lua/msgpack.h".
On 17/08/2018 15:24, Kirill Shcherbatov wrote:
> We shouldn't use legacy Enc structure in SQL as we have
> featured class mpstream in tarantool core. With this path
> all allocations temporally commited with mpstream initialized
> on region and then duplicated in dynamic persistent memory via
> sqlite3DbMallocRaw.
>
> Closes #3545.
> ---
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3545-remove-enc-struct
> Issue: https://github.com/tarantool/tarantool/issues/3545
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] [PATCH v1 1/2] box: export mpstream methods to core
[not found] ` <cover.1534514520.git.kshcherbatov@tarantool.org>
@ 2018-08-17 14:04 ` Kirill Shcherbatov
2018-08-24 16:33 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 8+ messages in thread
From: Kirill Shcherbatov @ 2018-08-17 14:04 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, korablev, Kirill Shcherbatov
As we going to use mpstream not only in LUA, let's move this
API in tarantool core.
Part of #3545.
---
src/lua/msgpack.c | 85 +++----------------
src/lua/msgpack.h | 53 +-----------
src/mpstream.h | 238 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 248 insertions(+), 128 deletions(-)
create mode 100644 src/mpstream.h
diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
index acd860a..bbcb886 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -50,46 +50,6 @@ luamp_error(void *error_ctx)
luaL_error(L, diag_last_error(diag_get())->errmsg);
}
-void
-mpstream_init(struct mpstream *stream, void *ctx,
- luamp_reserve_f reserve, luamp_alloc_f alloc,
- luamp_error_f error, void *error_ctx)
-{
- stream->ctx = ctx;
- stream->reserve = reserve;
- stream->alloc = alloc;
- stream->error = error;
- stream->error_ctx = error_ctx;
- mpstream_reset(stream);
-}
-
-void
-mpstream_reserve_slow(struct mpstream *stream, size_t size)
-{
- stream->alloc(stream->ctx, stream->pos - stream->buf);
- stream->buf = (char *) stream->reserve(stream->ctx, &size);
- if (stream->buf == NULL) {
- diag_set(OutOfMemory, size, "mpstream", "reserve");
- stream->error(stream->error_ctx);
- }
- stream->pos = stream->buf;
- stream->end = stream->pos + size;
-}
-
-void
-mpstream_reset(struct mpstream *stream)
-{
- size_t size = 0;
- stream->buf = (char *) stream->reserve(stream->ctx, &size);
- if (stream->buf == NULL) {
- diag_set(OutOfMemory, size, "mpstream", "reset");
- stream->error(stream->error_ctx);
- }
- stream->pos = stream->buf;
- stream->end = stream->pos + size;
-}
-
-
static uint32_t CTID_CHAR_PTR;
static uint32_t CTID_STRUCT_IBUF;
@@ -112,10 +72,7 @@ luamp_encode_array(struct luaL_serializer *cfg, struct mpstream *stream,
uint32_t size)
{
(void) cfg;
- assert(mp_sizeof_array(size) <= 5);
- char *data = mpstream_reserve(stream, 5);
- char *pos = mp_encode_array(data, size);
- mpstream_advance(stream, pos - data);
+ mpstream_encode_array(stream, size);
}
void
@@ -123,10 +80,7 @@ luamp_encode_map(struct luaL_serializer *cfg, struct mpstream *stream,
uint32_t size)
{
(void) cfg;
- assert(mp_sizeof_map(size) <= 5);
- char *data = mpstream_reserve(stream, 5);
- char *pos = mp_encode_map(data, size);
- mpstream_advance(stream, pos - data);
+ mpstream_encode_map(stream, size);
}
void
@@ -134,10 +88,7 @@ luamp_encode_uint(struct luaL_serializer *cfg, struct mpstream *stream,
uint64_t num)
{
(void) cfg;
- assert(mp_sizeof_uint(num) <= 9);
- char *data = mpstream_reserve(stream, 9);
- char *pos = mp_encode_uint(data, num);
- mpstream_advance(stream, pos - data);
+ mpstream_encode_uint(stream, num);
}
void
@@ -145,10 +96,7 @@ luamp_encode_int(struct luaL_serializer *cfg, struct mpstream *stream,
int64_t num)
{
(void) cfg;
- assert(mp_sizeof_int(num) <= 9);
- char *data = mpstream_reserve(stream, 9);
- char *pos = mp_encode_int(data, num);
- mpstream_advance(stream, pos - data);
+ mpstream_encode_int(stream, num);
}
void
@@ -156,10 +104,7 @@ luamp_encode_float(struct luaL_serializer *cfg, struct mpstream *stream,
float num)
{
(void) cfg;
- assert(mp_sizeof_float(num) <= 5);
- char *data = mpstream_reserve(stream, 5);
- char *pos = mp_encode_float(data, num);
- mpstream_advance(stream, pos - data);
+ mpstream_encode_float(stream, num);
}
void
@@ -167,10 +112,7 @@ luamp_encode_double(struct luaL_serializer *cfg, struct mpstream *stream,
double num)
{
(void) cfg;
- assert(mp_sizeof_double(num) <= 9);
- char *data = mpstream_reserve(stream, 9);
- char *pos = mp_encode_double(data, num);
- mpstream_advance(stream, pos - data);
+ mpstream_encode_double(stream, num);
}
void
@@ -178,20 +120,14 @@ luamp_encode_str(struct luaL_serializer *cfg, struct mpstream *stream,
const char *str, uint32_t len)
{
(void) cfg;
- assert(mp_sizeof_str(len) <= 5 + len);
- char *data = mpstream_reserve(stream, 5 + len);
- char *pos = mp_encode_str(data, str, len);
- mpstream_advance(stream, pos - data);
+ mpstream_encode_str(stream, str, len);
}
void
luamp_encode_nil(struct luaL_serializer *cfg, struct mpstream *stream)
{
(void) cfg;
- assert(mp_sizeof_nil() <= 1);
- char *data = mpstream_reserve(stream, 1);
- char *pos = mp_encode_nil(data);
- mpstream_advance(stream, pos - data);
+ mpstream_encode_nil(stream);
}
void
@@ -199,10 +135,7 @@ luamp_encode_bool(struct luaL_serializer *cfg, struct mpstream *stream,
bool val)
{
(void) cfg;
- assert(mp_sizeof_bool(val) <= 1);
- char *data = mpstream_reserve(stream, 1);
- char *pos = mp_encode_bool(data, val);
- mpstream_advance(stream, pos - data);
+ mpstream_encode_bool(stream, val);
}
static enum mp_type
diff --git a/src/lua/msgpack.h b/src/lua/msgpack.h
index bacf3e0..d8ad202 100644
--- a/src/lua/msgpack.h
+++ b/src/lua/msgpack.h
@@ -34,6 +34,7 @@
#include <stdbool.h>
#include "utils.h"
+#include "mpstream.h"
#if defined(__cplusplus)
extern "C" {
@@ -51,11 +52,6 @@ struct luaL_serializer;
extern struct luaL_serializer *luaL_msgpack_default;
/**
- * A streaming API so that it's possible to encode to any output
- * stream.
- */
-
-/**
* Ask the allocator to reserve at least size bytes. It can reserve
* more, and update *size with the new size.
*/
@@ -67,59 +63,12 @@ typedef void *(*luamp_alloc_f)(void *ctx, size_t size);
/** Actually use the bytes. */
typedef void (*luamp_error_f)(void *error_ctx);
-struct mpstream {
- /**
- * When pos >= end, or required size doesn't fit in
- * pos..end range alloc() is called to advance the stream
- * and reserve() to get a new chunk.
- */
- char *buf, *pos, *end;
- void *ctx;
- luamp_reserve_f reserve;
- luamp_alloc_f alloc;
- luamp_error_f error;
- void *error_ctx;
-};
-
/**
* luaL_error()
*/
void
luamp_error(void *);
-void
-mpstream_init(struct mpstream *stream, void *ctx,
- luamp_reserve_f reserve, luamp_alloc_f alloc,
- luamp_error_f error, void *error_ctx);
-
-void
-mpstream_reset(struct mpstream *stream);
-
-void
-mpstream_reserve_slow(struct mpstream *stream, size_t size);
-
-static inline void
-mpstream_flush(struct mpstream *stream)
-{
- stream->alloc(stream->ctx, stream->pos - stream->buf);
- stream->buf = stream->pos;
-}
-
-static inline char *
-mpstream_reserve(struct mpstream *stream, size_t size)
-{
- if (stream->pos + size > stream->end)
- mpstream_reserve_slow(stream, size);
- return stream->pos;
-}
-
-static inline void
-mpstream_advance(struct mpstream *stream, size_t size)
-{
- assert(stream->pos + size <= stream->end);
- stream->pos += size;
-}
-
enum { LUAMP_ALLOC_FACTOR = 256 };
void
diff --git a/src/mpstream.h b/src/mpstream.h
new file mode 100644
index 0000000..c8696f6
--- /dev/null
+++ b/src/mpstream.h
@@ -0,0 +1,238 @@
+#ifndef TARANTOOL_LUA_MPSTREAM_H_INCLUDED
+#define TARANTOOL_LUA_MPSTREAM_H_INCLUDED
+/*
+ * Copyright 2010-2015, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ * copyright notice, this list of conditions and the
+ * following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer in the documentation and/or other materials
+ * provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+#include <stddef.h>
+#include <assert.h>
+#include <stdint.h>
+#include "msgpuck.h"
+#include "diag.h"
+
+/**
+* Ask the allocator to reserve at least size bytes. It can reserve
+* more, and update *size with the new size.
+*/
+typedef void *(*mpstream_reserve_f)(void *ctx, size_t *size);
+
+/** Actually use the bytes. */
+typedef void *(*mpstream_alloc_f)(void *ctx, size_t size);
+
+/** Actually use the bytes. */
+typedef void (*mpstream_error_f)(void *error_ctx);
+
+struct mpstream {
+ /**
+ * When pos >= end, or required size doesn't fit in
+ * pos..end range alloc() is called to advance the stream
+ * and reserve() to get a new chunk.
+ */
+ char *buf, *pos, *end;
+ void *ctx;
+ mpstream_reserve_f reserve;
+ mpstream_alloc_f alloc;
+ mpstream_error_f error;
+ void *error_ctx;
+};
+
+static inline void
+mpstream_reserve_slow(struct mpstream *stream, size_t size)
+{
+ stream->alloc(stream->ctx, stream->pos - stream->buf);
+ stream->buf = (char *) stream->reserve(stream->ctx, &size);
+ if (stream->buf == NULL) {
+ diag_set(OutOfMemory, size, "mpstream", "reserve");
+ stream->error(stream->error_ctx);
+ }
+ stream->pos = stream->buf;
+ stream->end = stream->pos + size;
+}
+
+static inline void
+mpstream_reset(struct mpstream *stream)
+{
+ size_t size = 0;
+ stream->buf = (char *) stream->reserve(stream->ctx, &size);
+ if (stream->buf == NULL) {
+ diag_set(OutOfMemory, size, "mpstream", "reset");
+ stream->error(stream->error_ctx);
+ }
+ stream->pos = stream->buf;
+ stream->end = stream->pos + size;
+}
+
+/**
+ * A streaming API so that it's possible to encode to any output
+ * stream.
+ */
+static inline void
+mpstream_init(struct mpstream *stream, void *ctx,
+ mpstream_reserve_f reserve, mpstream_alloc_f alloc,
+ mpstream_error_f error, void *error_ctx)
+{
+ stream->ctx = ctx;
+ stream->reserve = reserve;
+ stream->alloc = alloc;
+ stream->error = error;
+ stream->error_ctx = error_ctx;
+ mpstream_reset(stream);
+}
+
+static inline void
+mpstream_flush(struct mpstream *stream)
+{
+ stream->alloc(stream->ctx, stream->pos - stream->buf);
+ stream->buf = stream->pos;
+}
+
+static inline char *
+mpstream_reserve(struct mpstream *stream, size_t size)
+{
+ if (stream->pos + size > stream->end)
+ mpstream_reserve_slow(stream, size);
+ return stream->pos;
+}
+
+static inline void
+mpstream_advance(struct mpstream *stream, size_t size)
+{
+ assert(stream->pos + size <= stream->end);
+ stream->pos += size;
+}
+
+static inline void
+mpstream_encode_array(struct mpstream *stream, uint32_t size)
+{
+ assert(mp_sizeof_array(size) <= 5);
+ char *data = mpstream_reserve(stream, 5);
+ if (data == NULL)
+ return;
+ char *pos = mp_encode_array(data, size);
+ mpstream_advance(stream, pos - data);
+}
+
+static inline void
+mpstream_encode_map(struct mpstream *stream, uint32_t size)
+{
+ assert(mp_sizeof_map(size) <= 5);
+ char *data = mpstream_reserve(stream, 5);
+ if (data == NULL)
+ return;
+ char *pos = mp_encode_map(data, size);
+ mpstream_advance(stream, pos - data);
+}
+
+static inline void
+mpstream_encode_uint(struct mpstream *stream, uint64_t num)
+{
+ assert(mp_sizeof_uint(num) <= 9);
+ char *data = mpstream_reserve(stream, 9);
+ if (data == NULL)
+ return;
+ char *pos = mp_encode_uint(data, num);
+ mpstream_advance(stream, pos - data);
+}
+
+static inline void
+mpstream_encode_int(struct mpstream *stream, int64_t num)
+{
+ assert(mp_sizeof_int(num) <= 9);
+ char *data = mpstream_reserve(stream, 9);
+ if (data == NULL)
+ return;
+ char *pos = mp_encode_int(data, num);
+ mpstream_advance(stream, pos - data);
+}
+
+static inline void
+mpstream_encode_float(struct mpstream *stream, float num)
+{
+ assert(mp_sizeof_float(num) <= 5);
+ char *data = mpstream_reserve(stream, 5);
+ if (data == NULL)
+ return;
+ char *pos = mp_encode_float(data, num);
+ mpstream_advance(stream, pos - data);
+}
+
+static inline void
+mpstream_encode_double(struct mpstream *stream, double num)
+{
+ assert(mp_sizeof_double(num) <= 9);
+ char *data = mpstream_reserve(stream, 9);
+ char *pos = mp_encode_double(data, num);
+ if (data == NULL)
+ return;
+ mpstream_advance(stream, pos - data);
+}
+
+static inline void
+mpstream_encode_str(struct mpstream *stream, const char *str, uint32_t len)
+{
+ assert(mp_sizeof_str(len) <= 5 + len);
+ char *data = mpstream_reserve(stream, 5 + len);
+ if (data == NULL)
+ return;
+ char *pos = mp_encode_str(data, str, len);
+ mpstream_advance(stream, pos - data);
+}
+
+static inline void
+mpstream_encode_nil(struct mpstream *stream)
+{
+ assert(mp_sizeof_nil() <= 1);
+ char *data = mpstream_reserve(stream, 1);
+ if (data == NULL)
+ return;
+ char *pos = mp_encode_nil(data);
+ mpstream_advance(stream, pos - data);
+}
+
+static inline void
+mpstream_encode_bool(struct mpstream *stream, bool val)
+{
+ assert(mp_sizeof_bool(val) <= 1);
+ char *data = mpstream_reserve(stream, 1);
+ if (data == NULL)
+ return;
+ char *pos = mp_encode_bool(data, val);
+ mpstream_advance(stream, pos - data);
+}
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
+
+#endif /* TARANTOOL_LUA_MPSTREAM_H_INCLUDED */
--
2.7.4
^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: remove struct Enc
2018-08-17 12:40 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-08-17 14:04 ` Kirill Shcherbatov
0 siblings, 0 replies; 8+ messages in thread
From: Kirill Shcherbatov @ 2018-08-17 14:04 UTC (permalink / raw)
To: tarantool-patches, Nikita Pettik; +Cc: Vladislav Shpilevoy
On 17.08.2018 15:40, Vladislav Shpilevoy wrote:
> Hi! Please, do not use any Lua things in SQL.
> You should not even include this: #include "lua/msgpack.h".
======================================
diff --git a/src/box/sql.c b/src/box/sql.c
index f1e860d..321d3c4 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -56,7 +56,7 @@
#include "xrow.h"
#include "iproto_constants.h"
#include "fkey.h"
-#include "lua/msgpack.h"
+#include "mpstream.h"
#include "small/region.h"
static sqlite3 *db = NULL;
@@ -1250,7 +1250,7 @@ sql_encode_table(struct region *region, struct Table *table, uint32_t *size)
pk_forced_int = pk;
}
uint32_t field_count = def->field_count;
- luamp_encode_array(luaL_msgpack_default, &stream, field_count);
+ mpstream_encode_array(&stream, field_count);
for (uint32_t i = 0; i < field_count && !is_error; i++) {
const char *t;
uint32_t cid = def->fields[i].coll_id;
@@ -1261,13 +1261,10 @@ sql_encode_table(struct region *region, struct Table *table, uint32_t *size)
base_len += 1;
if (default_str != NULL)
base_len += 1;
- luamp_encode_map(luaL_msgpack_default, &stream, base_len);
- luamp_encode_str(luaL_msgpack_default, &stream, "name",
- strlen("name"));
- luamp_encode_str(luaL_msgpack_default, &stream, field->name,
- strlen(field->name));
- luamp_encode_str(luaL_msgpack_default, &stream, "type",
- strlen("type"));
+ mpstream_encode_map(&stream, base_len);
+ mpstream_encode_str(&stream, "name", strlen("name"));
+ mpstream_encode_str(&stream, field->name, strlen(field->name));
+ mpstream_encode_str(&stream, "type", strlen("type"));
if (i == pk_forced_int) {
t = "integer";
} else {
@@ -1280,32 +1277,30 @@ sql_encode_table(struct region *region, struct Table *table, uint32_t *size)
assert(def->fields[i].is_nullable ==
action_is_nullable(def->fields[i].
nullable_action));
- luamp_encode_str(luaL_msgpack_default, &stream, t, strlen(t));
- luamp_encode_str(luaL_msgpack_default, &stream, "affinity",
- strlen("affinity"));
- luamp_encode_uint(luaL_msgpack_default, &stream,
+ mpstream_encode_str(&stream, t, strlen(t));
+ mpstream_encode_str(&stream, "affinity", strlen("affinity"));
+ mpstream_encode_uint(&stream,
def->fields[i].affinity);
- luamp_encode_str(luaL_msgpack_default, &stream, "is_nullable",
- strlen("is_nullable"));
- luamp_encode_bool(luaL_msgpack_default, &stream,
+ mpstream_encode_str(&stream, "is_nullable",
+ strlen("is_nullable"));
+ mpstream_encode_bool(&stream,
def->fields[i].is_nullable);
- luamp_encode_str(luaL_msgpack_default, &stream,
- "nullable_action", strlen("nullable_action"));
+ mpstream_encode_str(&stream, "nullable_action",
+ strlen("nullable_action"));
assert(def->fields[i].nullable_action < on_conflict_action_MAX);
const char *action =
on_conflict_action_strs[def->fields[i].nullable_action];
- luamp_encode_str(luaL_msgpack_default, &stream, action,
- strlen(action));
+ mpstream_encode_str(&stream, action, strlen(action));
if (cid != COLL_NONE) {
- luamp_encode_str(luaL_msgpack_default, &stream,
- "collation", strlen("collation"));
- luamp_encode_uint(luaL_msgpack_default, &stream, cid);
+ mpstream_encode_str(&stream, "collation",
+ strlen("collation"));
+ mpstream_encode_uint(&stream, cid);
}
if (default_str != NULL) {
- luamp_encode_str(luaL_msgpack_default, &stream,
- "default", strlen("default"));
- luamp_encode_str(luaL_msgpack_default, &stream,
- default_str, strlen(default_str));
+ mpstream_encode_str(&stream, "default",
+ strlen("default"));
+ mpstream_encode_str(&stream, default_str,
+ strlen(default_str));
}
}
mpstream_flush(&stream);
@@ -1337,27 +1332,24 @@ sql_encode_table_opts(struct region *region, struct Table *table,
has_checks = table->def->opts.checks != NULL;
}
uint32_t checks_cnt = has_checks ? table->def->opts.checks->nExpr : 0;
- luamp_encode_map(luaL_msgpack_default, &stream,
- 1 + is_view + (checks_cnt > 0));
+ mpstream_encode_map(&stream, 1 + is_view + (checks_cnt > 0));
- luamp_encode_str(luaL_msgpack_default, &stream, "sql", strlen("sql"));
- luamp_encode_str(luaL_msgpack_default, &stream, sql, strlen(sql));
+ mpstream_encode_str(&stream, "sql", strlen("sql"));
+ mpstream_encode_str(&stream, sql, strlen(sql));
if (is_view) {
- luamp_encode_str(luaL_msgpack_default, &stream, "view",
- strlen("view"));
- luamp_encode_bool(luaL_msgpack_default, &stream, true);
+ mpstream_encode_str(&stream, "view", strlen("view"));
+ mpstream_encode_bool(&stream, true);
}
if (checks_cnt == 0)
goto finish;
/* Encode checks. */
struct ExprList_item *a = table->def->opts.checks->a;
- luamp_encode_str(luaL_msgpack_default, &stream, "checks",
- strlen("checks"));
- luamp_encode_array(luaL_msgpack_default, &stream, checks_cnt);
+ mpstream_encode_str(&stream, "checks", strlen("checks"));
+ mpstream_encode_array(&stream, checks_cnt);
for (uint32_t i = 0; i < checks_cnt && !is_error; ++i) {
int items = (a[i].pExpr != NULL) + (a[i].zName != NULL);
- luamp_encode_map(luaL_msgpack_default, &stream, items);
+ mpstream_encode_map(&stream, items);
/*
* a[i].pExpr could be NULL for VIEW column names
* represented as checks.
@@ -1365,17 +1357,14 @@ sql_encode_table_opts(struct region *region, struct Table *table,
if (a[i].pExpr != NULL) {
struct Expr *pExpr = a[i].pExpr;
assert(pExpr->u.zToken != NULL);
- luamp_encode_str(luaL_msgpack_default, &stream, "expr",
- strlen("expr"));
- luamp_encode_str(luaL_msgpack_default, &stream,
- pExpr->u.zToken,
- strlen(pExpr->u.zToken));
+ mpstream_encode_str(&stream, "expr", strlen("expr"));
+ mpstream_encode_str(&stream, pExpr->u.zToken,
+ strlen(pExpr->u.zToken));
}
if (a[i].zName != NULL) {
- luamp_encode_str(luaL_msgpack_default, &stream, "name",
- strlen("name"));
- luamp_encode_str(luaL_msgpack_default, &stream,
- a[i].zName, strlen(a[i].zName));
+ mpstream_encode_str(&stream, "name", strlen("name"));
+ mpstream_encode_str(&stream, a[i].zName,
+ strlen(a[i].zName));
}
}
@@ -1402,11 +1391,9 @@ fkey_encode_links(struct region *region, const struct fkey_def *def, int type,
mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
set_encode_error, &is_error);
uint32_t field_count = def->field_count;
- luamp_encode_array(luaL_msgpack_default, &stream, field_count);
- for (uint32_t i = 0; i < field_count && !is_error; ++i) {
- luamp_encode_uint(luaL_msgpack_default, &stream,
- def->links[i].fields[type]);
- }
+ mpstream_encode_array(&stream, field_count);
+ for (uint32_t i = 0; i < field_count && !is_error; ++i)
+ mpstream_encode_uint(&stream, def->links[i].fields[type]);
mpstream_flush(&stream);
if (is_error)
return NULL;
@@ -1451,7 +1438,7 @@ sql_encode_index_parts(struct region *region, struct SqliteIndex *index,
struct key_def *key_def = index->def->key_def;
uint32_t part_count = key_def->part_count;
struct key_part *part = key_def->parts;
- luamp_encode_array(luaL_msgpack_default, &stream, part_count);
+ mpstream_encode_array(&stream, part_count);
for (uint32_t i = 0; i < part_count; ++i, ++part) {
uint32_t col = part->fieldno;
assert(fields[col].is_nullable ==
@@ -1465,37 +1452,33 @@ sql_encode_index_parts(struct region *region, struct SqliteIndex *index,
}
/* Do not decode default collation. */
uint32_t cid = part->coll_id;
- luamp_encode_map(luaL_msgpack_default, &stream,
- 5 + (cid != COLL_NONE));
- luamp_encode_str(luaL_msgpack_default, &stream, "type",
- strlen("type"));
- luamp_encode_str(luaL_msgpack_default, &stream, t, strlen(t));
- luamp_encode_str(luaL_msgpack_default, &stream, "field",
- strlen("field"));
- luamp_encode_uint(luaL_msgpack_default, &stream, col);
+ mpstream_encode_map(&stream, 5 + (cid != COLL_NONE));
+ mpstream_encode_str(&stream, "type", strlen("type"));
+ mpstream_encode_str(&stream, t, strlen(t));
+ mpstream_encode_str(&stream, "field", strlen("field"));
+ mpstream_encode_uint(&stream, col);
if (cid != COLL_NONE) {
- luamp_encode_str(luaL_msgpack_default, &stream,
- "collation", strlen("collation"));
- luamp_encode_uint(luaL_msgpack_default, &stream, cid);
+ mpstream_encode_str(&stream, "collation",
+ strlen("collation"));
+ mpstream_encode_uint(&stream, cid);
}
- luamp_encode_str(luaL_msgpack_default, &stream, "is_nullable",
- strlen("is_nullable"));
- luamp_encode_bool(luaL_msgpack_default, &stream,
+ mpstream_encode_str(&stream, "is_nullable",
+ strlen("is_nullable"));
+ mpstream_encode_bool(&stream,
fields[col].is_nullable);
- luamp_encode_str(luaL_msgpack_default, &stream,
- "nullable_action", strlen("nullable_action"));
+ mpstream_encode_str(&stream, "nullable_action",
+ strlen("nullable_action"));
const char *action_str =
on_conflict_action_strs[fields[col].nullable_action];
- luamp_encode_str(luaL_msgpack_default, &stream, action_str,
- strlen(action_str));
+ mpstream_encode_str(&stream, action_str, strlen(action_str));
- luamp_encode_str(luaL_msgpack_default, &stream, "sort_order",
- strlen("sort_order"));
+ mpstream_encode_str(&stream, "sort_order",
+ strlen("sort_order"));
enum sort_order sort_order = part->sort_order;
assert(sort_order < sort_order_MAX);
const char *sort_order_str = sort_order_strs[sort_order];
- luamp_encode_str(luaL_msgpack_default, &stream, sort_order_str,
- strlen(sort_order_str));
+ mpstream_encode_str(&stream, sort_order_str,
+ strlen(sort_order_str));
}
mpstream_flush(&stream);
if (is_error)
@@ -1518,10 +1501,9 @@ sql_encode_index_opts(struct region *region, struct SqliteIndex *index,
bool is_error = false;
mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
set_encode_error, &is_error);
- luamp_encode_map(luaL_msgpack_default, &stream, 2);
+ mpstream_encode_map(&stream, 1 + (sql != NULL));
/* Mark as unique pk and unique indexes */
- luamp_encode_str(luaL_msgpack_default, &stream, "unique",
- strlen("unique"));
+ mpstream_encode_str(&stream, "unique", strlen("unique"));
/* If user didn't defined ON CONFLICT OPTIONS, all uniqueness checks
* will be made by Tarantool. However, Tarantool doesn't have ON
* CONFLIT option, so in that case (except ON CONFLICT ABORT, which is
@@ -1529,10 +1511,11 @@ sql_encode_index_opts(struct region *region, struct SqliteIndex *index,
* INSERT OR REPLACE/IGNORE uniqueness checks will be also done by
* Tarantool.
*/
- luamp_encode_bool(luaL_msgpack_default, &stream, IsUniqueIndex(index));
- luamp_encode_str(luaL_msgpack_default, &stream, "sql", strlen("sql"));
- luamp_encode_str(luaL_msgpack_default, &stream, sql,
- sql != NULL ? strlen(sql) : 0);
+ mpstream_encode_bool(&stream, IsUniqueIndex(index));
+ if (sql != NULL) {
+ mpstream_encode_str(&stream, "sql", strlen("sql"));
+ mpstream_encode_str(&stream, sql, strlen(sql));
+ }
mpstream_flush(&stream);
if (is_error)
return NULL;
^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: remove struct Enc
2018-08-17 12:24 [tarantool-patches] [PATCH v1 1/1] sql: remove struct Enc Kirill Shcherbatov
2018-08-17 12:40 ` [tarantool-patches] " Vladislav Shpilevoy
[not found] ` <cover.1534514520.git.kshcherbatov@tarantool.org>
@ 2018-08-21 12:09 ` n.pettik
2018-08-21 15:50 ` Kirill Shcherbatov
2 siblings, 1 reply; 8+ messages in thread
From: n.pettik @ 2018-08-21 12:09 UTC (permalink / raw)
To: tarantool-patches; +Cc: Kirill Shcherbatov
In general parch LGTM except several nitpickings.
But I hope smb is going to review it as well since
I am not sure about proper way of using mpstream
(I haven’t faced it before actually).
> -/*
> - * Format "opts" dictionary for _space entry.
> - * Returns result size.
> - * If buf==NULL estimate result size.
> - *
> - * Ex: {"temporary": true, "sql": "CREATE TABLE student (name, grade)"}
> - */
> -int
> -tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, char *buf)
> +char *
> +sql_encode_table_opts(struct region *region, struct Table *table,
> + const char *sql, uint32_t *size)
> {
> - const struct Enc *enc = get_enc(buf);
> - bool is_view = pTable != NULL && pTable->def->opts.is_view;
> - bool has_checks = pTable != NULL && pTable->def->opts.checks != NULL;
> - int checks_cnt = has_checks ? pTable->def->opts.checks->nExpr : 0;
> - char *p = enc->encode_map(buf, 1 + is_view + (checks_cnt > 0));
> -
> - p = enc->encode_str(p, "sql", 3);
> - p = enc->encode_str(p, zSql, strlen(zSql));
> + assert(sql != NULL);
> + 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);
> + bool is_view = false;
> + bool has_checks = false;
> + if (table != NULL) {
> + is_view = table->def->opts.is_view;
> + has_checks = table->def->opts.checks != NULL;
> + }
> + uint32_t checks_cnt = has_checks ? table->def->opts.checks->nExpr : 0;
> + luamp_encode_map(luaL_msgpack_default, &stream,
> + 1 + is_view + (checks_cnt > 0));
> +
> + luamp_encode_str(luaL_msgpack_default, &stream, "sql", strlen("sql"));
> + luamp_encode_str(luaL_msgpack_default, &stream, sql, strlen(sql));
> if (is_view) {
> - p = enc->encode_str(p, "view", 4);
> - p = enc->encode_bool(p, true);
> + luamp_encode_str(luaL_msgpack_default, &stream, "view",
> + strlen("view"));
> + luamp_encode_bool(luaL_msgpack_default, &stream, true);
> }
> if (checks_cnt == 0)
> - return p - buf;
> + goto finish;
> +
> /* Encode checks. */
> - struct ExprList_item *a = pTable->def->opts.checks->a;
> - p = enc->encode_str(p, "checks", 6);
> - p = enc->encode_array(p, checks_cnt);
> - for (int i = 0; i < checks_cnt; ++i) {
> + struct ExprList_item *a = table->def->opts.checks->a;
> + luamp_encode_str(luaL_msgpack_default, &stream, "checks",
> + strlen("checks"));
> + luamp_encode_array(luaL_msgpack_default, &stream, checks_cnt);
> + for (uint32_t i = 0; i < checks_cnt && !is_error; ++i) {
> int items = (a[i].pExpr != NULL) + (a[i].zName != NULL);
> - p = enc->encode_map(p, items);
> + luamp_encode_map(luaL_msgpack_default, &stream, items);
> /*
> * a[i].pExpr could be NULL for VIEW column names
> * represented as checks.
> */
I guess this comment is no longer relevant: VIEW column names are
stored in def->fields. You can remove additional check:
+++ b/src/box/sql.c
@@ -1350,17 +1350,11 @@ sql_encode_table_opts(struct region *region, struct Table *table,
for (uint32_t i = 0; i < checks_cnt && !is_error; ++i) {
int items = (a[i].pExpr != NULL) + (a[i].zName != NULL);
mpstream_encode_map(&stream, items);
- /*
- * a[i].pExpr could be NULL for VIEW column names
- * represented as checks.
- */
- if (a[i].pExpr != NULL) {
- struct Expr *pExpr = a[i].pExpr;
- assert(pExpr->u.zToken != NULL);
- mpstream_encode_str(&stream, "expr", strlen("expr"));
- mpstream_encode_str(&stream, pExpr->u.zToken,
+ struct Expr *pExpr = a[i].pExpr;
+ assert(pExpr->u.zToken != NULL);
+ mpstream_encode_str(&stream, "expr", strlen("expr"));
+ mpstream_encode_str(&stream, pExpr->u.zToken,
strlen(pExpr->u.zToken));
- }
> if (a[i].pExpr != NULL) {
> - Expr *pExpr = a[i].pExpr;
> + struct Expr *pExpr = a[i].pExpr;
> assert(pExpr->u.zToken != NULL);
> - p = enc->encode_str(p, "expr", 4);
> - p = enc->encode_str(p, pExpr->u.zToken,
> - strlen(pExpr->u.zToken));
> + luamp_encode_str(luaL_msgpack_default, &stream, "expr",
> + strlen("expr"));
> + luamp_encode_str(luaL_msgpack_default, &stream,
> + pExpr->u.zToken,
> + strlen(pExpr->u.zToken));
> }
> if (a[i].zName != NULL) {
> - p = enc->encode_str(p, "name", 4);
> - p = enc->encode_str(p, a[i].zName, strlen(a[i].zName));
> + luamp_encode_str(luaL_msgpack_default, &stream, "name",
> + strlen("name"));
> + luamp_encode_str(luaL_msgpack_default, &stream,
> + a[i].zName, strlen(a[i].zName));
> }
> }
> - return p - buf;
> +
> +finish:
> + mpstream_flush(&stream);
> + if (is_error)
> + return NULL;
> + uint32_t sz = region_used(region) - used;
Nitpicking: why do you use uint32_t meanwhile region_used() return size_t?
The same for other functions.
> -/*
> - * Format "opts" dictionary for _index entry.
> - * Returns result size.
> - * If buf==NULL estimate result size.
> - *
> - * Ex: {
> - * "unique": "true",
> - * "sql": "CREATE INDEX student_by_name ON students(name)"
> - * }
> - */
> -int tarantoolSqlite3MakeIdxOpts(SqliteIndex *index, const char *zSql, void *buf)
> +char *
> +sql_encode_index_opts(struct region *region, struct SqliteIndex *index,
> + const char *sql, uint32_t *size)
> {
> - const struct Enc *enc = get_enc(buf);
> - char *base = buf, *p;
> -
> - (void)index;
> -
> - p = enc->encode_map(base, 2);
> + 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);
> + luamp_encode_map(luaL_msgpack_default, &stream, 2);
> /* Mark as unique pk and unique indexes */
> - p = enc->encode_str(p, "unique", 6);
> + luamp_encode_str(luaL_msgpack_default, &stream, "unique",
> + strlen("unique"));
> /* If user didn't defined ON CONFLICT OPTIONS, all uniqueness checks
> * will be made by Tarantool. However, Tarantool doesn't have ON
> * CONFLIT option, so in that case (except ON CONFLICT ABORT, which is
> @@ -1511,10 +1529,20 @@ int tarantoolSqlite3MakeIdxOpts(SqliteIndex *index, const char *zSql, void *buf)
> * INSERT OR REPLACE/IGNORE uniqueness checks will be also done by
> * Tarantool.
> */
> - p = enc->encode_bool(p, IsUniqueIndex(index));
> - p = enc->encode_str(p, "sql", 3);
> - p = enc->encode_str(p, zSql, zSql ? strlen(zSql) : 0);
> - return (int)(p - base);
> + luamp_encode_bool(luaL_msgpack_default, &stream, IsUniqueIndex(index));
> + luamp_encode_str(luaL_msgpack_default, &stream, "sql", strlen("sql"));
> + luamp_encode_str(luaL_msgpack_default, &stream, sql,
> + sql != NULL ? strlen(sql) : 0);
> + mpstream_flush(&stream);
> + if (is_error)
> + return NULL;
> + uint32_t sz = region_used(region) - used;
> + char *raw = region_join(region, sz);
> + if (raw == NULL)
> + diag_set(OutOfMemory, sz, "region_join", "raw");
> + else
> + *size = sz;
> + return raw;
> }
>
> void
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index dddeb12..ca4ba44 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1241,25 +1241,30 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId,
> Vdbe *v = sqlite3GetVdbe(pParse);
> int iFirstCol = ++pParse->nMem;
> int iRecord = (pParse->nMem += 6); /* 6 total columns */
> - char *zOpts, *zParts;
> - int zOptsSz, zPartsSz;
> -
> - /* Format "opts" and "parts" for _index entry. */
> - zOpts = sqlite3DbMallocRaw(pParse->db,
> - tarantoolSqlite3MakeIdxOpts(pIndex, zSql,
> - NULL) +
> - tarantoolSqlite3MakeIdxParts(pIndex,
> - NULL) + 2);
> - if (!zOpts)
> + char *index_opts = NULL, *index_parts = NULL;
Nitpicking: I would combine declaration and first usage.
The same for index_opts_sz and index_parts_sz.
Overall, consider following refactoring:
+++ b/src/box/sql/build.c
@@ -1241,22 +1241,19 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId,
Vdbe *v = sqlite3GetVdbe(pParse);
int iFirstCol = ++pParse->nMem;
int iRecord = (pParse->nMem += 6); /* 6 total columns */
- char *index_opts = NULL, *index_parts = NULL;
- uint32_t index_opts_sz = 0, index_parts_sz = 0;
struct region *region = &fiber()->gc;
- index_opts =
- sql_encode_index_opts(region, pIndex, zSql, &index_opts_sz);
- if (index_opts != NULL) {
- index_parts =
- sql_encode_index_parts(region, pIndex, &index_parts_sz);
- }
- if (index_opts == NULL || index_parts == NULL) {
- pParse->rc = SQL_TARANTOOL_ERROR;
- pParse->nErr++;
- return;
- }
+ uint32_t index_opts_sz = 0;
+ char *index_opts = sql_encode_index_opts(region, pIndex, zSql,
+ &index_opts_sz);
+ if (index_opts == NULL)
+ goto error;
+ uint32_t index_parts_sz = 0;
+ char *index_parts = sql_encode_index_parts(region, pIndex,
+ &index_parts_sz);
+ if (index_parts == NULL)
+ goto error;
char *raw = sqlite3DbMallocRaw(pParse->db,
- index_opts_sz + index_parts_sz);
+ index_opts_sz + index_parts_sz);
if (raw == NULL)
return;
@@ -1310,6 +1307,10 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId,
if (pIndex->index_type == SQL_INDEX_TYPE_NON_UNIQUE ||
pIndex->index_type == SQL_INDEX_TYPE_UNIQUE)
sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+ return;
+error:
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
}
> + uint32_t index_opts_sz = 0, index_parts_sz = 0;
> + struct region *region = &fiber()->gc;
> + index_opts =
> + sql_encode_index_opts(region, pIndex, zSql, &index_opts_sz);
Why do you pass region as param if you anyway get it from global fiber()?
> + if (index_opts != NULL) {
> + index_parts =
> + sql_encode_index_parts(region, pIndex, &index_parts_sz);
> + }
> + if (index_opts == NULL || index_parts == NULL) {
> + pParse->rc = SQL_TARANTOOL_ERROR;
> + pParse->nErr++;
> return;
> - zOptsSz = tarantoolSqlite3MakeIdxOpts(pIndex, zSql, zOpts);
> - zParts = zOpts + zOptsSz + 1;
> - zPartsSz = tarantoolSqlite3MakeIdxParts(pIndex, zParts);
> -#if SQLITE_DEBUG
> - /* NUL-termination is necessary for VDBE trace facility only */
> - zOpts[zOptsSz] = 0;
> - zParts[zPartsSz] = 0;
> -#endif
> + }
> + char *raw = sqlite3DbMallocRaw(pParse->db,
> + index_opts_sz + index_parts_sz);
Wrong indentation.
> + if (raw == NULL)
> + return;
> +
> + memcpy(raw, index_opts, index_opts_sz);
> + index_opts = raw;
> + raw += index_opts_sz;
> + memcpy(raw, index_parts, index_parts_sz);
> + index_parts = raw;
>
> if (pParse->pNewTable) {
> int reg;
> @@ -1295,11 +1300,11 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId,
> P4_DYNAMIC);
> sqlite3VdbeAddOp4(v, OP_String8, 0, iFirstCol + 3, 0, "tree",
> P4_STATIC);
> - sqlite3VdbeAddOp4(v, OP_Blob, zOptsSz, iFirstCol + 4,
> - SQL_SUBTYPE_MSGPACK, zOpts, P4_DYNAMIC);
> + sqlite3VdbeAddOp4(v, OP_Blob, index_opts_sz, iFirstCol + 4,
> + SQL_SUBTYPE_MSGPACK, index_opts, P4_DYNAMIC);
> /* zOpts and zParts are co-located, hence STATIC */
> - sqlite3VdbeAddOp4(v, OP_Blob, zPartsSz, iFirstCol + 5,
> - SQL_SUBTYPE_MSGPACK,zParts, P4_STATIC);
> + sqlite3VdbeAddOp4(v, OP_Blob, index_parts_sz, iFirstCol + 5,
> + SQL_SUBTYPE_MSGPACK, index_parts, P4_STATIC);
> sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 6, iRecord);
> sqlite3VdbeAddOp2(v, OP_SInsert, BOX_INDEX_ID, iRecord);
> if (pIndex->index_type == SQL_INDEX_TYPE_NON_UNIQUE ||
> @@ -1359,47 +1364,50 @@ makeIndexSchemaRecord(Parse * pParse,
> static void
> createSpace(Parse * pParse, int iSpaceId, char *zStmt)
The same comments to this functions as to previous one
and the same refactoring.
> {
> - Table *p = pParse->pNewTable;
> + struct Table *table = pParse->pNewTable;
> Vdbe *v = sqlite3GetVdbe(pParse);
> int iFirstCol = ++pParse->nMem;
> int iRecord = (pParse->nMem += 7);
> - char *zOpts, *zFormat;
> - int zOptsSz, zFormatSz;
> -
> - zOpts = sqlite3DbMallocRaw(pParse->db,
> - tarantoolSqlite3MakeTableFormat(p, NULL) +
> - tarantoolSqlite3MakeTableOpts(p, zStmt,
> - NULL) + 2);
> - if (!zOpts) {
> - zOptsSz = 0;
> - zFormat = NULL;
> - zFormatSz = 0;
> - } else {
> - zOptsSz = tarantoolSqlite3MakeTableOpts(p, zStmt, zOpts);
> - zFormat = zOpts + zOptsSz + 1;
> - zFormatSz = tarantoolSqlite3MakeTableFormat(p, zFormat);
> -#if SQLITE_DEBUG
> - /* NUL-termination is necessary for VDBE-tracing facility only */
> - zOpts[zOptsSz] = 0;
> - zFormat[zFormatSz] = 0;
> -#endif
> + uint32_t table_stmt_sz = 0, table_opts_stmt_sz = 0;
> + char *table_stmt = NULL, *table_opts_stmt = NULL;
> +
> + struct region *region = &fiber()->gc;
> + table_opts_stmt =
> + sql_encode_table_opts(region, table, zStmt, &table_opts_stmt_sz);
Out of 80 chars.
> + if (table_opts_stmt != NULL)
> + table_stmt = sql_encode_table(region, table, &table_stmt_sz);
> + if (table_opts_stmt == NULL || table_stmt == NULL) {
> + pParse->nErr++;
> + pParse->rc = SQL_TARANTOOL_ERROR;
> + return;
> }
> + char *raw = sqlite3DbMallocRaw(pParse->db,
> + table_stmt_sz + table_opts_stmt_sz);
> + if (raw == NULL)
> + return;
> +
> + memcpy(raw, table_opts_stmt, table_opts_stmt_sz);
> + table_opts_stmt = raw;
> + raw += table_opts_stmt_sz;
> + memcpy(raw, table_stmt, table_stmt_sz);
> + table_stmt = raw;
>
> sqlite3VdbeAddOp2(v, OP_SCopy, iSpaceId, iFirstCol /* spaceId */ );
> sqlite3VdbeAddOp2(v, OP_Integer, effective_user()->uid,
> iFirstCol + 1 /* owner */ );
> sqlite3VdbeAddOp4(v, OP_String8, 0, iFirstCol + 2 /* name */ , 0,
> - sqlite3DbStrDup(pParse->db, p->def->name), P4_DYNAMIC);
> + sqlite3DbStrDup(pParse->db, table->def->name),
> + P4_DYNAMIC);
> sqlite3VdbeAddOp4(v, OP_String8, 0, iFirstCol + 3 /* engine */ , 0,
> - sqlite3DbStrDup(pParse->db, p->def->engine_name),
> + sqlite3DbStrDup(pParse->db, table->def->engine_name),
> P4_DYNAMIC);
> - sqlite3VdbeAddOp2(v, OP_Integer, p->def->field_count,
> + sqlite3VdbeAddOp2(v, OP_Integer, table->def->field_count,
> iFirstCol + 4 /* field_count */ );
> - sqlite3VdbeAddOp4(v, OP_Blob, zOptsSz, iFirstCol + 5,
> - SQL_SUBTYPE_MSGPACK, zOpts, P4_DYNAMIC);
> + sqlite3VdbeAddOp4(v, OP_Blob, table_opts_stmt_sz, iFirstCol + 5,
> + SQL_SUBTYPE_MSGPACK, table_opts_stmt, P4_DYNAMIC);
> /* zOpts and zFormat are co-located, hence STATIC */
> - sqlite3VdbeAddOp4(v, OP_Blob, zFormatSz, iFirstCol + 6,
> - SQL_SUBTYPE_MSGPACK, zFormat, P4_STATIC);
> + sqlite3VdbeAddOp4(v, OP_Blob, table_stmt_sz, iFirstCol + 6,
> + SQL_SUBTYPE_MSGPACK, table_stmt, P4_STATIC);
> sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, iRecord);
> sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, iRecord);
> sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
> @@ -1603,26 +1611,42 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
> fkey_action_strs[fk->on_delete], P4_STATIC);
> sqlite3VdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 6, 0,
> fkey_action_strs[fk->on_update], P4_STATIC);
> - size_t parent_size = fkey_encode_links(fk, FIELD_LINK_PARENT, NULL);
> - size_t child_size = fkey_encode_links(fk, FIELD_LINK_CHILD, NULL);
> + struct region *region = &fiber()->gc;
> + uint32_t parent_links_size = 0, child_links_size = 0;
> + char *parent_links = NULL, *child_links = NULL;
> + parent_links = fkey_encode_links(region, fk, FIELD_LINK_PARENT,
> + &parent_links_size);
Consider refactoring:
@@ -1612,20 +1613,16 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
sqlite3VdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 6, 0,
fkey_action_strs[fk->on_update], P4_STATIC);
struct region *region = &fiber()->gc;
- uint32_t parent_links_size = 0, child_links_size = 0;
- char *parent_links = NULL, *child_links = NULL;
- parent_links = fkey_encode_links(region, fk, FIELD_LINK_PARENT,
- &parent_links_size);
- if (parent_links != NULL) {
- child_links = fkey_encode_links(region, fk, FIELD_LINK_CHILD,
- &child_links_size);
- }
- if (parent_links == NULL || child_links == NULL) {
- parse_context->nErr++;
- parse_context->rc = SQL_TARANTOOL_ERROR;
- sqlite3DbFree(parse_context->db, name_copy);
- return;
- }
+ uint32_t parent_links_size;
+ char *parent_links = fkey_encode_links(region, fk, FIELD_LINK_PARENT,
+ &parent_links_size);
+ if (parent_links == NULL)
+ goto tnt_error;
+ uint32_t child_links_size;
+ char *child_links = fkey_encode_links(region, fk, FIELD_LINK_CHILD,
+ &child_links_size);
+ if (child_links == NULL)
+ goto tnt_error;
/*
* We are allocating memory for both parent and child
* arrays in the same chunk. Thus, first OP_Blob opcode
@@ -1633,7 +1630,7 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
* as dynamic and releases memory.
*/
char *raw = sqlite3DbMallocRaw(parse_context->db,
- parent_links_size + child_links_size);
+ parent_links_size + child_links_size);
if (raw == NULL) {
sqlite3DbFree(parse_context->db, name_copy);
return;
@@ -1654,6 +1651,10 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
constr_tuple_reg + 9);
sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
sqlite3ReleaseTempRange(parse_context, constr_tuple_reg, 10);
+tnt_error:
+ parse_context->nErr++;
+ parse_context->rc = SQL_TARANTOOL_ERROR;
+ sqlite3DbFree(parse_context->db, name_copy);
> + if (parent_links != NULL) {
> + child_links = fkey_encode_links(region, fk, FIELD_LINK_CHILD,
> + &child_links_size);
> + }
> + if (parent_links == NULL || child_links == NULL) {
> + parse_context->nErr++;
> + parse_context->rc = SQL_TARANTOOL_ERROR;
> + sqlite3DbFree(parse_context->db, name_copy);
> + return;
> + }
> /*
> * We are allocating memory for both parent and child
> * arrays in the same chunk. Thus, first OP_Blob opcode
> * interprets it as static memory, and the second one -
> * as dynamic and releases memory.
> */
> - char *parent_links = sqlite3DbMallocRaw(parse_context->db,
> - parent_size + child_size);
> - if (parent_links == NULL) {
> - sqlite3DbFree(parse_context->db, (void *) name_copy);
> + char *raw = sqlite3DbMallocRaw(parse_context->db,
> + parent_links_size + child_links_size);
> + if (raw == NULL) {
> + sqlite3DbFree(parse_context->db, name_copy);
> return;
> }
> - parent_size = fkey_encode_links(fk, FIELD_LINK_PARENT, parent_links);
> - char *child_links = parent_links + parent_size;
> - child_size = fkey_encode_links(fk, FIELD_LINK_CHILD, child_links);
> - sqlite3VdbeAddOp4(vdbe, OP_Blob, child_size, constr_tuple_reg + 7,
> + memcpy(raw, parent_links, parent_links_size);
> + parent_links = raw;
> + raw += parent_links_size;
> + memcpy(raw, child_links, child_links_size);
> + child_links = raw;
> +
> + sqlite3VdbeAddOp4(vdbe, OP_Blob, child_links_size, constr_tuple_reg + 7,
> SQL_SUBTYPE_MSGPACK, child_links, P4_STATIC);
> - sqlite3VdbeAddOp4(vdbe, OP_Blob, parent_size, constr_tuple_reg + 8,
> + sqlite3VdbeAddOp4(vdbe, OP_Blob, parent_links_size, constr_tuple_reg + 8,
Out of 80 chars.
> SQL_SUBTYPE_MSGPACK, parent_links, P4_DYNAMIC);
> sqlite3VdbeAddOp3(vdbe, OP_MakeRecord, constr_tuple_reg, 9,
> constr_tuple_reg + 9);
> diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
> index 94517f6..b158cc7 100644
> --- a/src/box/sql/tarantoolInt.h
> +++ b/src/box/sql/tarantoolInt.h
> @@ -137,46 +137,74 @@ tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor,
> int
> tarantoolSqlite3IncrementMaxid(uint64_t *space_max_id);
>
> -/*
> - * Render "format" array for _space entry.
> - * Returns result size.
> - * If buf==NULL estimate result size.
> +/**
> + * Encode Table AST to msgpack on @region.
Table doesn’t feature any AST. I like previous comment:
“Encode format as entry to be inserted to _space” or smth like that.
> + *
> + * @param region to use.
Don’t cut arg names. Ex.:
@param region Region to use
The same for other functions.
> + * @param table AST to encode.
> + * @param[out] size of result allocation.
> + * @retval NULL on error.
> + * @retval not NULL msgpack pointer on success.
Pointer to encoded into mspack format?
> */
> -int tarantoolSqlite3MakeTableFormat(Table * pTable, void *buf);
> +char *
> +sql_encode_table(struct region *region, struct Table *table, uint32_t *size);
>
> -/*
> - * Format "opts" dictionary for _space entry.
> - * Returns result size.
> - * If buf==NULL estimate result size.
> +/**
> + * Encode "opts" dictionary for _space entry on @region.
> + *
> + * @param region to use.
> + * @param table AST containing opts to encode.
> + * @param sql source request to encode.
> + * @param[out] size of result allocation.
> + * @retval NULL on error.
> + * @retval not NULL msgpack pointer on success.
> */
> -int tarantoolSqlite3MakeTableOpts(Table * pTable, const char *zSql, char *buf);
> +char *
> +sql_encode_table_opts(struct region *region, struct Table *table,
> + const char *sql, uint32_t *size);
>
> /**
> - * Encode links of given foreign key constraint into MsgPack.
> + * Encode links of given foreign key constraint into MsgPack on
> + * @region.
> *
> + * @param region to use.
> * @param def FK def to encode links of.
> * @param type Links type to encode.
> - * @param buf Buffer to hold encoded links. Can be NULL. In this
> - * case function would simply calculate memory required
> - * for such buffer.
> - * @retval Length of encoded array.
> + * @param[out] size of result allocation.
> + * @retval NULL on error.
> + * @retval not NULL msgpack pointer on success.
> */
> -int
> -fkey_encode_links(const struct fkey_def *def, int type, char *buf);
> +char *
> +fkey_encode_links(struct region *region, const struct fkey_def *def, int type,
> + uint32_t *size);
>
> -/*
> - * Format "parts" array for _index entry.
> - * Returns result size.
> - * If buf==NULL estimate result size.
> +/**
> + * Encode links of given foreign key constraint into MsgPack on
> + * @region.
??? I guess comment doesn’t belong to this function.
> + *
> + * @param region to use.
> + * @param index to encode.
> + * @param[out] size of result allocation.
> + * @retval NULL on error.
> + * @retval not NULL msgpack pointer on success.
> */
> -int tarantoolSqlite3MakeIdxParts(Index * index, void *buf);
> +char *
> +sql_encode_index_parts(struct region *region, struct Index *index,
> + uint32_t *size);
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: remove struct Enc
2018-08-21 12:09 ` [tarantool-patches] Re: [PATCH v1 1/1] sql: remove struct Enc n.pettik
@ 2018-08-21 15:50 ` Kirill Shcherbatov
2018-08-24 16:33 ` Vladislav Shpilevoy
0 siblings, 1 reply; 8+ messages in thread
From: Kirill Shcherbatov @ 2018-08-21 15:50 UTC (permalink / raw)
To: n.pettik, tarantool-patches
> In general parch LGTM except several nitpickings.
Hi! Thank you for review.
> I guess this comment is no longer relevant: VIEW column names are
> stored in def->fields. You can remove additional check:
- /*
- * a[i].pExpr could be NULL for VIEW column names
- * represented as checks.
- */
- if (a[i].pExpr != NULL) {
- struct Expr *pExpr = a[i].pExpr;
- assert(pExpr->u.zToken != NULL);
- mpstream_encode_str(&stream, "expr", strlen("expr"));
- mpstream_encode_str(&stream, pExpr->u.zToken,
- strlen(pExpr->u.zToken));
- }
+ assert(a[i].pExpr != NULL);
+ struct Expr *pExpr = a[i].pExpr;
+ assert(pExpr->u.zToken != NULL);
+ mpstream_encode_str(&stream, "expr", strlen("expr"));
+ mpstream_encode_str(&stream, pExpr->u.zToken,
+ strlen(pExpr->u.zToken));
> Nitpicking: why do you use uint32_t meanwhile region_used() return size_t?
> The same for other functions.
done.
> Nitpicking: I would combine declaration and first usage.
> The same for index_opts_sz and index_parts_sz.
> Overall, consider following refactoring:
Done.
> Why do you pass region as param if you anyway get it from global fiber()?
- struct region *region = &fiber()->gc;
+ struct region *region = &pParse->region;
> Wrong indentation.
Fixed.
> The same comments to this functions as to previous one
> and the same refactoring.
done
> Out of 80 chars.
done
> Pointer to encoded into mspack format?
> ??? I guess comment doesn’t belong to this function.
Fixed.
===========================================
diff --git a/src/box/sql.c b/src/box/sql.c
index ae12cae..28ad9d3 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -56,6 +56,8 @@
#include "xrow.h"
#include "iproto_constants.h"
#include "fkey.h"
+#include "mpstream.h"
+#include "small/region.h"
static sqlite3 *db = NULL;
@@ -1190,66 +1192,6 @@ void tarantoolSqlite3LoadSchema(struct init_data *init)
*/
/*
- * Resulting data is of the variable length. Routines are called twice:
- * 1. with a NULL buffer, yielding result size estimation;
- * 2. with a buffer of the estimated size, rendering the result.
- *
- * For convenience, formatting routines use Enc structure to call
- * Enc is either configured to perform size estimation
- * or to render the result.
- */
-struct Enc {
- char *(*encode_uint)(char *data, uint64_t num);
- char *(*encode_str)(char *data, const char *str, uint32_t len);
- char *(*encode_bool)(char *data, bool v);
- char *(*encode_array)(char *data, uint32_t len);
- char *(*encode_map)(char *data, uint32_t len);
-};
-
-/* no_encode_XXX functions estimate result size */
-
-static char *no_encode_uint(char *data, uint64_t num)
-{
- /* MsgPack UINT is encoded in 9 bytes or less */
- (void)num; return data + 9;
-}
-
-static char *no_encode_str(char *data, const char *str, uint32_t len)
-{
- /* MsgPack STR header is encoded in 5 bytes or less, followed by
- * the string data. */
- (void)str; return data + 5 + len;
-}
-
-static char *no_encode_bool(char *data, bool v)
-{
- /* MsgPack BOOL is encoded in 1 byte. */
- (void)v; return data + 1;
-}
-
-static char *no_encode_array_or_map(char *data, uint32_t len)
-{
- /* MsgPack ARRAY or MAP header is encoded in 5 bytes or less. */
- (void)len; return data + 5;
-}
-
-/*
- * If buf==NULL, return Enc that will perform size estimation;
- * otherwize, return Enc that renders results in the provided buf.
- */
-static const struct Enc *get_enc(void *buf)
-{
- static const struct Enc mp_enc = {
- mp_encode_uint, mp_encode_str, mp_encode_bool,
- mp_encode_array, mp_encode_map
- }, no_enc = {
- no_encode_uint, no_encode_str, no_encode_bool,
- no_encode_array_or_map, no_encode_array_or_map
- };
- return buf ? &mp_enc : &no_enc;
-}
-
-/*
* Convert SQLite affinity value to the corresponding Tarantool type
* string which is suitable for _index.parts field.
*/
@@ -1278,34 +1220,38 @@ static const char *convertSqliteAffinity(int affinity, bool allow_nulls)
}
}
-/*
- * Render "format" array for _space entry.
- * Returns result size.
- * If buf==NULL estimate result size.
- *
- * Ex: [{"name": "col1", "type": "integer"}, ... ]
- */
-int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
+static void
+set_encode_error(void *error_ctx)
{
- const struct Enc *enc = get_enc(buf);
- const struct space_def *def = pTable->def;
- assert(def != NULL);
- struct SqliteIndex *pk_idx = sqlite3PrimaryKeyIndex(pTable);
- int pk_forced_int = -1;
- char *base = buf, *p;
- int i, n = def->field_count;
+ bool *is_error = error_ctx;
+ *is_error = true;
+}
- p = enc->encode_array(base, n);
+char *
+sql_encode_table(struct region *region, struct Table *table, 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);
- /* If table's PK is single column which is INTEGER, then
- * treat it as strict type, not affinity. */
+ const struct space_def *def = table->def;
+ assert(def != NULL);
+ /*
+ * If table's PK is single column which is INTEGER, then
+ * treat it as strict type, not affinity.
+ */
+ struct SqliteIndex *pk_idx = sqlite3PrimaryKeyIndex(table);
+ uint32_t pk_forced_int = UINT32_MAX;
if (pk_idx != NULL && pk_idx->def->key_def->part_count == 1) {
int pk = pk_idx->def->key_def->parts[0].fieldno;
if (def->fields[pk].type == FIELD_TYPE_INTEGER)
pk_forced_int = pk;
}
-
- for (i = 0; i < n; i++) {
+ uint32_t field_count = def->field_count;
+ mpstream_encode_array(&stream, field_count);
+ for (uint32_t i = 0; i < field_count && !is_error; i++) {
const char *t;
uint32_t cid = def->fields[i].coll_id;
struct field_def *field = &def->fields[i];
@@ -1315,10 +1261,10 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
base_len += 1;
if (default_str != NULL)
base_len += 1;
- p = enc->encode_map(p, base_len);
- p = enc->encode_str(p, "name", 4);
- p = enc->encode_str(p, field->name, strlen(field->name));
- p = enc->encode_str(p, "type", 4);
+ mpstream_encode_map(&stream, base_len);
+ mpstream_encode_str(&stream, "name", strlen("name"));
+ mpstream_encode_str(&stream, field->name, strlen(field->name));
+ mpstream_encode_str(&stream, "type", strlen("type"));
if (i == pk_forced_int) {
t = "integer";
} else {
@@ -1329,108 +1275,149 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
}
assert(def->fields[i].is_nullable ==
- action_is_nullable(def->fields[i].nullable_action));
- p = enc->encode_str(p, t, strlen(t));
- p = enc->encode_str(p, "affinity", 8);
- p = enc->encode_uint(p, def->fields[i].affinity);
- p = enc->encode_str(p, "is_nullable", 11);
- p = enc->encode_bool(p, def->fields[i].is_nullable);
- p = enc->encode_str(p, "nullable_action", 15);
+ action_is_nullable(def->fields[i].
+ nullable_action));
+ mpstream_encode_str(&stream, t, strlen(t));
+ mpstream_encode_str(&stream, "affinity", strlen("affinity"));
+ mpstream_encode_uint(&stream,
+ def->fields[i].affinity);
+ mpstream_encode_str(&stream, "is_nullable",
+ strlen("is_nullable"));
+ mpstream_encode_bool(&stream,
+ def->fields[i].is_nullable);
+ mpstream_encode_str(&stream, "nullable_action",
+ strlen("nullable_action"));
assert(def->fields[i].nullable_action < on_conflict_action_MAX);
const char *action =
on_conflict_action_strs[def->fields[i].nullable_action];
- p = enc->encode_str(p, action, strlen(action));
+ mpstream_encode_str(&stream, action, strlen(action));
if (cid != COLL_NONE) {
- p = enc->encode_str(p, "collation", strlen("collation"));
- p = enc->encode_uint(p, cid);
+ mpstream_encode_str(&stream, "collation",
+ strlen("collation"));
+ mpstream_encode_uint(&stream, cid);
}
if (default_str != NULL) {
- p = enc->encode_str(p, "default", strlen("default"));
- p = enc->encode_str(p, default_str, strlen(default_str));
+ mpstream_encode_str(&stream, "default",
+ strlen("default"));
+ mpstream_encode_str(&stream, default_str,
+ strlen(default_str));
}
}
- return (int)(p - base);
+ mpstream_flush(&stream);
+ if (is_error)
+ return NULL;
+ size_t sz = region_used(region) - used;
+ char *raw = region_join(region, sz);
+ if (raw == NULL)
+ diag_set(OutOfMemory, sz, "region_join", "raw");
+ else
+ *size = sz;
+ return raw;
}
-/*
- * Format "opts" dictionary for _space entry.
- * Returns result size.
- * If buf==NULL estimate result size.
- *
- * Ex: {"temporary": true, "sql": "CREATE TABLE student (name, grade)"}
- */
-int
-tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, char *buf)
+char *
+sql_encode_table_opts(struct region *region, struct Table *table,
+ const char *sql, uint32_t *size)
{
- const struct Enc *enc = get_enc(buf);
- bool is_view = pTable != NULL && pTable->def->opts.is_view;
- bool has_checks = pTable != NULL && pTable->def->opts.checks != NULL;
- int checks_cnt = has_checks ? pTable->def->opts.checks->nExpr : 0;
- char *p = enc->encode_map(buf, 1 + is_view + (checks_cnt > 0));
-
- p = enc->encode_str(p, "sql", 3);
- p = enc->encode_str(p, zSql, strlen(zSql));
+ assert(sql != NULL);
+ 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);
+ bool is_view = false;
+ bool has_checks = false;
+ if (table != NULL) {
+ is_view = table->def->opts.is_view;
+ has_checks = table->def->opts.checks != NULL;
+ }
+ uint32_t checks_cnt = has_checks ? table->def->opts.checks->nExpr : 0;
+ mpstream_encode_map(&stream, 1 + is_view + (checks_cnt > 0));
+
+ mpstream_encode_str(&stream, "sql", strlen("sql"));
+ mpstream_encode_str(&stream, sql, strlen(sql));
if (is_view) {
- p = enc->encode_str(p, "view", 4);
- p = enc->encode_bool(p, true);
+ mpstream_encode_str(&stream, "view", strlen("view"));
+ mpstream_encode_bool(&stream, true);
}
if (checks_cnt == 0)
- return p - buf;
+ goto finish;
+
/* Encode checks. */
- struct ExprList_item *a = pTable->def->opts.checks->a;
- p = enc->encode_str(p, "checks", 6);
- p = enc->encode_array(p, checks_cnt);
- for (int i = 0; i < checks_cnt; ++i) {
+ struct ExprList_item *a = table->def->opts.checks->a;
+ mpstream_encode_str(&stream, "checks", strlen("checks"));
+ mpstream_encode_array(&stream, checks_cnt);
+ for (uint32_t i = 0; i < checks_cnt && !is_error; ++i) {
int items = (a[i].pExpr != NULL) + (a[i].zName != NULL);
- p = enc->encode_map(p, items);
- /*
- * a[i].pExpr could be NULL for VIEW column names
- * represented as checks.
- */
- if (a[i].pExpr != NULL) {
- Expr *pExpr = a[i].pExpr;
- assert(pExpr->u.zToken != NULL);
- p = enc->encode_str(p, "expr", 4);
- p = enc->encode_str(p, pExpr->u.zToken,
- strlen(pExpr->u.zToken));
- }
+ mpstream_encode_map(&stream, items);
+ assert(a[i].pExpr != NULL);
+ struct Expr *pExpr = a[i].pExpr;
+ assert(pExpr->u.zToken != NULL);
+ mpstream_encode_str(&stream, "expr", strlen("expr"));
+ mpstream_encode_str(&stream, pExpr->u.zToken,
+ strlen(pExpr->u.zToken));
if (a[i].zName != NULL) {
- p = enc->encode_str(p, "name", 4);
- p = enc->encode_str(p, a[i].zName, strlen(a[i].zName));
+ mpstream_encode_str(&stream, "name", strlen("name"));
+ mpstream_encode_str(&stream, a[i].zName,
+ strlen(a[i].zName));
}
}
- return p - buf;
+
+finish:
+ mpstream_flush(&stream);
+ if (is_error)
+ return NULL;
+ size_t sz = region_used(region) - used;
+ char *raw = region_join(region, sz);
+ if (raw == NULL)
+ diag_set(OutOfMemory, sz, "region_join", "raw");
+ else
+ *size = sz;
+ return raw;
}
-int
-fkey_encode_links(const struct fkey_def *def, int type, char *buf)
+char *
+fkey_encode_links(struct region *region, const struct fkey_def *def, int type,
+ uint32_t *size)
{
- const struct Enc *enc = get_enc(buf);
- char *p = enc->encode_array(buf, def->field_count);
- for (uint32_t i = 0; i < def->field_count; ++i)
- p = enc->encode_uint(p, def->links[i].fields[type]);
- return p - buf;
+ 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);
+ uint32_t field_count = def->field_count;
+ mpstream_encode_array(&stream, field_count);
+ for (uint32_t i = 0; i < field_count && !is_error; ++i)
+ mpstream_encode_uint(&stream, def->links[i].fields[type]);
+ mpstream_flush(&stream);
+ if (is_error)
+ return NULL;
+ size_t sz = region_used(region) - used;
+ char *raw = region_join(region, sz);
+ if (raw == NULL)
+ diag_set(OutOfMemory, sz, "region_join", "raw");
+ else
+ *size = sz;
+ return raw;
}
-/*
- * Format "parts" array for _index entry.
- * Returns result size.
- * If buf==NULL estimate result size.
- *
- * Ex: [[0, "integer"]]
- */
-int tarantoolSqlite3MakeIdxParts(SqliteIndex *pIndex, void *buf)
+char *
+sql_encode_index_parts(struct region *region, struct SqliteIndex *index,
+ uint32_t *size)
{
- struct field_def *fields = pIndex->pTable->def->fields;
- struct key_def *key_def = pIndex->def->key_def;
- const struct Enc *enc = get_enc(buf);
- char *base = buf;
+ 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);
+ /*
+ * If table's PK is single column which is INTEGER, then
+ * treat it as strict type, not affinity.
+ */
uint32_t pk_forced_int = UINT32_MAX;
struct SqliteIndex *primary_index =
- sqlite3PrimaryKeyIndex(pIndex->pTable);
-
- /* If table's PK is single column which is INTEGER, then
- * treat it as strict type, not affinity. */
+ sqlite3PrimaryKeyIndex(index->pTable);
+ struct field_def *fields = index->pTable->def->fields;
if (primary_index->def->key_def->part_count == 1) {
int pk = primary_index->def->key_def->parts[0].fieldno;
if (fields[pk].type == FIELD_TYPE_INTEGER)
@@ -1443,9 +1430,11 @@ int tarantoolSqlite3MakeIdxParts(SqliteIndex *pIndex, void *buf)
* primary key columns. Query planner depends on this particular
* data layout.
*/
+ struct key_def *key_def = index->def->key_def;
+ uint32_t part_count = key_def->part_count;
struct key_part *part = key_def->parts;
- char *p = enc->encode_array(base, key_def->part_count);
- for (uint32_t i = 0; i < key_def->part_count; ++i, ++part) {
+ mpstream_encode_array(&stream, part_count);
+ for (uint32_t i = 0; i < part_count; ++i, ++part) {
uint32_t col = part->fieldno;
assert(fields[col].is_nullable ==
action_is_nullable(fields[col].nullable_action));
@@ -1456,54 +1445,60 @@ int tarantoolSqlite3MakeIdxParts(SqliteIndex *pIndex, void *buf)
t = convertSqliteAffinity(fields[col].affinity,
fields[col].is_nullable);
}
- /* do not decode default collation */
+ /* Do not decode default collation. */
uint32_t cid = part->coll_id;
- p = enc->encode_map(p, cid == COLL_NONE ? 5 : 6);
- p = enc->encode_str(p, "type", sizeof("type")-1);
- p = enc->encode_str(p, t, strlen(t));
- p = enc->encode_str(p, "field", sizeof("field")-1);
- p = enc->encode_uint(p, col);
+ mpstream_encode_map(&stream, 5 + (cid != COLL_NONE));
+ mpstream_encode_str(&stream, "type", strlen("type"));
+ mpstream_encode_str(&stream, t, strlen(t));
+ mpstream_encode_str(&stream, "field", strlen("field"));
+ mpstream_encode_uint(&stream, col);
if (cid != COLL_NONE) {
- p = enc->encode_str(p, "collation",
- sizeof("collation") - 1);
- p = enc->encode_uint(p, cid);
+ mpstream_encode_str(&stream, "collation",
+ strlen("collation"));
+ mpstream_encode_uint(&stream, cid);
}
- p = enc->encode_str(p, "is_nullable", 11);
- p = enc->encode_bool(p, fields[col].is_nullable);
- p = enc->encode_str(p, "nullable_action", 15);
+ mpstream_encode_str(&stream, "is_nullable",
+ strlen("is_nullable"));
+ mpstream_encode_bool(&stream,
+ fields[col].is_nullable);
+ mpstream_encode_str(&stream, "nullable_action",
+ strlen("nullable_action"));
const char *action_str =
on_conflict_action_strs[fields[col].nullable_action];
- p = enc->encode_str(p, action_str, strlen(action_str));
+ mpstream_encode_str(&stream, action_str, strlen(action_str));
- p = enc->encode_str(p, "sort_order", 10);
+ mpstream_encode_str(&stream, "sort_order",
+ strlen("sort_order"));
enum sort_order sort_order = part->sort_order;
assert(sort_order < sort_order_MAX);
const char *sort_order_str = sort_order_strs[sort_order];
- p = enc->encode_str(p, sort_order_str, strlen(sort_order_str));
+ mpstream_encode_str(&stream, sort_order_str,
+ strlen(sort_order_str));
}
- return p - base;
+ mpstream_flush(&stream);
+ if (is_error)
+ return NULL;
+ size_t sz = region_used(region) - used;
+ char *raw = region_join(region, sz);
+ if (raw == NULL)
+ diag_set(OutOfMemory, sz, "region_join", "raw");
+ else
+ *size = sz;
+ return raw;
}
-/*
- * Format "opts" dictionary for _index entry.
- * Returns result size.
- * If buf==NULL estimate result size.
- *
- * Ex: {
- * "unique": "true",
- * "sql": "CREATE INDEX student_by_name ON students(name)"
- * }
- */
-int tarantoolSqlite3MakeIdxOpts(SqliteIndex *index, const char *zSql, void *buf)
+char *
+sql_encode_index_opts(struct region *region, struct SqliteIndex *index,
+ const char *sql, uint32_t *size)
{
- const struct Enc *enc = get_enc(buf);
- char *base = buf, *p;
-
- (void)index;
-
- p = enc->encode_map(base, 2);
+ 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 + (sql != NULL));
/* Mark as unique pk and unique indexes */
- p = enc->encode_str(p, "unique", 6);
+ mpstream_encode_str(&stream, "unique", strlen("unique"));
/* If user didn't defined ON CONFLICT OPTIONS, all uniqueness checks
* will be made by Tarantool. However, Tarantool doesn't have ON
* CONFLIT option, so in that case (except ON CONFLICT ABORT, which is
@@ -1511,10 +1506,21 @@ int tarantoolSqlite3MakeIdxOpts(SqliteIndex *index, const char *zSql, void *buf)
* INSERT OR REPLACE/IGNORE uniqueness checks will be also done by
* Tarantool.
*/
- p = enc->encode_bool(p, IsUniqueIndex(index));
- p = enc->encode_str(p, "sql", 3);
- p = enc->encode_str(p, zSql, zSql ? strlen(zSql) : 0);
- return (int)(p - base);
+ mpstream_encode_bool(&stream, IsUniqueIndex(index));
+ if (sql != NULL) {
+ mpstream_encode_str(&stream, "sql", strlen("sql"));
+ mpstream_encode_str(&stream, sql, strlen(sql));
+ }
+ mpstream_flush(&stream);
+ if (is_error)
+ return NULL;
+ size_t sz = region_used(region) - used;
+ char *raw = region_join(region, sz);
+ if (raw == NULL)
+ diag_set(OutOfMemory, sz, "region_join", "raw");
+ else
+ *size = sz;
+ return raw;
}
void
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index dddeb12..462b6e6 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1241,25 +1241,27 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId,
Vdbe *v = sqlite3GetVdbe(pParse);
int iFirstCol = ++pParse->nMem;
int iRecord = (pParse->nMem += 6); /* 6 total columns */
- char *zOpts, *zParts;
- int zOptsSz, zPartsSz;
-
- /* Format "opts" and "parts" for _index entry. */
- zOpts = sqlite3DbMallocRaw(pParse->db,
- tarantoolSqlite3MakeIdxOpts(pIndex, zSql,
- NULL) +
- tarantoolSqlite3MakeIdxParts(pIndex,
- NULL) + 2);
- if (!zOpts)
+ struct region *region = &pParse->region;
+ uint32_t index_opts_sz = 0;
+ char *index_opts =
+ sql_encode_index_opts(region, pIndex, zSql, &index_opts_sz);
+ if (index_opts == NULL)
+ goto error;
+ uint32_t index_parts_sz = 0;
+ char *index_parts =
+ sql_encode_index_parts(region, pIndex, &index_parts_sz);
+ if (index_parts == NULL)
+ goto error;
+ char *raw = sqlite3DbMallocRaw(pParse->db,
+ index_opts_sz + index_parts_sz);
+ if (raw == NULL)
return;
- zOptsSz = tarantoolSqlite3MakeIdxOpts(pIndex, zSql, zOpts);
- zParts = zOpts + zOptsSz + 1;
- zPartsSz = tarantoolSqlite3MakeIdxParts(pIndex, zParts);
-#if SQLITE_DEBUG
- /* NUL-termination is necessary for VDBE trace facility only */
- zOpts[zOptsSz] = 0;
- zParts[zPartsSz] = 0;
-#endif
+
+ memcpy(raw, index_opts, index_opts_sz);
+ index_opts = raw;
+ raw += index_opts_sz;
+ memcpy(raw, index_parts, index_parts_sz);
+ index_parts = raw;
if (pParse->pNewTable) {
int reg;
@@ -1295,16 +1297,20 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId,
P4_DYNAMIC);
sqlite3VdbeAddOp4(v, OP_String8, 0, iFirstCol + 3, 0, "tree",
P4_STATIC);
- sqlite3VdbeAddOp4(v, OP_Blob, zOptsSz, iFirstCol + 4,
- SQL_SUBTYPE_MSGPACK, zOpts, P4_DYNAMIC);
+ sqlite3VdbeAddOp4(v, OP_Blob, index_opts_sz, iFirstCol + 4,
+ SQL_SUBTYPE_MSGPACK, index_opts, P4_DYNAMIC);
/* zOpts and zParts are co-located, hence STATIC */
- sqlite3VdbeAddOp4(v, OP_Blob, zPartsSz, iFirstCol + 5,
- SQL_SUBTYPE_MSGPACK,zParts, P4_STATIC);
+ sqlite3VdbeAddOp4(v, OP_Blob, index_parts_sz, iFirstCol + 5,
+ SQL_SUBTYPE_MSGPACK, index_parts, P4_STATIC);
sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 6, iRecord);
sqlite3VdbeAddOp2(v, OP_SInsert, BOX_INDEX_ID, iRecord);
if (pIndex->index_type == SQL_INDEX_TYPE_NON_UNIQUE ||
pIndex->index_type == SQL_INDEX_TYPE_UNIQUE)
sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+ return;
+error:
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
}
/*
@@ -1359,50 +1365,55 @@ makeIndexSchemaRecord(Parse * pParse,
static void
createSpace(Parse * pParse, int iSpaceId, char *zStmt)
{
- Table *p = pParse->pNewTable;
+ struct Table *table = pParse->pNewTable;
Vdbe *v = sqlite3GetVdbe(pParse);
int iFirstCol = ++pParse->nMem;
int iRecord = (pParse->nMem += 7);
- char *zOpts, *zFormat;
- int zOptsSz, zFormatSz;
-
- zOpts = sqlite3DbMallocRaw(pParse->db,
- tarantoolSqlite3MakeTableFormat(p, NULL) +
- tarantoolSqlite3MakeTableOpts(p, zStmt,
- NULL) + 2);
- if (!zOpts) {
- zOptsSz = 0;
- zFormat = NULL;
- zFormatSz = 0;
- } else {
- zOptsSz = tarantoolSqlite3MakeTableOpts(p, zStmt, zOpts);
- zFormat = zOpts + zOptsSz + 1;
- zFormatSz = tarantoolSqlite3MakeTableFormat(p, zFormat);
-#if SQLITE_DEBUG
- /* NUL-termination is necessary for VDBE-tracing facility only */
- zOpts[zOptsSz] = 0;
- zFormat[zFormatSz] = 0;
-#endif
- }
+ struct region *region = &pParse->region;
+ uint32_t table_opts_stmt_sz = 0;
+ char *table_opts_stmt =
+ sql_encode_table_opts(region, table, zStmt,
+ &table_opts_stmt_sz);
+ if (table_opts_stmt == NULL)
+ goto error;
+ uint32_t table_stmt_sz = 0;
+ char *table_stmt = sql_encode_table(region, table, &table_stmt_sz);
+ if (table_stmt == NULL)
+ goto error;
+ char *raw = sqlite3DbMallocRaw(pParse->db,
+ table_stmt_sz + table_opts_stmt_sz);
+ if (raw == NULL)
+ return;
+
+ memcpy(raw, table_opts_stmt, table_opts_stmt_sz);
+ table_opts_stmt = raw;
+ raw += table_opts_stmt_sz;
+ memcpy(raw, table_stmt, table_stmt_sz);
+ table_stmt = raw;
sqlite3VdbeAddOp2(v, OP_SCopy, iSpaceId, iFirstCol /* spaceId */ );
sqlite3VdbeAddOp2(v, OP_Integer, effective_user()->uid,
iFirstCol + 1 /* owner */ );
sqlite3VdbeAddOp4(v, OP_String8, 0, iFirstCol + 2 /* name */ , 0,
- sqlite3DbStrDup(pParse->db, p->def->name), P4_DYNAMIC);
+ sqlite3DbStrDup(pParse->db, table->def->name),
+ P4_DYNAMIC);
sqlite3VdbeAddOp4(v, OP_String8, 0, iFirstCol + 3 /* engine */ , 0,
- sqlite3DbStrDup(pParse->db, p->def->engine_name),
+ sqlite3DbStrDup(pParse->db, table->def->engine_name),
P4_DYNAMIC);
- sqlite3VdbeAddOp2(v, OP_Integer, p->def->field_count,
+ sqlite3VdbeAddOp2(v, OP_Integer, table->def->field_count,
iFirstCol + 4 /* field_count */ );
- sqlite3VdbeAddOp4(v, OP_Blob, zOptsSz, iFirstCol + 5,
- SQL_SUBTYPE_MSGPACK, zOpts, P4_DYNAMIC);
+ sqlite3VdbeAddOp4(v, OP_Blob, table_opts_stmt_sz, iFirstCol + 5,
+ SQL_SUBTYPE_MSGPACK, table_opts_stmt, P4_DYNAMIC);
/* zOpts and zFormat are co-located, hence STATIC */
- sqlite3VdbeAddOp4(v, OP_Blob, zFormatSz, iFirstCol + 6,
- SQL_SUBTYPE_MSGPACK, zFormat, P4_STATIC);
+ sqlite3VdbeAddOp4(v, OP_Blob, table_stmt_sz, iFirstCol + 6,
+ SQL_SUBTYPE_MSGPACK, table_stmt, P4_STATIC);
sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, iRecord);
sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, iRecord);
sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+ return;
+error:
+ pParse->nErr++;
+ pParse->rc = SQL_TARANTOOL_ERROR;
}
/*
@@ -1603,33 +1614,53 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
fkey_action_strs[fk->on_delete], P4_STATIC);
sqlite3VdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 6, 0,
fkey_action_strs[fk->on_update], P4_STATIC);
- size_t parent_size = fkey_encode_links(fk, FIELD_LINK_PARENT, NULL);
- size_t child_size = fkey_encode_links(fk, FIELD_LINK_CHILD, NULL);
+ struct region *region = &parse_context->region;
+ uint32_t parent_links_size = 0;
+ char *parent_links =
+ fkey_encode_links(region, fk, FIELD_LINK_PARENT,
+ &parent_links_size);
+ if (parent_links == NULL)
+ goto error;
+ uint32_t child_links_size = 0;
+ char *child_links =
+ fkey_encode_links(region, fk, FIELD_LINK_CHILD,
+ &child_links_size);
+ if (child_links == NULL)
+ goto error;
/*
* We are allocating memory for both parent and child
* arrays in the same chunk. Thus, first OP_Blob opcode
* interprets it as static memory, and the second one -
* as dynamic and releases memory.
*/
- char *parent_links = sqlite3DbMallocRaw(parse_context->db,
- parent_size + child_size);
- if (parent_links == NULL) {
- sqlite3DbFree(parse_context->db, (void *) name_copy);
+ char *raw = sqlite3DbMallocRaw(parse_context->db,
+ parent_links_size + child_links_size);
+ if (raw == NULL) {
+ sqlite3DbFree(parse_context->db, name_copy);
return;
}
- parent_size = fkey_encode_links(fk, FIELD_LINK_PARENT, parent_links);
- char *child_links = parent_links + parent_size;
- child_size = fkey_encode_links(fk, FIELD_LINK_CHILD, child_links);
- sqlite3VdbeAddOp4(vdbe, OP_Blob, child_size, constr_tuple_reg + 7,
+ memcpy(raw, parent_links, parent_links_size);
+ parent_links = raw;
+ raw += parent_links_size;
+ memcpy(raw, child_links, child_links_size);
+ child_links = raw;
+
+ sqlite3VdbeAddOp4(vdbe, OP_Blob, child_links_size, constr_tuple_reg + 7,
SQL_SUBTYPE_MSGPACK, child_links, P4_STATIC);
- sqlite3VdbeAddOp4(vdbe, OP_Blob, parent_size, constr_tuple_reg + 8,
- SQL_SUBTYPE_MSGPACK, parent_links, P4_DYNAMIC);
+ sqlite3VdbeAddOp4(vdbe, OP_Blob, parent_links_size,
+ constr_tuple_reg + 8, SQL_SUBTYPE_MSGPACK,
+ parent_links, P4_DYNAMIC);
sqlite3VdbeAddOp3(vdbe, OP_MakeRecord, constr_tuple_reg, 9,
constr_tuple_reg + 9);
sqlite3VdbeAddOp2(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID,
constr_tuple_reg + 9);
sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
sqlite3ReleaseTempRange(parse_context, constr_tuple_reg, 10);
+ return;
+error:
+ parse_context->nErr++;
+ parse_context->rc = SQL_TARANTOOL_ERROR;
+ sqlite3DbFree(parse_context->db, name_copy);
}
/**
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index 94517f6..6a42ed3 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -137,46 +137,74 @@ tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor,
int
tarantoolSqlite3IncrementMaxid(uint64_t *space_max_id);
-/*
- * Render "format" array for _space entry.
- * Returns result size.
- * If buf==NULL estimate result size.
+/**
+ * Encode format as entry to be inserted to _space on @region.
+ *
+ * @param region region to use.
+ * @param table table to encode.
+ * @param[out] size size of result allocation.
+ * @retval NULL on error.
+ * @retval not NULL pointer to msgpack on success.
*/
-int tarantoolSqlite3MakeTableFormat(Table * pTable, void *buf);
+char *
+sql_encode_table(struct region *region, struct Table *table, uint32_t *size);
-/*
- * Format "opts" dictionary for _space entry.
- * Returns result size.
- * If buf==NULL estimate result size.
+/**
+ * Encode "opts" dictionary for _space entry on @region.
+ *
+ * @param region region to use.
+ * @param table table containing opts to encode.
+ * @param sql source request to encode.
+ * @param[out] size size of result allocation.
+ * @retval NULL on error.
+ * @retval not NULL pointer to msgpack on success
*/
-int tarantoolSqlite3MakeTableOpts(Table * pTable, const char *zSql, char *buf);
+char *
+sql_encode_table_opts(struct region *region, struct Table *table,
+ const char *sql, uint32_t *size);
/**
- * Encode links of given foreign key constraint into MsgPack.
+ * Encode links of given foreign key constraint into MsgPack on
+ * @region.
*
+ * @param region region to use.
* @param def FK def to encode links of.
* @param type Links type to encode.
- * @param buf Buffer to hold encoded links. Can be NULL. In this
- * case function would simply calculate memory required
- * for such buffer.
- * @retval Length of encoded array.
+ * @param[out] size size of result allocation.
+ * @retval NULL on error.
+ * @retval not NULL pointer to msgpack on success
*/
-int
-fkey_encode_links(const struct fkey_def *def, int type, char *buf);
+char *
+fkey_encode_links(struct region *region, const struct fkey_def *def, int type,
+ uint32_t *size);
-/*
- * Format "parts" array for _index entry.
- * Returns result size.
- * If buf==NULL estimate result size.
+/**
+ * Encode index parts of given foreign key constraint into
+ * MsgPack on @region.
+ *
+ * @param region region to use.
+ * @param index index to encode.
+ * @param[out] size size of result allocation.
+ * @retval NULL on error.
+ * @retval not NULL pointer to msgpack on success
*/
-int tarantoolSqlite3MakeIdxParts(Index * index, void *buf);
+char *
+sql_encode_index_parts(struct region *region, struct Index *index,
+ uint32_t *size);
-/*
- * Format "opts" dictionary for _index entry.
- * Returns result size.
- * If buf==NULL estimate result size.
+/**
+ * Encode "opts" dictionary for _index entry on @region.
+ *
+ * @param region region to use.
+ * @param index index to encode.
+ * @param sql source request to encode.
+ * @param[out] size size of result allocation.
+ * @retval NULL on error.
+ * @retval not NULL pointer to msgpack on success
*/
-int tarantoolSqlite3MakeIdxOpts(Index * index, const char *zSql, void *buf);
+char *
+sql_encode_index_opts(struct region *region, struct Index *index,
+ const char *sql, uint32_t *size);
/**
* Extract next id from _sequence space.
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index bd730c4..6a07081 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -225,20 +225,22 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
parse->nMem += 3;
int record = ++parse->nMem;
- opts_buff =
- sqlite3DbMallocRaw(parse->db,
- tarantoolSqlite3MakeTableOpts(0,
- sql_str,
- NULL) +
- 1);
- if (db->mallocFailed)
+ uint32_t opts_buff_sz = 0;
+ char *data = sql_encode_table_opts(&fiber()->gc, NULL, sql_str,
+ &opts_buff_sz);
+ if (data != NULL) {
+ opts_buff = sqlite3DbMallocRaw(parse->db, opts_buff_sz);
+ if (opts_buff != NULL)
+ memcpy(opts_buff, data, opts_buff_sz);
+ } else {
+ parse->nErr++;
+ parse->rc = SQL_TARANTOOL_ERROR;
+ }
+ if (opts_buff == NULL)
goto triggerfinish_cleanup;
- int opts_buff_sz =
- tarantoolSqlite3MakeTableOpts(0, sql_str, opts_buff);
-
trigger_name = sqlite3DbStrDup(parse->db, trigger_name);
- if (db->mallocFailed)
+ if (trigger_name == NULL)
goto triggerfinish_cleanup;
sqlite3VdbeAddOp4(v,
^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/2] box: export mpstream methods to core
2018-08-17 14:04 ` [tarantool-patches] [PATCH v1 1/2] box: export mpstream methods to core Kirill Shcherbatov
@ 2018-08-24 16:33 ` Vladislav Shpilevoy
0 siblings, 0 replies; 8+ messages in thread
From: Vladislav Shpilevoy @ 2018-08-24 16:33 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov; +Cc: korablev
Hi! Thanks for the patch! See 8 comments below.
1. Please, do not send a patchset in different mail threads.
On 17/08/2018 17:04, Kirill Shcherbatov wrote:
> As we going to use mpstream not only in LUA, let's move this
> API in tarantool core.
>
> Part of #3545.
> ---
> src/lua/msgpack.c | 85 +++----------------
> src/lua/msgpack.h | 53 +-----------
> src/mpstream.h | 238 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 248 insertions(+), 128 deletions(-)
> create mode 100644 src/mpstream.h
>
> diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
> index acd860a..bbcb886 100644
> --- a/src/lua/msgpack.c
> +++ b/src/lua/msgpack.c
> @@ -50,46 +50,6 @@ luamp_error(void *error_ctx)
> luaL_error(L, diag_last_error(diag_get())->errmsg);
> }
>
> -void
> -mpstream_init(struct mpstream *stream, void *ctx,
> - luamp_reserve_f reserve, luamp_alloc_f alloc,
> - luamp_error_f error, void *error_ctx)
> -{
> - stream->ctx = ctx;
> - stream->reserve = reserve;
> - stream->alloc = alloc;
> - stream->error = error;
> - stream->error_ctx = error_ctx;
> - mpstream_reset(stream);
> -}
> -
> -void
> -mpstream_reserve_slow(struct mpstream *stream, size_t size)
> -{
> - stream->alloc(stream->ctx, stream->pos - stream->buf);
> - stream->buf = (char *) stream->reserve(stream->ctx, &size);
> - if (stream->buf == NULL) {
> - diag_set(OutOfMemory, size, "mpstream", "reserve");
> - stream->error(stream->error_ctx);
> - }
> - stream->pos = stream->buf;
> - stream->end = stream->pos + size;
> -}
> -
> -void
> -mpstream_reset(struct mpstream *stream)
> -{
> - size_t size = 0;
> - stream->buf = (char *) stream->reserve(stream->ctx, &size);
> - if (stream->buf == NULL) {
> - diag_set(OutOfMemory, size, "mpstream", "reset");
> - stream->error(stream->error_ctx);
> - }
> - stream->pos = stream->buf;
> - stream->end = stream->pos + size;
> -}
> -
> -
> static uint32_t CTID_CHAR_PTR;
> static uint32_t CTID_STRUCT_IBUF;
>
> @@ -112,10 +72,7 @@ luamp_encode_array(struct luaL_serializer *cfg, struct mpstream *stream,
> uint32_t size)
> {
> (void) cfg;
> - assert(mp_sizeof_array(size) <= 5);
> - char *data = mpstream_reserve(stream, 5);
> - char *pos = mp_encode_array(data, size);
> - mpstream_advance(stream, pos - data);
> + mpstream_encode_array(stream, size);
> }
>
> void
> @@ -123,10 +80,7 @@ luamp_encode_map(struct luaL_serializer *cfg, struct mpstream *stream,
> uint32_t size)
> {
> (void) cfg;
> - assert(mp_sizeof_map(size) <= 5);
> - char *data = mpstream_reserve(stream, 5);
> - char *pos = mp_encode_map(data, size);
> - mpstream_advance(stream, pos - data);
> + mpstream_encode_map(stream, size);
> }
>
> void
> @@ -134,10 +88,7 @@ luamp_encode_uint(struct luaL_serializer *cfg, struct mpstream *stream,
> uint64_t num)
> {
> (void) cfg;
> - assert(mp_sizeof_uint(num) <= 9);
> - char *data = mpstream_reserve(stream, 9);
> - char *pos = mp_encode_uint(data, num);
> - mpstream_advance(stream, pos - data);
> + mpstream_encode_uint(stream, num);
> }
>
> void
> @@ -145,10 +96,7 @@ luamp_encode_int(struct luaL_serializer *cfg, struct mpstream *stream,
> int64_t num)
> {
> (void) cfg;
> - assert(mp_sizeof_int(num) <= 9);
> - char *data = mpstream_reserve(stream, 9);
> - char *pos = mp_encode_int(data, num);
> - mpstream_advance(stream, pos - data);
> + mpstream_encode_int(stream, num);
> }
>
> void
> @@ -156,10 +104,7 @@ luamp_encode_float(struct luaL_serializer *cfg, struct mpstream *stream,
> float num)
> {
> (void) cfg;
> - assert(mp_sizeof_float(num) <= 5);
> - char *data = mpstream_reserve(stream, 5);
> - char *pos = mp_encode_float(data, num);
> - mpstream_advance(stream, pos - data);
> + mpstream_encode_float(stream, num);
> }
>
> void
> @@ -167,10 +112,7 @@ luamp_encode_double(struct luaL_serializer *cfg, struct mpstream *stream,
> double num)
> {
> (void) cfg;
> - assert(mp_sizeof_double(num) <= 9);
> - char *data = mpstream_reserve(stream, 9);
> - char *pos = mp_encode_double(data, num);
> - mpstream_advance(stream, pos - data);
> + mpstream_encode_double(stream, num);
> }
>
> void
> @@ -178,20 +120,14 @@ luamp_encode_str(struct luaL_serializer *cfg, struct mpstream *stream,
> const char *str, uint32_t len)
> {
> (void) cfg;
> - assert(mp_sizeof_str(len) <= 5 + len);
> - char *data = mpstream_reserve(stream, 5 + len);
> - char *pos = mp_encode_str(data, str, len);
> - mpstream_advance(stream, pos - data);
> + mpstream_encode_str(stream, str, len);
> }
>
> void
> luamp_encode_nil(struct luaL_serializer *cfg, struct mpstream *stream)
> {
> (void) cfg;
> - assert(mp_sizeof_nil() <= 1);
> - char *data = mpstream_reserve(stream, 1);
> - char *pos = mp_encode_nil(data);
> - mpstream_advance(stream, pos - data);
> + mpstream_encode_nil(stream);
> }
>
> void
> @@ -199,10 +135,7 @@ luamp_encode_bool(struct luaL_serializer *cfg, struct mpstream *stream,
> bool val)
> {
> (void) cfg;
> - assert(mp_sizeof_bool(val) <= 1);
> - char *data = mpstream_reserve(stream, 1);
> - char *pos = mp_encode_bool(data, val);
> - mpstream_advance(stream, pos - data);
> + mpstream_encode_bool(stream, val);
> }
2. All these functions are useless now and can be
inlined in the places of usage. Please, do it.
>
> static enum mp_type
> diff --git a/src/lua/msgpack.h b/src/lua/msgpack.h
> index bacf3e0..d8ad202 100644
> --- a/src/lua/msgpack.h
> +++ b/src/lua/msgpack.h
> @@ -34,6 +34,7 @@
> #include <stdbool.h>
>
> #include "utils.h"
> +#include "mpstream.h"
3. It is enough to announce struct mpstream.
You do not need to include the whole header.
>
> #if defined(__cplusplus)
> extern "C" {
> diff --git a/src/mpstream.h b/src/mpstream.h
> new file mode 100644
> index 0000000..c8696f6
> --- /dev/null
> +++ b/src/mpstream.h
> @@ -0,0 +1,238 @@
> +#ifndef TARANTOOL_LUA_MPSTREAM_H_INCLUDED
> +#define TARANTOOL_LUA_MPSTREAM_H_INCLUDED
> +/*
> + * Copyright 2010-2015, Tarantool AUTHORS, please see AUTHORS file.
4. 2018
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + * copyright notice, this list of conditions and the
> + * following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following
> + * disclaimer in the documentation and/or other materials
> + * provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif /* defined(__cplusplus) */
5. Why do you need c++ protector, if this is
a header-only lib? By the way, I do not think it
is a true method. There is no point in making all
these functions static inline. Please, add a C
file and move these functions there.
> +
> +#include <stddef.h>
6. Why do you need stddef.h?
> +#include <assert.h>
> +#include <stdint.h>
> +#include "msgpuck.h"
> +#include "diag.h"
> +
> +/**
> +* Ask the allocator to reserve at least size bytes. It can reserve
> +* more, and update *size with the new size.
> +*/
> +typedef void *(*mpstream_reserve_f)(void *ctx, size_t *size);
7. Excessive white spaces. The same below.
> +
> +/** Actually use the bytes. */
> +typedef void *(*mpstream_alloc_f)(void *ctx, size_t size);
> +
> +/** Actually use the bytes. */
> +typedef void (*mpstream_error_f)(void *error_ctx);
> +
> +struct mpstream {
> + /**
> + * When pos >= end, or required size doesn't fit in
> + * pos..end range alloc() is called to advance the stream
> + * and reserve() to get a new chunk.
> + */
> + char *buf, *pos, *end;
> + void *ctx;
> + mpstream_reserve_f reserve;
> + mpstream_alloc_f alloc;
> + mpstream_error_f error;
> + void *error_ctx;
> +};
> +
> +static inline void
> +mpstream_reserve_slow(struct mpstream *stream, size_t size)
8. There was a reason, why reserve_slow is an independent function
and reserve is static inline. This is why in most cases reserve
just returns a current position, and rarely it falls down to
alloc(). Please, return as it was. Same for advance.
> +{
> + stream->alloc(stream->ctx, stream->pos - stream->buf);
> + stream->buf = (char *) stream->reserve(stream->ctx, &size);
> + if (stream->buf == NULL) {
> + diag_set(OutOfMemory, size, "mpstream", "reserve");
> + stream->error(stream->error_ctx);
> + }
> + stream->pos = stream->buf;
> + stream->end = stream->pos + size;
> +}
^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: remove struct Enc
2018-08-21 15:50 ` Kirill Shcherbatov
@ 2018-08-24 16:33 ` Vladislav Shpilevoy
0 siblings, 0 replies; 8+ messages in thread
From: Vladislav Shpilevoy @ 2018-08-24 16:33 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov, n.pettik
Thanks for the patch!
> diff --git a/src/box/sql.c b/src/box/sql.c
> index ae12cae..28ad9d3 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1315,10 +1261,10 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
> base_len += 1;
> if (default_str != NULL)
> base_len += 1;
> - p = enc->encode_map(p, base_len);
> - p = enc->encode_str(p, "name", 4);
> - p = enc->encode_str(p, field->name, strlen(field->name));
> - p = enc->encode_str(p, "type", 4);
> + mpstream_encode_map(&stream, base_len);
> + mpstream_encode_str(&stream, "name", strlen("name"));
> + mpstream_encode_str(&stream, field->name, strlen(field->name));
> + mpstream_encode_str(&stream, "type", strlen("type"));
1. This construction 'encode_str(stream, str, strlen(str))' is
extremely frequent in the overall project. Please, in the previous
commit split mpstream_encode_str into 2 functions:
mpstream_encode_str(stream, str)
mpstream_encode_strn(stream, str, len)
The first one calls mpstream_encode_strn(stream, str, strlen(str))
and is static inline in the header. With those SQL encoders we can
use mpstream_encode_str(stream, str).
> if (i == pk_forced_int) {
> t = "integer";
> } else {
> @@ -1329,108 +1275,149 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
> }
>
> assert(def->fields[i].is_nullable ==
> - action_is_nullable(def->fields[i].nullable_action));
> - p = enc->encode_str(p, t, strlen(t));
> - p = enc->encode_str(p, "affinity", 8);
> - p = enc->encode_uint(p, def->fields[i].affinity);
> - p = enc->encode_str(p, "is_nullable", 11);
> - p = enc->encode_bool(p, def->fields[i].is_nullable);
> - p = enc->encode_str(p, "nullable_action", 15);
> + action_is_nullable(def->fields[i].
> + nullable_action));
> + mpstream_encode_str(&stream, t, strlen(t));
> + mpstream_encode_str(&stream, "affinity", strlen("affinity"));
> + mpstream_encode_uint(&stream,
> + def->fields[i].affinity);
> + mpstream_encode_str(&stream, "is_nullable",
> + strlen("is_nullable"));
> + mpstream_encode_bool(&stream,
> + def->fields[i].is_nullable);
> + mpstream_encode_str(&stream, "nullable_action",
> + strlen("nullable_action"));
> assert(def->fields[i].nullable_action < on_conflict_action_MAX);
> const char *action =
> on_conflict_action_strs[def->fields[i].nullable_action];
> - p = enc->encode_str(p, action, strlen(action));
> + mpstream_encode_str(&stream, action, strlen(action));
> if (cid != COLL_NONE) {
> - p = enc->encode_str(p, "collation", strlen("collation"));
> - p = enc->encode_uint(p, cid);
> + mpstream_encode_str(&stream, "collation",
> + strlen("collation"));
> + mpstream_encode_uint(&stream, cid);
> }
> if (default_str != NULL) {
> - p = enc->encode_str(p, "default", strlen("default"));
> - p = enc->encode_str(p, default_str, strlen(default_str));
> + mpstream_encode_str(&stream, "default",
> + strlen("default"));
> + mpstream_encode_str(&stream, default_str,
> + strlen(default_str));
> }
> }
> - return (int)(p - base);
> + mpstream_flush(&stream);
> + if (is_error)
> + return NULL;
2. Neither region_alloc_cb nor reserve_cb set diagnostics, so
here you return NULL, but have an empty diag.
> + size_t sz = region_used(region) - used;
> + char *raw = region_join(region, sz);
> + if (raw == NULL)
> + diag_set(OutOfMemory, sz, "region_join", "raw");
> + else
> + *size = sz;
> + return raw;
> }
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index dddeb12..462b6e6 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1603,33 +1614,53 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
> fkey_action_strs[fk->on_delete], P4_STATIC);
> sqlite3VdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 6, 0,
> fkey_action_strs[fk->on_update], P4_STATIC);
> - size_t parent_size = fkey_encode_links(fk, FIELD_LINK_PARENT, NULL);
> - size_t child_size = fkey_encode_links(fk, FIELD_LINK_CHILD, NULL);
> + struct region *region = &parse_context->region;
> + uint32_t parent_links_size = 0;
> + char *parent_links =
> + fkey_encode_links(region, fk, FIELD_LINK_PARENT,
> + &parent_links_size);
> + if (parent_links == NULL)
> + goto error;
> + uint32_t child_links_size = 0;
> + char *child_links =
> + fkey_encode_links(region, fk, FIELD_LINK_CHILD,
> + &child_links_size);
> + if (child_links == NULL)
> + goto error;
> /*
> * We are allocating memory for both parent and child
> * arrays in the same chunk. Thus, first OP_Blob opcode
> * interprets it as static memory, and the second one -
> * as dynamic and releases memory.
> */
> - char *parent_links = sqlite3DbMallocRaw(parse_context->db,
> - parent_size + child_size);
> - if (parent_links == NULL) {
> - sqlite3DbFree(parse_context->db, (void *) name_copy);
> + char *raw = sqlite3DbMallocRaw(parse_context->db,
> + parent_links_size + child_links_size);
> + if (raw == NULL) {
> + sqlite3DbFree(parse_context->db, name_copy);
3. You shall not free name copy here. It is added to Vdbe in the
first lines of the function as P4_DYNAMIC and will be freed on
Vdbe destruction.
> return;
> }
4. I have pushed my fixes on the branch. Please, look,
squash, and fix other things.
My diff is below:
===============================================================
commit 19fb0e03bf99a74e41349af4553299899f32deeb
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Fri Aug 24 16:59:22 2018 +0300
Review fixes
diff --git a/src/box/sql.c b/src/box/sql.c
index 28ad9d37f..790c0529a 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1220,11 +1220,11 @@ static const char *convertSqliteAffinity(int affinity, bool allow_nulls)
}
}
+/** Callback to forward and error from mpstream methods. */
static void
set_encode_error(void *error_ctx)
{
- bool *is_error = error_ctx;
- *is_error = true;
+ *(bool *)error_ctx = true;
}
char *
@@ -1273,18 +1273,14 @@ sql_encode_table(struct region *region, struct Table *table, uint32_t *size)
convertSqliteAffinity(affinity,
def->fields[i].is_nullable);
}
-
assert(def->fields[i].is_nullable ==
- action_is_nullable(def->fields[i].
- nullable_action));
+ action_is_nullable(def->fields[i].nullable_action));
mpstream_encode_str(&stream, t, strlen(t));
mpstream_encode_str(&stream, "affinity", strlen("affinity"));
- mpstream_encode_uint(&stream,
- def->fields[i].affinity);
+ mpstream_encode_uint(&stream, def->fields[i].affinity);
mpstream_encode_str(&stream, "is_nullable",
strlen("is_nullable"));
- mpstream_encode_bool(&stream,
- def->fields[i].is_nullable);
+ mpstream_encode_bool(&stream, def->fields[i].is_nullable);
mpstream_encode_str(&stream, "nullable_action",
strlen("nullable_action"));
assert(def->fields[i].nullable_action < on_conflict_action_MAX);
@@ -1306,12 +1302,10 @@ sql_encode_table(struct region *region, struct Table *table, uint32_t *size)
mpstream_flush(&stream);
if (is_error)
return NULL;
- size_t sz = region_used(region) - used;
- char *raw = region_join(region, sz);
+ *size = region_used(region) - used;
+ char *raw = region_join(region, *size);
if (raw == NULL)
- diag_set(OutOfMemory, sz, "region_join", "raw");
- else
- *size = sz;
+ diag_set(OutOfMemory, *size, "region_join", "raw");
return raw;
}
@@ -1326,12 +1320,16 @@ sql_encode_table_opts(struct region *region, struct Table *table,
mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
set_encode_error, &is_error);
bool is_view = false;
- bool has_checks = false;
+ int checks_cnt = 0;
+ struct ExprList_item *a;
if (table != NULL) {
is_view = table->def->opts.is_view;
- has_checks = table->def->opts.checks != NULL;
+ struct ExprList *checks = table->def->opts.checks;
+ if (checks != NULL) {
+ checks_cnt = checks->nExpr;
+ a = checks->a;
+ }
}
- uint32_t checks_cnt = has_checks ? table->def->opts.checks->nExpr : 0;
mpstream_encode_map(&stream, 1 + is_view + (checks_cnt > 0));
mpstream_encode_str(&stream, "sql", strlen("sql"));
@@ -1340,39 +1338,32 @@ sql_encode_table_opts(struct region *region, struct Table *table,
mpstream_encode_str(&stream, "view", strlen("view"));
mpstream_encode_bool(&stream, true);
}
- if (checks_cnt == 0)
- goto finish;
-
- /* Encode checks. */
- struct ExprList_item *a = table->def->opts.checks->a;
- mpstream_encode_str(&stream, "checks", strlen("checks"));
- mpstream_encode_array(&stream, checks_cnt);
- for (uint32_t i = 0; i < checks_cnt && !is_error; ++i) {
- int items = (a[i].pExpr != NULL) + (a[i].zName != NULL);
+ if (checks_cnt > 0) {
+ mpstream_encode_str(&stream, "checks", strlen("checks"));
+ mpstream_encode_array(&stream, checks_cnt);
+ }
+ for (int i = 0; i < checks_cnt && !is_error; ++i, ++a) {
+ int items = (a->pExpr != NULL) + (a->zName != NULL);
mpstream_encode_map(&stream, items);
- assert(a[i].pExpr != NULL);
- struct Expr *pExpr = a[i].pExpr;
+ assert(a->pExpr != NULL);
+ struct Expr *pExpr = a->pExpr;
assert(pExpr->u.zToken != NULL);
mpstream_encode_str(&stream, "expr", strlen("expr"));
mpstream_encode_str(&stream, pExpr->u.zToken,
strlen(pExpr->u.zToken));
- if (a[i].zName != NULL) {
+ if (a->zName != NULL) {
mpstream_encode_str(&stream, "name", strlen("name"));
- mpstream_encode_str(&stream, a[i].zName,
- strlen(a[i].zName));
+ mpstream_encode_str(&stream, a->zName,
+ strlen(a->zName));
}
}
-
-finish:
mpstream_flush(&stream);
if (is_error)
return NULL;
- size_t sz = region_used(region) - used;
- char *raw = region_join(region, sz);
+ *size = region_used(region) - used;
+ char *raw = region_join(region, *size);
if (raw == NULL)
- diag_set(OutOfMemory, sz, "region_join", "raw");
- else
- *size = sz;
+ diag_set(OutOfMemory, *size, "region_join", "raw");
return raw;
}
@@ -1392,18 +1383,16 @@ fkey_encode_links(struct region *region, const struct fkey_def *def, int type,
mpstream_flush(&stream);
if (is_error)
return NULL;
- size_t sz = region_used(region) - used;
- char *raw = region_join(region, sz);
+ *size = region_used(region) - used;
+ char *raw = region_join(region, *size);
if (raw == NULL)
- diag_set(OutOfMemory, sz, "region_join", "raw");
- else
- *size = sz;
+ diag_set(OutOfMemory, *size, "region_join", "raw");
return raw;
}
char *
sql_encode_index_parts(struct region *region, struct SqliteIndex *index,
- uint32_t *size)
+ uint32_t *size)
{
size_t used = region_used(region);
struct mpstream stream;
@@ -1415,13 +1404,12 @@ sql_encode_index_parts(struct region *region, struct SqliteIndex *index,
* treat it as strict type, not affinity.
*/
uint32_t pk_forced_int = UINT32_MAX;
- struct SqliteIndex *primary_index =
- sqlite3PrimaryKeyIndex(index->pTable);
+ struct SqliteIndex *pk = sqlite3PrimaryKeyIndex(index->pTable);
struct field_def *fields = index->pTable->def->fields;
- if (primary_index->def->key_def->part_count == 1) {
- int pk = primary_index->def->key_def->parts[0].fieldno;
- if (fields[pk].type == FIELD_TYPE_INTEGER)
- pk_forced_int = pk;
+ if (pk->def->key_def->part_count == 1) {
+ int fieldno = pk->def->key_def->parts[0].fieldno;
+ if (fields[fieldno].type == FIELD_TYPE_INTEGER)
+ pk_forced_int = fieldno;
}
/* gh-2187
@@ -1431,10 +1419,9 @@ sql_encode_index_parts(struct region *region, struct SqliteIndex *index,
* data layout.
*/
struct key_def *key_def = index->def->key_def;
- uint32_t part_count = key_def->part_count;
struct key_part *part = key_def->parts;
- mpstream_encode_array(&stream, part_count);
- for (uint32_t i = 0; i < part_count; ++i, ++part) {
+ mpstream_encode_array(&stream, key_def->part_count);
+ for (uint32_t i = 0; i < key_def->part_count; ++i, ++part) {
uint32_t col = part->fieldno;
assert(fields[col].is_nullable ==
action_is_nullable(fields[col].nullable_action));
@@ -1459,8 +1446,7 @@ sql_encode_index_parts(struct region *region, struct SqliteIndex *index,
}
mpstream_encode_str(&stream, "is_nullable",
strlen("is_nullable"));
- mpstream_encode_bool(&stream,
- fields[col].is_nullable);
+ mpstream_encode_bool(&stream, fields[col].is_nullable);
mpstream_encode_str(&stream, "nullable_action",
strlen("nullable_action"));
const char *action_str =
@@ -1478,12 +1464,10 @@ sql_encode_index_parts(struct region *region, struct SqliteIndex *index,
mpstream_flush(&stream);
if (is_error)
return NULL;
- size_t sz = region_used(region) - used;
- char *raw = region_join(region, sz);
+ *size = region_used(region) - used;
+ char *raw = region_join(region, *size);
if (raw == NULL)
- diag_set(OutOfMemory, sz, "region_join", "raw");
- else
- *size = sz;
+ diag_set(OutOfMemory, *size, "region_join", "raw");
return raw;
}
@@ -1514,12 +1498,10 @@ sql_encode_index_opts(struct region *region, struct SqliteIndex *index,
mpstream_flush(&stream);
if (is_error)
return NULL;
- size_t sz = region_used(region) - used;
- char *raw = region_join(region, sz);
+ *size = region_used(region) - used;
+ char *raw = region_join(region, *size);
if (raw == NULL)
- diag_set(OutOfMemory, sz, "region_join", "raw");
- else
- *size = sz;
+ diag_set(OutOfMemory, *size, "region_join", "raw");
return raw;
}
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 462b6e67b..ab1438db4 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1371,9 +1371,8 @@ createSpace(Parse * pParse, int iSpaceId, char *zStmt)
int iRecord = (pParse->nMem += 7);
struct region *region = &pParse->region;
uint32_t table_opts_stmt_sz = 0;
- char *table_opts_stmt =
- sql_encode_table_opts(region, table, zStmt,
- &table_opts_stmt_sz);
+ char *table_opts_stmt = sql_encode_table_opts(region, table, zStmt,
+ &table_opts_stmt_sz);
if (table_opts_stmt == NULL)
goto error;
uint32_t table_stmt_sz = 0;
@@ -1602,10 +1601,8 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
BOX_FK_CONSTRAINT_ID, 0,
constr_tuple_reg, 2,
ER_CONSTRAINT_EXISTS, error_msg,
- false, OP_NoConflict) != 0) {
- sqlite3DbFree(parse_context->db, name_copy);
+ false, OP_NoConflict) != 0)
return;
- }
sqlite3VdbeAddOp2(vdbe, OP_Bool, 0, constr_tuple_reg + 3);
sqlite3VdbeChangeP4(vdbe, -1, (char*)&fk->is_deferred, P4_BOOL);
sqlite3VdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 4, 0,
@@ -1616,15 +1613,13 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
fkey_action_strs[fk->on_update], P4_STATIC);
struct region *region = &parse_context->region;
uint32_t parent_links_size = 0;
- char *parent_links =
- fkey_encode_links(region, fk, FIELD_LINK_PARENT,
- &parent_links_size);
+ char *parent_links = fkey_encode_links(region, fk, FIELD_LINK_PARENT,
+ &parent_links_size);
if (parent_links == NULL)
goto error;
uint32_t child_links_size = 0;
- char *child_links =
- fkey_encode_links(region, fk, FIELD_LINK_CHILD,
- &child_links_size);
+ char *child_links = fkey_encode_links(region, fk, FIELD_LINK_CHILD,
+ &child_links_size);
if (child_links == NULL)
goto error;
/*
@@ -1635,10 +1630,8 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
*/
char *raw = sqlite3DbMallocRaw(parse_context->db,
parent_links_size + child_links_size);
- if (raw == NULL) {
- sqlite3DbFree(parse_context->db, name_copy);
+ if (raw == NULL)
return;
- }
memcpy(raw, parent_links, parent_links_size);
parent_links = raw;
raw += parent_links_size;
@@ -1660,7 +1653,6 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
error:
parse_context->nErr++;
parse_context->rc = SQL_TARANTOOL_ERROR;
- sqlite3DbFree(parse_context->db, name_copy);
}
/**
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index 6a42ed3c6..78790a2bb 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -139,25 +139,25 @@ tarantoolSqlite3IncrementMaxid(uint64_t *space_max_id);
/**
* Encode format as entry to be inserted to _space on @region.
+ * @param region Region to use.
+ * @param table Table to encode.
+ * @param[out] size Size of result allocation.
*
- * @param region region to use.
- * @param table table to encode.
- * @param[out] size size of result allocation.
- * @retval NULL on error.
- * @retval not NULL pointer to msgpack on success.
+ * @retval NULL Error.
+ * @retval not NULL Pointer to msgpack on success.
*/
char *
sql_encode_table(struct region *region, struct Table *table, uint32_t *size);
/**
* Encode "opts" dictionary for _space entry on @region.
+ * @param region Region to use.
+ * @param table Table containing opts to encode.
+ * @param sql Source request to encode.
+ * @param[out] size Size of result allocation.
*
- * @param region region to use.
- * @param table table containing opts to encode.
- * @param sql source request to encode.
- * @param[out] size size of result allocation.
- * @retval NULL on error.
- * @retval not NULL pointer to msgpack on success
+ * @retval NULL Error.
+ * @retval not NULL Pointer to msgpack on success.
*/
char *
sql_encode_table_opts(struct region *region, struct Table *table,
@@ -166,13 +166,13 @@ sql_encode_table_opts(struct region *region, struct Table *table,
/**
* Encode links of given foreign key constraint into MsgPack on
* @region.
- *
- * @param region region to use.
+ * @param region Wegion to use.
* @param def FK def to encode links of.
* @param type Links type to encode.
- * @param[out] size size of result allocation.
- * @retval NULL on error.
- * @retval not NULL pointer to msgpack on success
+ * @param[out] Size size of result allocation.
+ *
+ * @retval NULL Error.
+ * @retval not NULL Pointer to msgpack on success.
*/
char *
fkey_encode_links(struct region *region, const struct fkey_def *def, int type,
@@ -181,12 +181,12 @@ fkey_encode_links(struct region *region, const struct fkey_def *def, int type,
/**
* Encode index parts of given foreign key constraint into
* MsgPack on @region.
+ * @param region Region to use.
+ * @param index Index to encode.
+ * @param[out] size Size of result allocation.
*
- * @param region region to use.
- * @param index index to encode.
- * @param[out] size size of result allocation.
- * @retval NULL on error.
- * @retval not NULL pointer to msgpack on success
+ * @retval NULL Error.
+ * @retval not NULL Pointer to msgpack on success
*/
char *
sql_encode_index_parts(struct region *region, struct Index *index,
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 6a0708107..f400c2551 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -173,16 +173,12 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
{
/* Trigger being finished. */
struct sql_trigger *trigger = parse->parsed_ast.trigger;
- /* SQL text. */
- char *sql_str = NULL;
- /* MsgPack containing SQL options. */
- char *opts_buff = NULL;
/* The database. */
struct sqlite3 *db = parse->db;
parse->parsed_ast.trigger = NULL;
if (NEVER(parse->nErr) || trigger == NULL)
- goto triggerfinish_cleanup;
+ goto cleanup;
char *trigger_name = trigger->zName;
trigger->step_list = step_list;
while (step_list != NULL) {
@@ -202,17 +198,18 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
/* Make an entry in the _trigger space. */
struct Vdbe *v = sqlite3GetVdbe(parse);
if (v == 0)
- goto triggerfinish_cleanup;
+ goto cleanup;
struct Table *sys_trigger =
- sqlite3HashFind(&parse->db->pSchema->tblHash,
+ sqlite3HashFind(&db->pSchema->tblHash,
TARANTOOL_SYS_TRIGGER_NAME);
if (NEVER(sys_trigger == NULL))
- goto triggerfinish_cleanup;
+ goto cleanup;
- sql_str = sqlite3MPrintf(db, "CREATE TRIGGER %s", token->z);
- if (db->mallocFailed)
- goto triggerfinish_cleanup;
+ char *sql_str =
+ sqlite3MPrintf(db, "CREATE TRIGGER %s", token->z);
+ if (sql_str == NULL)
+ goto cleanup;
int cursor = parse->nTab++;
sqlite3OpenTable(parse, cursor, sys_trigger, OP_OpenWrite);
@@ -228,23 +225,24 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
uint32_t opts_buff_sz = 0;
char *data = sql_encode_table_opts(&fiber()->gc, NULL, sql_str,
&opts_buff_sz);
- if (data != NULL) {
- opts_buff = sqlite3DbMallocRaw(parse->db, opts_buff_sz);
- if (opts_buff != NULL)
- memcpy(opts_buff, data, opts_buff_sz);
- } else {
+ sqlite3DbFree(db, sql_str);
+ if (data == NULL) {
parse->nErr++;
parse->rc = SQL_TARANTOOL_ERROR;
+ goto cleanup;
}
+ char *opts_buff = sqlite3DbMallocRaw(db, opts_buff_sz);
if (opts_buff == NULL)
- goto triggerfinish_cleanup;
+ goto cleanup;
+ memcpy(opts_buff, data, opts_buff_sz);
- trigger_name = sqlite3DbStrDup(parse->db, trigger_name);
- if (trigger_name == NULL)
- goto triggerfinish_cleanup;
+ trigger_name = sqlite3DbStrDup(db, trigger_name);
+ if (trigger_name == NULL) {
+ sqlite3DbFree(db, opts_buff);
+ goto cleanup;
+ }
- sqlite3VdbeAddOp4(v,
- OP_String8, 0, first_col, 0,
+ sqlite3VdbeAddOp4(v, OP_String8, 0, first_col, 0,
trigger_name, P4_DYNAMIC);
sqlite3VdbeAddOp2(v, OP_Integer, trigger->space_id,
first_col + 1);
@@ -263,11 +261,7 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
trigger = NULL;
}
- triggerfinish_cleanup:
- if (db->mallocFailed) {
- sqlite3DbFree(db, sql_str);
- sqlite3DbFree(db, opts_buff);
- }
+cleanup:
sql_trigger_delete(db, trigger);
assert(parse->parsed_ast.trigger == NULL || parse->parse_only);
sqlite3DeleteTriggerStep(db, step_list);
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-08-24 16:33 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-17 12:24 [tarantool-patches] [PATCH v1 1/1] sql: remove struct Enc Kirill Shcherbatov
2018-08-17 12:40 ` [tarantool-patches] " Vladislav Shpilevoy
2018-08-17 14:04 ` Kirill Shcherbatov
[not found] ` <cover.1534514520.git.kshcherbatov@tarantool.org>
2018-08-17 14:04 ` [tarantool-patches] [PATCH v1 1/2] box: export mpstream methods to core Kirill Shcherbatov
2018-08-24 16:33 ` [tarantool-patches] " Vladislav Shpilevoy
2018-08-21 12:09 ` [tarantool-patches] Re: [PATCH v1 1/1] sql: remove struct Enc n.pettik
2018-08-21 15:50 ` Kirill Shcherbatov
2018-08-24 16:33 ` Vladislav Shpilevoy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox