Re: [Tarantool-patches] [PATCH v4 15/53] sql: rework vdbe_decode_msgpack_into_mem()

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.

> 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.

>  {
>  	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.

> +	}
> +	return 0;
> +}