Tarantool development patches archive
 help / color / mirror / Atom feed
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
To: "n.pettik" <korablev@tarantool.org>, tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: remove struct Enc
Date: Tue, 21 Aug 2018 18:50:27 +0300	[thread overview]
Message-ID: <6bdaa379-f063-0364-49ca-a5ad573c6975@tarantool.org> (raw)
In-Reply-To: <59B7B0ED-9BE2-402D-9953-0347E576BE00@tarantool.org>

> 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,

  reply	other threads:[~2018-08-21 15:50 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-17 12:24 [tarantool-patches] " 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 [this message]
2018-08-24 16:33     ` Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6bdaa379-f063-0364-49ca-a5ad573c6975@tarantool.org \
    --to=kshcherbatov@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v1 1/1] sql: remove struct Enc' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox