[Tarantool-patches] [PATCH v1 2/3] sql: refactor sql_vdbe_mem_encode_tuple()
Mergen Imeev
imeevma at tarantool.org
Wed Nov 24 10:24:52 MSK 2021
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 at 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);
}
/*
More information about the Tarantool-patches
mailing list