[Tarantool-patches] [PATCH v5 30/52] sql: introduce mem_copy_bin()

[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:05, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> On 23.03.2021 10:36, Mergen Imeev via Tarantool-patches wrote:
>> This patch introduces mem_copy_binary() function. Function
>> mem_copy_binary() clears MEM, allocates enough memory and copies given
>> binary to allocated memory.
>>
>> Part of #5818
>> ---
>>  src/box/sql/mem.c      | 14 ++++++++++++++
>>  src/box/sql/mem.h      |  3 +++
>>  src/box/sql/vdbe.c     |  7 ++-----
>>  src/box/sql/vdbeapi.c  |  4 ++--
>>  src/box/sql/vdbesort.c |  6 +-----
>>  5 files changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
>> index 5ee49cdca..99beec9ad 100644
>> --- a/src/box/sql/mem.c
>> +++ b/src/box/sql/mem.c
>> @@ -465,6 +465,20 @@ mem_set_allocated_binary(struct Mem *mem, char *value, uint32_t size)
>>  	mem_set_dyn_bin(mem, value, size, 0);
>>  }
>>  
>> +int
>> +mem_copy_binary(struct Mem *mem, const char *value, uint32_t size)
>> +{
>
> What if mem is not a binary now? What if it is a frame? Why don't you clear it?
>
Thanks! Fixed here and in mem_copy_str(), mem_copy_str0() functions.

>> +	bool is_own_value = (mem->flags & MEM_Blob) != 0 && mem->z == value;
>> +	if (sqlVdbeMemGrow(mem, size, is_own_value) != 0)
>> +		return -1;
>> +	if (!is_own_value)
>> +		memcpy(mem->z, value, size);
>> +	mem->n = size;
>> +	mem->flags = MEM_Blob;
>> +	mem->field_type = FIELD_TYPE_VARBINARY;
>> +	return 0;
>> +}



New patch:

commit ab44b535847f591f638826e19277b85bab9f6189
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Tue Mar 16 11:55:05 2021 +0300

    sql: introduce mem_copy_bin()
    
    This patch introduces mem_copy_bin() function. This function copies
    given binary value to a newly allocated by MEM memory.
    
    Part of #5818

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 2622cdd82..e30795de5 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -40,6 +40,19 @@
 #include "lua/utils.h"
 #include "lua/msgpack.h"
 
+/*
+ * Make sure pMem->z points to a writable allocation of at least
+ * min(n,32) bytes.
+ *
+ * If the bPreserve argument is true, then copy of the content of
+ * pMem->z into the new allocation.  pMem must be either a string or
+ * blob if bPreserve is true.  If bPreserve is false, any prior content
+ * in pMem->z is discarded.
+ */
+static int
+sqlVdbeMemGrow(struct Mem *pMem, int n, int preserve);
+
+
 bool
 mem_is_null(const struct Mem *mem)
 {
@@ -477,6 +490,23 @@ mem_set_bin_allocated(struct Mem *mem, char *value, uint32_t size)
 	set_bin_dynamic(mem, value, size, 0);
 }
 
+int
+mem_copy_bin(struct Mem *mem, const char *value, uint32_t size)
+{
+	if ((mem->flags & (MEM_Agg | MEM_Frame)) != 0)
+		mem_clear(mem);
+	bool is_own_value = (mem->flags & (MEM_Str | MEM_Blob)) != 0 &&
+			    mem->z == value;
+	if (sqlVdbeMemGrow(mem, size, is_own_value) != 0)
+		return -1;
+	if (!is_own_value)
+		memcpy(mem->z, value, size);
+	mem->n = size;
+	mem->flags = MEM_Blob;
+	mem->field_type = FIELD_TYPE_VARBINARY;
+	return 0;
+}
+
 int
 mem_copy(struct Mem *to, const struct Mem *from)
 {
@@ -2046,17 +2076,8 @@ mem_convert_to_numeric(struct Mem *mem, enum field_type type)
 	return mem_convert_to_integer(mem);
 }
 
