[Tarantool-patches] [PATCH v5 32/52] sql: introduce mem_set_*() for map and array

[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!
>
>> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
>> index 7885caaf5..583de00a2 100644
>> --- a/src/box/sql/mem.c
>> +++ b/src/box/sql/mem.c
>> @@ -508,6 +508,86 @@ mem_append_to_binary(struct Mem *mem, const char *value, uint32_t size)
>>  	return 0;
>>  }
>>  
>> +void
>> +mem_set_ephemeral_map(struct Mem *mem, char *value, uint32_t size)
>> +{
>> +	assert(mp_typeof(*value) == MP_MAP);
>> +	mem_set_const_bin(mem, value, size, MEM_Ephem);
>> +	mem->flags |= MEM_Subtype;
>> +	mem->subtype = SQL_SUBTYPE_MSGPACK;
>> +	mem->field_type = FIELD_TYPE_MAP;
>> +}
>> +
>> +void
>> +mem_set_static_map(struct Mem *mem, char *value, uint32_t size)
>> +{
>> +	assert(mp_typeof(*value) == MP_MAP);
>> +	mem_set_const_bin(mem, value, size, MEM_Static);
>> +	mem->flags |= MEM_Subtype;
>> +	mem->subtype = SQL_SUBTYPE_MSGPACK;
>> +	mem->field_type = FIELD_TYPE_MAP;
>> +}
>> +
>> +void
>> +mem_set_dynamic_map(struct Mem *mem, char *value, uint32_t size)
>
> I think I lost the clue of what is the difference between dynamic
> and allocated. Maybe worth adding a comment? Or find a better name?
>
Added a comment. Didn't thought about new names for now.

> For instance, if one of them is supposed to copy the map, and the
> other one to steal its ownership, then you could call them
> mem_set_map_copy() and mem_set_map_move(), where move is the same as
> C++ move - steal the resource. The same for the others dynamic/allocated
> terminology in the function names.
>
In some sense dynamic and allocated allocation types steals the ownership of
allocated memory, however there is difference in way they free it. I described
it in comments to the functions.

> I think I also lost the understanding of static vs ephem by now.
Static - available always, ephemeral - may become unavailable at some point.
Thought it is true that the border between these two allocation type not as
clear as should be.


New patch:

commit 26b61648509b08b4cde3307015d398372cdd20f3
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Tue Mar 16 13:50:54 2021 +0300

    sql: introduce mem_set_*() for map and array
    
    This patch introduces set of mem_set_*() functions for MAP and ARRAY.
    These functions clears MEM and sets it to given binary value. Binary
    value must be MAP or ARRAY. Degree of clearing and type of allocation of
    the binary value is determined by the function used.
    
    Part of #5818

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 508b1dee3..61849cde7 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -520,6 +520,75 @@ mem_set_zerobin(struct Mem *mem, int n)
 	mem->field_type = FIELD_TYPE_VARBINARY;
 }
 
+static inline void
+set_msgpack_value(struct Mem *mem, char *value, uint32_t size, int alloc_type,
+		  enum field_type type)
+{
+	if (alloc_type == MEM_Ephem || alloc_type == MEM_Static)
+		set_bin_const(mem, value, size, alloc_type);
+	else
+		set_bin_dynamic(mem, value, size, alloc_type);
+	mem->flags |= MEM_Subtype;
+	mem->subtype = SQL_SUBTYPE_MSGPACK;
+	mem->field_type = type;
+}
+
+void
+mem_set_map_ephemeral(struct Mem *mem, char *value, uint32_t size)
+{
+	assert(mp_typeof(*value) == MP_MAP);
+	set_msgpack_value(mem, value, size, MEM_Ephem, FIELD_TYPE_MAP);
+}
+
+void
+mem_set_map_static(struct Mem *mem, char *value, uint32_t size)
+{
+	assert(mp_typeof(*value) == MP_MAP);
+	set_msgpack_value(mem, value, size, MEM_Static, FIELD_TYPE_MAP);
+}
+
+void
+mem_set_map_dynamic(struct Mem *mem, char *value, uint32_t size)
+{
+	assert(mp_typeof(*value) == MP_MAP);
+	set_msgpack_value(mem, value, size, MEM_Dyn, FIELD_TYPE_MAP);
+}
+
+void
+mem_set_map_allocated(struct Mem *mem, char *value, uint32_t size)
+{
+	assert(mp_typeof(*value) == MP_MAP);
+	set_msgpack_value(mem, value, size, 0, FIELD_TYPE_MAP);
+}
+
+void
+mem_set_array_ephemeral(struct Mem *mem, char *value, uint32_t size)
+{
+	assert(mp_typeof(*value) == MP_ARRAY);
+	set_msgpack_value(mem, value, size, MEM_Ephem, FIELD_TYPE_ARRAY);
+}
+
+void
+mem_set_array_static(struct Mem *mem, char *value, uint32_t size)
+{
+	assert(mp_typeof(*value) == MP_ARRAY);
+	set_msgpack_value(mem, value, size, MEM_Static, FIELD_TYPE_ARRAY);
+}
+
+void
+mem_set_array_dynamic(struct Mem *mem, char *value, uint32_t size)
+{
+	assert(mp_typeof(*value) == MP_ARRAY);
+	set_msgpack_value(mem, value, size, MEM_Dyn, FIELD_TYPE_ARRAY);
+}
+
+void
+mem_set_array_allocated(struct Mem *mem, char *value, uint32_t size)
+{
+	assert(mp_typeof(*value) == MP_ARRAY);
+	set_msgpack_value(mem, value, size, 0, FIELD_TYPE_ARRAY);
+}
+
 int
 mem_copy(struct Mem *to, const struct Mem *from)
 {
@@ -2173,102 +2242,6 @@ mem_set_ptr(struct Mem *mem, void *ptr)
 	mem->u.p = ptr;
 }
 
