Re: [Tarantool-patches] [PATCH v5 13/52] sql: introduce mem_copy()

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

Thank you for the review! My answer, diff and new patch below.

On Sun, Apr 11, 2021 at 08:06:24PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> >> Also what was wrong with sqlVdbeMemCopy's way of using sqlVdbeMemMakeWriteable?
> >>
> > I see that this as a hack. It changes dynamic or allocated type (only type!) to
> > ephemeral and then calls sqlVdbeMemMakeWriteable(), which converts ephemeral
> > value to allocated value. Isn't it better to just directly copy?
> 
> Yes, your way sounds better.
> 
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index 25b2e75ee..ea3917fe3 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -267,6 +267,35 @@ mem_destroy(struct Mem *mem)
> >  	mem->zMalloc = NULL;
> >  }
> >  
> > +int
> > +mem_copy(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 0;
> > +	if ((to->flags & MEM_Static) != 0)
> > +		return 0;
> > +	if ((to->flags & (MEM_Zero | MEM_Blob)) == (MEM_Zero | MEM_Blob))
> > +		return sqlVdbeMemExpandBlob(to);
> > +	if (to->szMalloc == 0)
> > +		to->zMalloc = sqlDbMallocRaw(to->db, to->n);
> > +	else
> > +		to->zMalloc = sqlDbReallocOrFree(to->db, to->zMalloc, to->n);
> 
> You can call realloc always. It turns into malloc when
> the pointer is NULL, which is the case for szMalloc == 0
> I think.
> 
Thanks, fixed.

> > +	if (to->zMalloc == NULL)
> > +		return -1;
> > +	to->szMalloc = sqlDbMallocSize(to->db, to->zMalloc);
> > +	memcpy(to->zMalloc, to->z, to->n);
> > +	to->z = to->zMalloc;
> > +	to->flags &= (MEM_Str | MEM_Blob | MEM_Term | MEM_Subtype);
> > +	return 0;
> > +}


Diff:


diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 78a4fe1a5..79b330141 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -141,10 +141,7 @@ mem_copy(struct Mem *to, const struct Mem *from)
 		return 0;
 	if ((to->flags & (MEM_Zero | MEM_Blob)) == (MEM_Zero | MEM_Blob))
 		return sqlVdbeMemExpandBlob(to);
-	if (to->szMalloc == 0)
-		to->zMalloc = sqlDbMallocRaw(to->db, to->n);
-	else
-		to->zMalloc = sqlDbReallocOrFree(to->db, to->zMalloc, to->n);
+	to->zMalloc = sqlDbReallocOrFree(to->db, to->zMalloc, to->n);
 	if (to->zMalloc == NULL)
 		return -1;
 	to->szMalloc = sqlDbMallocSize(to->db, to->zMalloc);


New patch:


commit 38aceb6474bf967fc464561f5ea6e5e934257924
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Tue Mar 23 00:53:03 2021 +0300

    sql: introduce mem_copy()
    
    This patch introduces mem_copy(). This function copies value from source
    MEM to destination MEM. In case value is string or binary and have not
    static allocation type, it is copied to newly allocated memory.
    
    Part of #5818

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index a0108220f..0b85bf365 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1769,13 +1769,13 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
 		bool is_max = (func->flags & SQL_FUNC_MAX) != 0;
 		cmp = sqlMemCompare(pBest, pArg, pColl);
 		if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
-			sqlVdbeMemCopy(pBest, pArg);
+			mem_copy(pBest, pArg);
 		} else {
 			sqlSkipAccumulatorLoad(context);
 		}
 	} else {
 		pBest->db = sql_context_db_handle(context);
-		sqlVdbeMemCopy(pBest, pArg);
+		mem_copy(pBest, pArg);
 	}
 }
 
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 5eef15c62..79b330141 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -125,6 +125,32 @@ mem_destroy(struct Mem *mem)
 	mem->z = NULL;
 }
 
