Re: [Tarantool-patches] [PATCH v1 2/3] sql: refactor sql_vdbe_mem_encode_tuple()

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thank you for the review! My answer, diff and new patch below. Also, I
fixed one comment about region_truncate() from the letters about MAP.

On Sat, Nov 20, 2021 at 12:35:46AM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index 356b2c7be..2ba4135b0 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -3013,31 +3013,31 @@ mem_to_mpstream(const struct Mem *var, struct mpstream *stream)
> >  }
> >  
> >  char *
> > -sql_vdbe_mem_encode_tuple(struct Mem *fields, uint32_t field_count,
> > -			  uint32_t *tuple_size, struct region *region)
> > +mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
> > +		 struct region *region)
> >  {
> >  	size_t used = region_used(region);
> >  	bool is_error = false;
> >  	struct mpstream stream;
> >  	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
> >  		      set_encode_error, &is_error);
> > -	mpstream_encode_array(&stream, field_count);
> > -	for (struct Mem *field = fields; field < fields + field_count; field++)
> > -		mem_to_mpstream(field, &stream);
> > +	mpstream_encode_array(&stream, count);
> > +	for (const struct Mem *mem = mems; mem < mems + count; mem++)
> > +		mem_to_mpstream(mem, &stream);
> >  	mpstream_flush(&stream);
> >  	if (is_error) {
> 
> If some memory was allocated/reserved before mpstream_flush() failed (inside
> of mem_to_mpstream()), then the region might leak. I would recommend to add a
> truncate here.
> 
Thank you. Fixed.

> >  		diag_set(OutOfMemory, stream.pos - stream.buf,
> >  			 "mpstream_flush", "stream");
> >  		return NULL;
> >  	}

Diff:

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 71a10c9b3..32b8825bc 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -3054,6 +3054,7 @@ mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
 		mem_to_mpstream(mem, &stream);
 	mpstream_flush(&stream);
 	if (is_error) {
+		region_truncate(region, used);
 		diag_set(OutOfMemory, stream.pos - stream.buf,
 			 "mpstream_flush", "stream");
 		return NULL;
@@ -3061,6 +3062,7 @@ mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
 	*size = region_used(region) - used;
 	char *array = region_join(region, *size);
 	if (array == NULL) {
+		region_truncate(region, used);
 		diag_set(OutOfMemory, *size, "region_join", "array");
 		return NULL;
 	}



New patch:

commit 14ddc911967143075a373a24d318b36120144267
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Tue Nov 16 09:54:09 2021 +0300

    sql: refactor sql_vdbe_mem_encode_tuple()
    
    This function is used to create ARRAY value from array of MEMs. Since
    ARRAY was added to SQL, this function needs to be refactored.
    
    Part of #4762

diff --git a/src/box/sql.c b/src/box/sql.c
index d15159d6e..2a78a96d5 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -211,7 +211,7 @@ sql_cursor_seek(struct BtCursor *cur, struct Mem *mems, uint32_t len, int *res)
 	struct region *region = &fiber()->gc;
 	size_t used = region_used(region);
 	uint32_t size;
-	const char *tuple = sql_vdbe_mem_encode_tuple(mems, len, &size, region);
+	const char *tuple = mem_encode_array(mems, len, &size, region);
 	if (tuple == NULL)
 		return -1;
 	if (key_alloc(cur, size) != 0)
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 303c3a1a3..32b8825bc 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -3041,31 +3041,33 @@ mem_to_mpstream(const struct Mem *var, struct mpstream *stream)
 }
 
 char *
-sql_vdbe_mem_encode_tuple(struct Mem *fields, uint32_t field_count,
-			  uint32_t *tuple_size, struct region *region)
+mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
+		 struct region *region)
 {
 	size_t used = region_used(region);
 	bool is_error = false;
 	struct mpstream stream;
 	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
 		      set_encode_error, &is_error);
-	mpstream_encode_array(&stream, field_count);
-	for (struct Mem *field = fields; field < fields + field_count; field++)
-		mem_to_mpstream(field, &stream);
+	mpstream_encode_array(&stream, count);
+	for (const struct Mem *mem = mems; mem < mems + count; mem++)
+		mem_to_mpstream(mem, &stream);
 	mpstream_flush(&stream);
 	if (is_error) {
+		region_truncate(region, used);
 		diag_set(OutOfMemory, stream.pos - stream.buf,
 			 "mpstream_flush", "stream");
 		return NULL;
 	}
-	*tuple_size = region_used(region) - used;
-	char *tuple = region_join(region, *tuple_size);
-	if (tuple == NULL) {
-		diag_set(OutOfMemory, *tuple_size, "region_join", "tuple");
+	*size = region_used(region) - used;
+	char *array = region_join(region, *size);
+	if (array == NULL) {
+		region_truncate(region, used);
+		diag_set(OutOfMemory, *size, "region_join", "array");
 		return NULL;
 	}
-	mp_tuple_assert(tuple, tuple + *tuple_size);
-	return tuple;
+	mp_tuple_assert(array, array + *size);
+	return array;
 }
 
 /**
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index ea9ef709d..7f5ecf954 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -862,15 +862,15 @@ void
 mem_to_mpstream(const struct Mem *var, struct mpstream *stream);
 
 /**
- * Perform encoding field_count Vdbe memory fields on region as
- * msgpack array.
- * @param fields The first Vdbe memory field to encode.
- * @param field_count Count of fields to encode.
- * @param[out] tuple_size Size of encoded tuple.
+ * Encode array of MEMs as msgpack array on region.
+ *
+ * @param mems array of MEMs to encode.
+ * @param count number of elements in the array.
+ * @param[out] size Size of encoded msgpack array.
  * @param region Region to use.
  * @retval NULL on error, diag message is set.
- * @retval Pointer to valid tuple on success.
+ * @retval Pointer to valid msgpack array on success.
  */
 char *
-sql_vdbe_mem_encode_tuple(struct Mem *fields, uint32_t field_count,
-			  uint32_t *tuple_size, struct region *region);
+mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
+		 struct region *region);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 3892cc102..2e6893f1a 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2036,8 +2036,7 @@ case OP_MakeRecord: {
 	struct region *region = &fiber()->gc;
 	size_t used = region_used(region);
 	uint32_t tuple_size;
-	char *tuple =
-		sql_vdbe_mem_encode_tuple(pData0, nField, &tuple_size, region);
+	char *tuple = mem_encode_array(pData0, nField, &tuple_size, region);
 	if (tuple == NULL)
 		goto abort_due_to_error;
 	if ((int64_t)tuple_size > db->aLimit[SQL_LIMIT_LENGTH])
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 3894bb943..4ce5feeae 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -212,8 +212,8 @@ sql_stmt_result_to_msgpack(struct sql_stmt *stmt, uint32_t *tuple_size,
 			   struct region *region)
 {
 	struct Vdbe *vdbe = (struct Vdbe *)stmt;
-	return sql_vdbe_mem_encode_tuple(vdbe->pResultSet, vdbe->nResColumn,
-					 tuple_size, region);
+	return mem_encode_array(vdbe->pResultSet, vdbe->nResColumn, tuple_size,
+				region);
 }
 
 /*