-/*
- * Change the value of a Mem to be a string or a BLOB.
- *
- * The memory management strategy depends on the value of the xDel
- * parameter. If the value passed is SQL_TRANSIENT, then the
- * string is copied into a (possibly existing) buffer managed by the
- * Mem structure. Otherwise, any existing buffer is freed and the
- * pointer copied.
- *
- * If the string is too large (if it exceeds the SQL_LIMIT_LENGTH
- * size limit) then no memory allocation occurs.  If the string can be
- * stored without allocating memory, then it is.  If a memory allocation
- * is required to store the string, then value of pMem is unchanged.  In
- * either case, error is returned.
- */
-int
-sqlVdbeMemSetStr(Mem * pMem,	/* Memory cell to set to string value */
-		     const char *z,	/* String pointer */
-		     int n,	/* Bytes in string, or negative */
-		     u8 not_blob,	/* Encoding of z.  0 for BLOBs */
-		     void (*xDel) (void *)	/* Destructor function */
-    )
-{
-	int nByte = n;		/* New value for pMem->n */
-	int iLimit;		/* Maximum allowed string or blob size */
-	u16 flags = 0;		/* New value for pMem->flags */
-
-	/* If z is a NULL pointer, set pMem to contain an SQL NULL. */
-	if (!z) {
-		mem_clear(pMem);
-		return 0;
-	}
-
-	if (pMem->db) {
-		iLimit = pMem->db->aLimit[SQL_LIMIT_LENGTH];
-	} else {
-		iLimit = SQL_MAX_LENGTH;
-	}
-	flags = (not_blob == 0 ? MEM_Blob : MEM_Str);
-	if (nByte < 0) {
-		assert(not_blob != 0);
-		nByte = sqlStrlen30(z);
-		if (nByte > iLimit)
-			nByte = iLimit + 1;
-		flags |= MEM_Term;
-	}
-
-	/* The following block sets the new values of Mem.z and Mem.xDel. It
-	 * also sets a flag in local variable "flags" to indicate the memory
-	 * management (one of MEM_Dyn or MEM_Static).
-	 */
-	if (xDel == SQL_TRANSIENT) {
-		int nAlloc = nByte;
-		if (flags & MEM_Term) {
-			nAlloc += 1; //SQL_UTF8
-		}
-		if (nByte > iLimit) {
-			diag_set(ClientError, ER_SQL_EXECUTE, "string or binary"\
-				 "string is too big");
-			return -1;
-		}
-		testcase(nAlloc == 0);
-		testcase(nAlloc == 31);
-		testcase(nAlloc == 32);
-		if (sqlVdbeMemClearAndResize(pMem, MAX(nAlloc, 32))) {
-			return -1;
-		}
-		memcpy(pMem->z, z, nAlloc);
-	} else if (xDel == SQL_DYNAMIC) {
-		mem_destroy(pMem);
-		pMem->zMalloc = pMem->z = (char *)z;
-		pMem->szMalloc = sqlDbMallocSize(pMem->db, pMem->zMalloc);
-	} else {
-		mem_destroy(pMem);
-		pMem->z = (char *)z;
-		pMem->xDel = xDel;
-		flags |= ((xDel == SQL_STATIC) ? MEM_Static : MEM_Dyn);
-	}
-
-	pMem->n = nByte;
-	pMem->flags = flags;
-	assert((pMem->flags & (MEM_Str | MEM_Blob)) != 0);
-	if ((pMem->flags & MEM_Str) != 0)
-		pMem->field_type = FIELD_TYPE_STRING;
-	else
-		pMem->field_type = FIELD_TYPE_VARBINARY;
-
-	if (nByte > iLimit) {
-		diag_set(ClientError, ER_SQL_EXECUTE, "string or binary string"\
-			 "is too big");
-		return -1;
-	}
-
-	return 0;
-}
-
 /*
  * Free an sql_value object
  */
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 0aeb23496..b3a602f6e 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -287,6 +287,71 @@ mem_set_zerobin(struct Mem *mem, int n);
 int
 mem_copy_bin(struct Mem *mem, const char *value, uint32_t size);
 
