[Tarantool-patches] [PATCH v5 14/52] sql: introduce mem_copy_as_ephemeral()

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

Thank you for the review! My answers and new patch below.


On 30.03.2021 02:01, Vladislav Shpilevoy wrote:
> 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?
>
Fixed, now this function has void return type.

>> +{
>> +	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?
>
This check is only useful for strings and binaries, since they may be lost due
to change of another MEM. Also, due to this function it was possible that value
of type other than MEM_Blob or MEM_Str will have MEM_Ephem set. This is wrong, I
believe.

>>  		}
>>  	}
>>  	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.
Thanks, fixed in previous patch.


New patch:

commit 91cec4dbcc1d931b70cdba3c70d77e7c58a00675
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Mar 4 18:19:06 2021 +0300

    sql: introduce mem_copy_as_ephemeral()
    
    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

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index ea3917fe3..f75661e04 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -296,6 +296,25 @@ mem_copy(struct Mem *to, const struct Mem *from)
 	return 0;
 }
 
+void
+mem_copy_as_ephemeral(struct Mem *to, const struct Mem *from)
+{
+	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;
+	if ((to->flags & (MEM_Static | MEM_Ephem)) != 0)
+		return;
+	to->flags &= (MEM_Str | MEM_Blob | MEM_Term | MEM_Zero | MEM_Subtype);
+	to->flags |= MEM_Ephem;
+	return;
+}
+
 static inline bool
 mem_has_msgpack_subtype(struct Mem *mem)
 {
@@ -407,20 +426,6 @@ vdbeMemAddTerminator(Mem * pMem)
 	return 0;
 }
 
-/*
- * Make an shallow copy of pFrom into pTo.  Prior contents of
- * pTo are freed.  The pFrom->z field is not duplicated.  If
- * pFrom->z is used, then pTo->z points to the same thing as pFrom->z
- * and flags gets srcType (either MEM_Ephem or MEM_Static).
- */
-static SQL_NOINLINE void
-vdbeClrCopy(Mem * pTo, const Mem * pFrom, int eType)
-{
-	mem_clear(pTo);
-	assert(!VdbeMemDynamic(pTo));
-	sqlVdbeMemShallowCopy(pTo, pFrom, eType);
-}
-
 /*
  * Both *pMem1 and *pMem2 contain string values. Compare the two values
  * using the collation sequence pColl. As usual, return a negative , zero
@@ -1989,22 +1994,6 @@ vdbe_mem_alloc_blob_region(struct Mem *vdbe_mem, uint32_t size)
 	return 0;
 }
 
-void
-sqlVdbeMemShallowCopy(Mem * pTo, const Mem * pFrom, int srcType)
-{
-	assert(pTo->db == pFrom->db);
-	if (VdbeMemDynamic(pTo)) {
-		vdbeClrCopy(pTo, pFrom, srcType);
-		return;
-	}
-	memcpy(pTo, pFrom, MEMCELLSIZE);
-	if ((pFrom->flags & MEM_Static) == 0) {
-		pTo->flags &= ~(MEM_Dyn | MEM_Static | MEM_Ephem);
-		assert(srcType == MEM_Ephem || srcType == MEM_Static);
-		pTo->flags |= srcType;
-	}
-}
-
 /*
  * Transfer the contents of pFrom to pTo. Any existing value in pTo is
  * freed. If pFrom contains ephemeral data, a copy is made.
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index af36f31a2..3898888ff 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -81,12 +81,6 @@ struct Mem {
 #endif
 };
 
-/*
- * Size of struct Mem not including the Mem.zMalloc member or anything that
- * follows.
- */
-#define MEMCELLSIZE offsetof(Mem,zMalloc)
-
 bool
 mem_is_null(const struct Mem *mem);
 
@@ -176,6 +170,14 @@ mem_destroy(struct Mem *mem);
 int
 mem_copy(struct Mem *to, const struct Mem *from);
 
+/**
+ * Copy content of MEM from one MEM to another. In case source MEM contains
+ * string or binary and allocation type is not STATIC, this value is copied as
+ * value with ephemeral allocation type.
+ */
+void
+mem_copy_as_ephemeral(struct Mem *to, const struct Mem *from);
+
 /* One or more of the following flags are set to indicate the validOK
  * representations of the value stored in the Mem struct.
  *
@@ -475,7 +477,6 @@ mem_is_type_compatible(struct Mem *mem, enum field_type type);
 
 int
 vdbe_mem_alloc_blob_region(struct Mem *vdbe_mem, uint32_t size);
-void sqlVdbeMemShallowCopy(Mem *, const Mem *, int);
 void sqlVdbeMemMove(Mem *, Mem *);
 int sqlVdbeMemMakeWriteable(Mem *);
 
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index abd49b9bb..bbeabf238 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -74,9 +74,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 (mem_is_bytes(pX) && !mem_is_ephemeral(pX) &&
+		    !mem_is_static(pX)) {
+			if (pX->pScopyFrom == pMem) {
+				pX->flags |= MEM_Undefined;
+				pX->pScopyFrom = 0;
+			}
 		}
 	}
 	pMem->pScopyFrom = 0;
@@ -946,7 +949,7 @@ case OP_Variable: {            /* out2 */
 		goto too_big;
 	}
 	pOut = vdbe_prepare_null_out(p, pOp->p2);
-	sqlVdbeMemShallowCopy(pOut, pVar, MEM_Static);
+	mem_copy_as_ephemeral(pOut, pVar);
 	UPDATE_MAX_BLOBSIZE(pOut);
 	break;
 }
@@ -1034,7 +1037,7 @@ case OP_SCopy: {            /* out2 */
 	pIn1 = &aMem[pOp->p1];
 	pOut = &aMem[pOp->p2];
 	assert(pOut!=pIn1);
-	sqlVdbeMemShallowCopy(pOut, pIn1, MEM_Ephem);
+	mem_copy_as_ephemeral(pOut, pIn1);
 #ifdef SQL_DEBUG
 	if (pOut->pScopyFrom==0) pOut->pScopyFrom = pIn1;
 #endif
@@ -2313,7 +2316,7 @@ case OP_Column: {
 	if (mem_is_null(pDest) &&
 	    (uint32_t) p2  >= pC->field_ref.field_count &&
 	    default_val_mem != NULL) {
-		sqlVdbeMemShallowCopy(pDest, default_val_mem, MEM_Static);
+		mem_copy_as_ephemeral(pDest, default_val_mem);
 	}
 	pDest->field_type = field_type;
 op_column_out:
@@ -4491,7 +4494,7 @@ case OP_Param: {           /* out2 */
 	pOut = vdbe_prepare_null_out(p, pOp->p2);
 	pFrame = p->pFrame;
 	pIn = &pFrame->aMem[pOp->p1 + pFrame->aOp[pFrame->pc].p1];
-	sqlVdbeMemShallowCopy(pOut, pIn, MEM_Ephem);
+	mem_copy_as_ephemeral(pOut, pIn);
 	break;
 }