+int
+mem_copy(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 0;
+	if ((to->flags & MEM_Static) != 0)
+		return 0;
+	if ((to->flags & (MEM_Zero | MEM_Blob)) == (MEM_Zero | MEM_Blob))
+		return sqlVdbeMemExpandBlob(to);
+	to->zMalloc = sqlDbReallocOrFree(to->db, to->zMalloc, to->n);
+	if (to->zMalloc == NULL)
+		return -1;
+	to->szMalloc = sqlDbMallocSize(to->db, to->zMalloc);
+	memcpy(to->zMalloc, to->z, to->n);
+	to->z = to->zMalloc;
+	to->flags &= (MEM_Str | MEM_Blob | MEM_Term | MEM_Subtype);
+	return 0;
+}
+
 static inline bool
 mem_has_msgpack_subtype(struct Mem *mem)
 {
@@ -1818,28 +1844,6 @@ vdbe_mem_alloc_blob_region(struct Mem *vdbe_mem, uint32_t size)
 	return 0;
 }
 
-/*
- * Make a full copy of pFrom into pTo.  Prior contents of pTo are
- * freed before the copy is made.
- */
-int
-sqlVdbeMemCopy(Mem * pTo, const Mem * pFrom)
-{
-	int rc = 0;
-
-	mem_clear(pTo);
-	memcpy(pTo, pFrom, MEMCELLSIZE);
-	pTo->flags &= ~MEM_Dyn;
-	if (pTo->flags & (MEM_Str | MEM_Blob)) {
-		if (0 == (pFrom->flags & MEM_Static)) {
-			pTo->flags |= MEM_Ephem;
-			rc = sqlVdbeMemMakeWriteable(pTo);
-		}
-	}
-
-	return rc;
-}
-
 void
 sqlVdbeMemShallowCopy(Mem * pTo, const Mem * pFrom, int srcType)
 {
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 041d8a414..86dcdaec0 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -306,6 +306,14 @@ mem_create(struct Mem *mem);
 void
 mem_destroy(struct Mem *mem);
 
+/**
+ * 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 to
+ * newly allocated by destination MEM memory.
+ */
+int
+mem_copy(struct Mem *to, const struct Mem *from);
+
 /**
  * Simple type to str convertor. It is used to simplify
  * error reporting.
@@ -553,7 +561,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);
-int sqlVdbeMemCopy(Mem *, const Mem *);
 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 eb7d77f7e..a4718cef7 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1008,11 +1008,7 @@ case OP_Copy: {
 	pOut = &aMem[pOp->p2];
 	assert(pOut!=pIn1);
 	while( 1) {
-		sqlVdbeMemShallowCopy(pOut, pIn1, MEM_Ephem);
-		Deephemeralize(pOut);
-#ifdef SQL_DEBUG
-		pOut->pScopyFrom = 0;
-#endif
+		mem_copy(pOut, pIn1);
 		REGISTER_TRACE(p, pOp->p2+pOp->p3-n, pOut);
 		if ((n--)==0) break;
 		pOut++;
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 2a3561d42..7951996ea 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -229,7 +229,7 @@ sql_result_text64(sql_context * pCtx,
 void
 sql_result_value(sql_context * pCtx, sql_value * pValue)
 {
-	sqlVdbeMemCopy(pCtx->pOut, pValue);
+	mem_copy(pCtx->pOut, pValue);
 }
 
 void
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 7ccc6a957..cd89fc899 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2333,7 +2333,7 @@ sqlVdbeGetBoundValue(Vdbe * v, int iVar, u8 aff)
 		if (!mem_is_null(pMem)) {
 			sql_value *pRet = sqlValueNew(v->db);
 			if (pRet) {
-				sqlVdbeMemCopy((Mem *) pRet, pMem);
+				mem_copy(pRet, pMem);
 				sql_value_apply_type(pRet, aff);
 			}
 			return pRet;
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index bb87bb902..91cba9962 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -415,8 +415,7 @@ stat4ValueFromExpr(Parse * pParse,	/* Parse context */
 		if ((v = pParse->pReprepare) != 0) {
 			pVal = valueNew(db, pAlloc);
 			if (pVal) {
-				rc = sqlVdbeMemCopy((Mem *) pVal,
-							&v->aVar[iBindVar - 1]);
+				rc = mem_copy(pVal, &v->aVar[iBindVar - 1]);
 				if (rc == 0)
 					sql_value_apply_type(pVal, type);
 				pVal->db = pParse->db;