Re: [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. Patch was a bit
changed due to merge conflicts with new version of 
"sql: introduce mem_is_*() functions".

On Tue, Apr 13, 2021 at 01:36:15AM +0200, Vladislav Shpilevoy wrote:
> I appreciate the work you did here!
> 
> See 2 comments below.
> 
> > 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);
> > +
> > +
> 
> 1. Double empty line.
> 
Fixed during resolving merge conflicts with new version of
"sql: introduce mem_is_*() functions".

> >  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);
> 
> 2. The same comment as for copy_str.
> 
Same as there, in case copied value is value of the MEM it copied to and its
allocation type being MEM_Dyn, it will be lost during clear().

> > +	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;
> > +}



New patch:


commit 8788956575ecf095d86d63de5eeea2e932fb0bac
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 1e7adfcb0..190311d80 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -40,6 +40,18 @@
 #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);
+
 enum {
 	BUF_SIZE = 32,
 };
@@ -335,6 +347,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)
 {
@@ -1869,17 +1898,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 617966cf7..197c16461 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -456,6 +456,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
@@ -699,7 +706,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 cd7811846..d573e1e8f 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2240,11 +2240,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
@@ -2584,7 +2581,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 7114f4351..7656d8d67 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -168,7 +168,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;
 }
 
@@ -807,7 +807,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 dad9cab03..ffc85b91d 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;
 }