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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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