Re: [Tarantool-patches] [PATCH v4 13/53] sql: introduce mem_copy_as_ephemeral()

[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:
> This patch intoduces mem_copy_as_ephemeral(). This function copies value
> from source MEM to destination MEM. In case value is of string or binary
> type and its allocation type is not static, it copied as value with
> ephemeral allocation type.
> 
> Part of #5818
> ---
>  src/box/sql/mem.c  | 58 ++++++++++++++++++++--------------------------
>  src/box/sql/mem.h  | 15 ++++++------
>  src/box/sql/vdbe.c | 10 ++++----
>  3 files changed, 38 insertions(+), 45 deletions(-)
> 
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index f12441d7c..30c568970 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -289,6 +289,25 @@ mem_copy(struct Mem *to, const struct Mem *from)
>  	return 0;
>  }
>  
> +int
> +mem_copy_as_ephemeral(struct Mem *to, const struct Mem *from)

1. Why didn't you keep its return type void?

> +{
> +	mem_clear(to);
> +	to->u = from->u;
> +	to->flags = from->flags;
> +	to->subtype = from->subtype;
> +	to->field_type = from->field_type;
> +	to->n = from->n;
> +	to->z = from->z;
> +	if ((to->flags & (MEM_Str | MEM_Blob)) == 0)
> +		return 0;
> +	if ((to->flags & (MEM_Static | MEM_Ephem)) != 0)
> +		return 0;
> +	to->flags &= (MEM_Str | MEM_Blob | MEM_Term | MEM_Zero | MEM_Subtype);
> +	to->flags |= MEM_Ephem;
> +	return 0;
> +}
> @@ -593,9 +598,12 @@ sqlVdbeMemAboutToChange(Vdbe * pVdbe, Mem * pMem)
>  	int i;
>  	Mem *pX;
>  	for (i = 0, pX = pVdbe->aMem; i < pVdbe->nMem; i++, pX++) {
> -		if (pX->pScopyFrom == pMem) {
> -			pX->flags |= MEM_Undefined;
> -			pX->pScopyFrom = 0;
> +		if ((pX->flags & (MEM_Blob | MEM_Str)) != 0 &&
> +		    (pX->flags & (MEM_Ephem | MEM_Static)) == 0) {
> +			if (pX->pScopyFrom == pMem) {
> +				pX->flags |= MEM_Undefined;
> +				pX->pScopyFrom = 0;
> +			}

2. Why did you change that?

>  		}
>  	}
>  	pMem->pScopyFrom = 0;
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 05e0f78c1..55083fb23 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -979,7 +979,7 @@ case OP_Copy: {
>  	pOut = &aMem[pOp->p2];
>  	assert(pOut!=pIn1);
>  	while( 1) {
> -		sqlVdbeMemShallowCopy(pOut, pIn1, MEM_Ephem);
> +		mem_copy_as_ephemeral(pOut, pIn1);
>  		Deephemeralize(pOut);

3. You could turn mem_copy_as_ephemeral + Deephemeralize into mem_copy
the previous commit where mem_copy was introduced.