[Tarantool-patches] [PATCH v5 16/52] sql: rework vdbe_decode_msgpack_into_mem()
imeevma at tarantool.org
imeevma at tarantool.org
Fri Apr 9 20:57:14 MSK 2021
Thank you for the review! My answers and new patch below.
On 30.03.2021 02:02, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 3 comments below.
>
> On 23.03.2021 10:35, Mergen Imeev via Tarantool-patches wrote:
>> The original vdbe_decode_msgpack_into_mem() returns a MEM that contains
>> string and binary values as ephemeral. This patch renames this function
>> to vdbe_decode_msgpack_into_mem_ephemeral() and introduces new
>> vdbe_decode_msgpack_into_mem(), which returns a MEM that contains string
>> and binary values in newly allocated memory.
>>
>> This patch actually changes behavior in this case:
>
> 1. Changes how? I don't see any changes in the tests.
>
I make this change because it doesn't affect test.
Example of changed behaviour:
CREATE TABLE t1(m VARBINARY primary key);
INSERT INTO t1 VALUES(x'6178'), (x'6278'), (x'6379');
SELECT count(*), substr(m,2,1) AS mx FROM t1 GROUP BY mx;
Before this patch:
tarantool> SELECT count(*), substr(m,2,1) AS mx FROM t1 GROUP BY mx;
---
- metadata:
- name: COLUMN_1
type: integer
- name: MX
type: string
rows:
- [2, 'y']
- [1, 'y']
...
After this patch.
tarantool> SELECT count(*), substr(m,2,1) AS mx FROM t1 GROUP BY mx;
---
- metadata:
- name: COLUMN_1
type: integer
- name: MX
type: string
rows:
- [2, 'x']
- [1, 'y']
...
A bit more is written in issue #5890.
>> CREATE TABLE t1(m VARBINARY primary key);
>> INSERT INTO t1 VALUES(x'6178'), (x'6278'), (x'6379');
>> SELECT count(*), substr(m,2,1) AS m FROM t1 GROUP BY m;
>> SELECT count(*), substr(m,2,1) AS mx FROM t1 GROUP BY mx;
>>
>> But it doesn't change behaviour for this:
>>
>> CREATE TABLE t2(m STRING primary key);
>> INSERT INTO t2 VALUES('ax'), ('bx'), ('cy');
>> SELECT count(*), substr(m,2,1) AS m FROM t2 GROUP BY m;
>> SELECT count(*), substr(m,2,1) AS mx FROM t2 GROUP BY mx;
>>
>> Part of #5818
>> Part of #5890
>> ---
>> src/box/sql/mem.c | 16 +++++++++++++++-
>> src/box/sql/mem.h | 17 ++++++++++++++++-
>> src/box/sql/vdbe.c | 18 ------------------
>> src/box/sql/vdbeaux.c | 2 +-
>> 4 files changed, 32 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
>> index 3d42ac63c..a2316cc90 100644
>> --- a/src/box/sql/mem.c
>> +++ b/src/box/sql/mem.c
>> @@ -2253,7 +2253,8 @@ sqlVdbeRecordCompareMsgpack(const void *key1,
>> }
>>
>> int
>> -vdbe_decode_msgpack_into_mem(const char *buf, struct Mem *mem, uint32_t *len)
>> +vdbe_decode_msgpack_into_ephemeral_mem(const char *buf, struct Mem *mem,
>> + uint32_t *len)
>
> 2. The function name is getting Java vibes. I propose to rename it to
> mem_from_mp_ephemeral() and mem_from_mp() correspondingly. They also should
> start taking the mem as a first argument.
>
Thank you! Fixed.
>> {
>> const char *start_buf = buf;
>> switch (mp_typeof(*buf)) {
>> @@ -2354,6 +2355,19 @@ install_blob:
>> return 0;
>> }
>>
>> +int
>> +vdbe_decode_msgpack_into_mem(const char *buf, struct Mem *mem, uint32_t *len)
>> +{
>> + if (vdbe_decode_msgpack_into_ephemeral_mem(buf, mem, len) != 0)
>> + return -1;
>> + if ((mem->flags & (MEM_Str | MEM_Blob)) != 0) {
>> + assert((mem->flags & MEM_Ephem) != 0);
>> + if (sqlVdbeMemGrow(mem, mem->n, 1) != 0)
>> + return -1;
>
> 3. Maybe it is worth adding a function like mem_materialize() or
> mem_make_writable() for that kind of work.
>
Not sure. For now decided to not add a new function.
>> + }
>> + return 0;
>> +}
New patch:
commit 8c81c3e86a628e5777145b30f69392dd5a0fd873
Author: Mergen Imeev <imeevma at gmail.com>
Date: Sat Mar 13 15:43:38 2021 +0300
sql: rework vdbe_decode_msgpack_into_mem()
The original vdbe_decode_msgpack_into_mem() returns a MEM that contains
string and binary values as ephemeral. This patch renames this function
to mem_from_mp_ephemeral() and introduces new function mem_from_mp(),
which returns a MEM that contains string and binary values in newly
allocated memory.
This patch changes behavior for this query:
CREATE TABLE t1(m VARBINARY primary key);
INSERT INTO t1 VALUES(x'6178'), (x'6278'), (x'6379');
SELECT count(*), substr(m,2,1) AS mx FROM t1 GROUP BY mx;
Before this patch:
tarantool> SELECT count(*), substr(m,2,1) AS mx FROM t1 GROUP BY mx;
---
- metadata:
- name: COLUMN_1
type: integer
- name: MX
type: string
rows:
- [2, 'y']
- [1, 'y']
...
After this patch.
tarantool> SELECT count(*), substr(m,2,1) AS mx FROM t1 GROUP BY mx;
---
- metadata:
- name: COLUMN_1
type: integer
- name: MX
type: string
rows:
- [2, 'x']
- [1, 'y']
...
Part of #5818
Closes #5890
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index d56fe56c6..7d06e256c 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -2214,7 +2214,7 @@ sqlVdbeRecordCompareMsgpack(const void *key1,
}
int
-vdbe_decode_msgpack_into_mem(const char *buf, struct Mem *mem, uint32_t *len)
+mem_from_mp_ephemeral(struct Mem *mem, const char *buf, uint32_t *len)
{
const char *start_buf = buf;
switch (mp_typeof(*buf)) {
@@ -2315,6 +2315,19 @@ install_blob:
return 0;
}
+int
+mem_from_mp(struct Mem *mem, const char *buf, uint32_t *len)
+{
+ if (mem_from_mp_ephemeral(mem, buf, len) != 0)
+ return -1;
+ if ((mem->flags & (MEM_Str | MEM_Blob)) != 0) {
+ assert((mem->flags & MEM_Ephem) != 0);
+ if (sqlVdbeMemGrow(mem, mem->n, 1) != 0)
+ return -1;
+ }
+ return 0;
+}
+
void
mpstream_encode_vdbe_mem(struct mpstream *stream, struct Mem *var)
{
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 394055db9..55f8f0c9f 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -520,16 +520,30 @@ int sqlVdbeRecordCompareMsgpack(const void *key1,
struct UnpackedRecord *key2);
/**
- * Decode msgpack and save value into VDBE memory cell.
+ * Decode msgpack and save value into VDBE memory cell. String and binary string
+ * values set as ephemeral.
*
+ * @param mem Memory cell to write value into.
* @param buf Buffer to deserialize msgpack from.
+ * @param len[out] Length of decoded part.
+ * @retval Return code: < 0 in case of error.
+ * @retval 0 on success.
+ */
+int
+mem_from_mp_ephemeral(struct Mem *mem, const char *buf, uint32_t *len);
+
+/**
+ * Decode msgpack and save value into VDBE memory cell. String and binary string
+ * values copied to newly allocated memory.
+ *
* @param mem Memory cell to write value into.
+ * @param buf Buffer to deserialize msgpack from.
* @param len[out] Length of decoded part.
* @retval Return code: < 0 in case of error.
* @retval 0 on success.
*/
int
-vdbe_decode_msgpack_into_mem(const char *buf, struct Mem *mem, uint32_t *len);
+mem_from_mp(struct Mem *mem, const char *buf, uint32_t *len);
/**
* Perform encoding memory variable to stream.
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 0c19acff5..378c7a043 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -405,26 +405,8 @@ vdbe_field_ref_fetch(struct vdbe_field_ref *field_ref, uint32_t fieldno,
assert(sqlVdbeCheckMemInvariants(dest_mem) != 0);
const char *data = vdbe_field_ref_fetch_data(field_ref, fieldno);
uint32_t dummy;
- if (vdbe_decode_msgpack_into_mem(data, dest_mem, &dummy) != 0)
+ if (mem_from_mp(dest_mem, data, &dummy) != 0)
return -1;
-
- /*
- * Add 0 termination (at most for strings)
- * Not sure why do we check MEM_Ephem
- */
- if (mem_is_str(dest_mem) && mem_is_ephemeral(dest_mem)) {
- int len = dest_mem->n;
- if (dest_mem->szMalloc < len + 1) {
- if (sqlVdbeMemGrow(dest_mem, len + 1, 1) != 0)
- return -1;
- } else {
- dest_mem->z =
- memcpy(dest_mem->zMalloc, dest_mem->z, len);
- dest_mem->flags &= ~MEM_Ephem;
- }
- dest_mem->z[len] = 0;
- dest_mem->flags |= MEM_Term;
- }
UPDATE_MAX_BLOBSIZE(dest_mem);
return 0;
}
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index bec8a532a..dff108412 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2358,7 +2358,7 @@ sqlVdbeRecordUnpackMsgpack(struct key_def *key_def, /* Information about the rec
pMem->szMalloc = 0;
pMem->z = 0;
uint32_t sz = 0;
- vdbe_decode_msgpack_into_mem(zParse, pMem, &sz);
+ mem_from_mp_ephemeral(pMem, zParse, &sz);
assert(sz != 0);
zParse += sz;
pMem++;
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 91cba9962..ba5c08a00 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -563,7 +563,7 @@ sql_stat4_column(struct sql *db, const char *record, uint32_t col_num,
}
}
uint32_t unused;
- return vdbe_decode_msgpack_into_mem(a, mem, &unused);
+ return mem_from_mp(mem, a, &unused);
}
/*
More information about the Tarantool-patches
mailing list