-/*
- * Make sure pMem->z points to a writable allocation of at least
- * min(n,32) bytes.
- *
- * If the bPreserve argument is true, then copy of the content of
- * pMem->z into the new allocation.  pMem must be either a string or
- * blob if bPreserve is true.  If bPreserve is false, any prior content
- * in pMem->z is discarded.
- */
-SQL_NOINLINE int
-sqlVdbeMemGrow(Mem * pMem, int n, int bPreserve)
+static int
+sqlVdbeMemGrow(struct Mem *pMem, int n, int bPreserve)
 {
 	assert(sqlVdbeCheckMemInvariants(pMem));
 	testcase(pMem->db == 0);
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 532860ff7..e17a722f4 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -274,6 +274,13 @@ mem_set_bin_dynamic(struct Mem *mem, char *value, uint32_t size);
 void
 mem_set_bin_allocated(struct Mem *mem, char *value, uint32_t size);
 
+/**
+ * Copy binary value to a newly allocated memory. The MEM type becomes
+ * VARBINARY.
+ */
+int
+mem_copy_bin(struct Mem *mem, const char *value, uint32_t size);
+
 /**
  * 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
@@ -541,7 +548,6 @@ mem_convert_to_numeric(struct Mem *mem, enum field_type type);
 
 /** Setters = Change MEM value. */
 
-int sqlVdbeMemGrow(struct Mem * pMem, int n, int preserve);
 int sqlVdbeMemClearAndResize(struct Mem * pMem, int n);
 
 /**
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 85fb7a26d..19a0b041c 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2196,11 +2196,8 @@ case OP_MakeRecord: {
 	 * routine.
 	 */
 	if (bIsEphemeral) {
-		if (sqlVdbeMemClearAndResize(pOut, tuple_size) != 0)
+		if (mem_copy_bin(pOut, tuple, tuple_size) != 0)
 			goto abort_due_to_error;
-		pOut->flags = MEM_Blob;
-		pOut->n = tuple_size;
-		memcpy(pOut->z, tuple, tuple_size);
 		region_truncate(region, used);
 	} else {
 		/* Allocate memory on the region for the tuple
@@ -2540,7 +2537,7 @@ case OP_SequenceTest: {
  * Open a new cursor that points to a fake table that contains a single
  * row of data.  The content of that one row is the content of memory
  * register P2.  In other words, cursor P1 becomes an alias for the
- * MEM_Blob content contained in register P2.
+ * MEM with binary content contained in register P2.
  *
  * A pseudo-table created by this opcode is used to hold a single
  * row output from the sorter so that the row can be decomposed into
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 8c7a0b6cd..d16b92cf6 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -188,7 +188,7 @@ sql_result_blob(sql_context * pCtx,
 		mem_set_bin_allocated(pCtx->pOut, (char *)z, n);
 	else if (xDel != SQL_TRANSIENT)
 		mem_set_bin_dynamic(pCtx->pOut, (char *)z, n);
-	else if (sqlVdbeMemSetStr(pCtx->pOut, z, n, 0, xDel) != 0)
+	else if (mem_copy_bin(pCtx->pOut, z, n) != 0)
 		pCtx->is_aborted = true;
 }
 
@@ -843,7 +843,7 @@ sql_bind_blob(sql_stmt * pStmt,
 		mem_set_bin_allocated(var, (char *)zData, nData);
 	else if (xDel != SQL_TRANSIENT)
 		mem_set_bin_dynamic(var, (char *)zData, nData);
-	else if (sqlVdbeMemSetStr(var, zData, nData, 0, xDel) != 0)
+	else if (mem_copy_bin(var, zData, nData) != 0)
 		return -1;
 	return sql_bind_type(p, i, "varbinary");
 }
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 171cb8946..c294a0286 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1305,19 +1305,26 @@ sqlVdbeList(Vdbe * p)
 			 * has not already been seen.
 			 */
 			if (pOp->p4type == P4_SUBPROGRAM) {
-				int nByte = (nSub + 1) * sizeof(SubProgram *);
 				int j;
 				for (j = 0; j < nSub; j++) {
 					if (apSub[j] == pOp->p4.pProgram)
 						break;
 				}
-				if (j == nSub &&
-				    sqlVdbeMemGrow(pSub, nByte,
-						   nSub != 0) == 0) {
-					apSub = (SubProgram **) pSub->z;
-					apSub[nSub++] = pOp->p4.pProgram;
-					pSub->flags |= MEM_Blob;
-					pSub->n = nSub * sizeof(SubProgram *);
+				if (nSub == 0) {
+					uint32_t size = sizeof(SubProgram *);
+					char *bin = (char *)&pOp->p4.pProgram;
+					if (mem_copy_bin(pSub, bin, size) != 0)
+						return -1;
+				} else if (j == nSub) {
+					struct Mem tmp;
+					mem_create(&tmp);
+					uint32_t size = sizeof(SubProgram *);
+					char *bin = (char *)&pOp->p4.pProgram;
+					mem_set_bin_ephemeral(&tmp, bin, size);
+					int rc = mem_concat(pSub, &tmp, pSub);
+					mem_destroy(&tmp);
+					if (rc != 0)
+						return -1;
 				}
 			}
 		}
diff --git a/src/box/sql/vdbesort.c b/src/box/sql/vdbesort.c
index a9a5f45af..3da425be4 100644
--- a/src/box/sql/vdbesort.c
+++ b/src/box/sql/vdbesort.c
@@ -2164,12 +2164,8 @@ sqlVdbeSorterRowkey(const VdbeCursor * pCsr, Mem * pOut)
 	assert(pCsr->eCurType == CURTYPE_SORTER);
 	pSorter = pCsr->uc.pSorter;
 	pKey = vdbeSorterRowkey(pSorter, &nKey);
-	if (sqlVdbeMemClearAndResize(pOut, nKey)) {
+	if (mem_copy_bin(pOut, pKey, nKey) != 0)
 		return -1;
-	}
-	pOut->n = nKey;
-	MemSetTypeFlag(pOut, MEM_Blob);
-	memcpy(pOut->z, pKey, nKey);
 
 	return 0;
 }