[tarantool-patches] Re: [PATCH v1 1/1] sql: remove struct Enc

["n.pettik" <korablev@tarantool.org>]

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