+/**
+ * Clear MEM and set it to MAP. The binary value belongs to another object. The
+ * binary value must be msgpack of MAP type.
+ */
+void
+mem_set_map_ephemeral(struct Mem *mem, char *value, uint32_t size);
+
+/**
+ * Clear MEM and set it to MAP. The binary value is static. The binary value
+ * must be msgpack of MAP type.
+ */
+void
+mem_set_map_static(struct Mem *mem, char *value, uint32_t size);
+
+/**
+ * Clear MEM and set it to MAP. The binary value was allocated by another object
+ * and passed to MEM. The binary value must be msgpack of MAP type. MEMs with
+ * this allocation type must free given memory whenever the MEM changes.
+ */
+void
+mem_set_map_dynamic(struct Mem *mem, char *value, uint32_t size);
+
+/**
+ * Clear MEM and set it to MAP. The binary value was allocated by another object
+ * and passed to MEM. The binary value must be msgpack of MAP type. MEMs with
+ * this allocation type only deallocate the string on destruction. Also, the
+ * memory may be reallocated if MEM is set to a different value of this
+ * allocation type.
+ */
+void
+mem_set_map_allocated(struct Mem *mem, char *value, uint32_t size);
+
+/**
+ * Clear MEM and set it to ARRAY. The binary value belongs to another object.
+ * The binary value must be msgpack of ARRAY type.
+ */
+void
+mem_set_array_ephemeral(struct Mem *mem, char *value, uint32_t size);
+
+/**
+ * Clear MEM and set it to ARRAY. The binary value is static. The binary value
+ * must be msgpack of ARRAY type.
+ */
+void
+mem_set_array_static(struct Mem *mem, char *value, uint32_t size);
+
+/**
+ * Clear MEM and set it to ARRAY. The binary value was allocated by another
+ * object and passed to MEM. The binary value must be msgpack of ARRAY type.
+ * MEMs with this allocation type must free given memory whenever the MEM
+ * changes.
+ */
+void
+mem_set_array_dynamic(struct Mem *mem, char *value, uint32_t size);
+
+/**
+ * Clear MEM and set it to ARRAY. The binary value was allocated by another
+ * object and passed to MEM. The binary value must be msgpack of ARRAY type.
+ * MEMs with this allocation type only deallocate the string on destruction.
+ * Also, the memory may be reallocated if MEM is set to a different value of
+ * this allocation type.
+ */
+void
+mem_set_array_allocated(struct Mem *mem, 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
@@ -564,8 +629,6 @@ int sqlVdbeMemClearAndResize(struct Mem * pMem, int n);
 void
 mem_set_ptr(struct Mem *mem, void *ptr);
 
-int
-sqlVdbeMemSetStr(struct Mem *, const char *, int, u8, void (*)(void *));
 void sqlValueFree(struct Mem *);
 struct Mem *sqlValueNew(struct sql *);
 
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 19a0b041c..9434a4d06 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -894,9 +894,11 @@ case OP_Blob: {                /* out2 */
 		 */
 		mem_set_bin_static(pOut, pOp->p4.z, pOp->p1);
 	} else {
-		sqlVdbeMemSetStr(pOut, pOp->p4.z, pOp->p1, 0, 0);
-		pOut->flags |= MEM_Subtype;
-		pOut->subtype = pOp->p3;
+		assert(pOp->p3 == SQL_SUBTYPE_MSGPACK);
+		if (mp_typeof(*pOp->p4.z) == MP_MAP)
+			mem_set_map_static(pOut, pOp->p4.z, pOp->p1);
+		else
+			mem_set_array_static(pOut, pOp->p4.z, pOp->p1);
 	}
 	UPDATE_MAX_BLOBSIZE(pOut);
 	break;