[tarantool-patches] Re: [PATCH v1 1/1] sql: remove struct Enc
n.pettik
korablev at tarantool.org
Tue Aug 21 15:09:17 MSK 2018
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);
>
More information about the Tarantool-patches
mailing list