Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v3 0/2] Encapsulate MEM type changing and checking
@ 2021-02-20 16:59 Mergen Imeev via Tarantool-patches
  2021-02-20 16:59 ` [Tarantool-patches] [PATCH v3 1/2] sql: Initialize MEM in sqlVdbeAllocUnpackedRecord() Mergen Imeev via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-20 16:59 UTC (permalink / raw)
  To: v.shpilevoy, sergos, tsafin; +Cc: tarantool-patches

This patch-set is part of issue #5818. It changes the way to check and change
MEM from outside. However, there is almost no changes in functions that work
with internal structure of MEM, most of which located in vdbemem.c. This will be
done in another patch-set.

https://github.com/tarantool/tarantool/issues/5818
https://github.com/tarantool/tarantool/tree/imeevma/gh-5818-encapsulate-mem-type-checking-and-changing

Changes in v3:
  - Inlined most of the introduced functions to improve performance.
  - Some other fixes in code to improve performance.

Changes in v2:
  - Squashed almost all patches.
  - Review fixes.

Mergen Imeev (2):
  sql: Initialize MEM in sqlVdbeAllocUnpackedRecord()
  sql: Encapsulate MEM type changing and checking

 src/box/sql/func.c      |  14 +-
 src/box/sql/sqlInt.h    |   1 -
 src/box/sql/vdbe.c      | 513 ++++++++++++++++++----------------------
 src/box/sql/vdbeInt.h   | 465 +++++++++++++++++++++++++++++++++---
 src/box/sql/vdbeapi.c   |  57 ++---
 src/box/sql/vdbeaux.c   | 360 ++++++++++++++--------------
 src/box/sql/vdbemem.c   | 146 +-----------
 src/box/sql/vdbesort.c  |   9 +-
 src/box/sql/vdbetrace.c |  12 +-
 9 files changed, 874 insertions(+), 703 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Tarantool-patches] [PATCH v3 1/2] sql: Initialize MEM in sqlVdbeAllocUnpackedRecord()
  2021-02-20 16:59 [Tarantool-patches] [PATCH v3 0/2] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
@ 2021-02-20 16:59 ` Mergen Imeev via Tarantool-patches
  2021-02-20 16:59 ` [Tarantool-patches] [PATCH v3 2/2] sql: Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
  2021-02-28 17:35 ` [Tarantool-patches] [PATCH v3 0/2] " Vladislav Shpilevoy via Tarantool-patches
  2 siblings, 0 replies; 7+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-20 16:59 UTC (permalink / raw)
  To: v.shpilevoy, sergos, tsafin; +Cc: tarantool-patches

This patch adds initialization for newly created MEM objects in
sqlVdbeAllocUnpackedRecord(). Changing a MEM without being
initialized may give us unexpected result.

Part of #5818
---
 src/box/sql/vdbeaux.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 91b64316e..5c2706e49 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2329,6 +2329,8 @@ sqlVdbeAllocUnpackedRecord(struct sql *db, struct key_def *key_def)
 	if (!p)
 		return 0;
 	p->aMem = (Mem *) & ((char *)p)[ROUND8(sizeof(UnpackedRecord))];
+	for (uint32_t i = 0; i < key_def->part_count + 1; ++i)
+		sqlVdbeMemInit(&p->aMem[i], sql_get(), MEM_Null);
 	p->key_def = key_def;
 	p->nField = key_def->part_count + 1;
 	return p;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Tarantool-patches] [PATCH v3 2/2] sql: Encapsulate MEM type changing and checking
  2021-02-20 16:59 [Tarantool-patches] [PATCH v3 0/2] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
  2021-02-20 16:59 ` [Tarantool-patches] [PATCH v3 1/2] sql: Initialize MEM in sqlVdbeAllocUnpackedRecord() Mergen Imeev via Tarantool-patches
@ 2021-02-20 16:59 ` Mergen Imeev via Tarantool-patches
  2021-02-28 17:35   ` Vladislav Shpilevoy via Tarantool-patches
  2021-02-28 17:35 ` [Tarantool-patches] [PATCH v3 0/2] " Vladislav Shpilevoy via Tarantool-patches
  2 siblings, 1 reply; 7+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-20 16:59 UTC (permalink / raw)
  To: v.shpilevoy, sergos, tsafin; +Cc: tarantool-patches

This patch encapsulates type changing and checking for MEM. This is done to make
it easier for us to introduce new rules for implicit and explicit type casting
and new types in SQL.

Part of #5818
---
 src/box/sql/func.c      |  14 +-
 src/box/sql/sqlInt.h    |   1 -
 src/box/sql/vdbe.c      | 513 ++++++++++++++++++----------------------
 src/box/sql/vdbeInt.h   | 465 +++++++++++++++++++++++++++++++++---
 src/box/sql/vdbeapi.c   |  57 ++---
 src/box/sql/vdbeaux.c   | 360 ++++++++++++++--------------
 src/box/sql/vdbemem.c   | 146 +-----------
 src/box/sql/vdbesort.c  |   9 +-
 src/box/sql/vdbetrace.c |  12 +-
 9 files changed, 873 insertions(+), 704 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index f15d27051..70b8f8eb7 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -94,7 +94,7 @@ vdbemem_alloc_on_region(uint32_t count)
 	}
 	memset(ret, 0, count * sizeof(*ret));
 	for (uint32_t i = 0; i < count; i++) {
-		sqlVdbeMemInit(&ret[i], sql_get(), MEM_Null);
+		mem_init(&ret[i]);
 		assert(memIsValid(&ret[i]));
 	}
 	return ret;
@@ -283,13 +283,12 @@ port_lua_get_vdbemem(struct port *base, uint32_t *size)
 			mem_set_u64(&val[i], field.ival);
 			break;
 		case MP_STR:
-			if (sqlVdbeMemSetStr(&val[i], field.sval.data,
-					     field.sval.len, 1,
-					     SQL_TRANSIENT) != 0)
+			if (mem_copy_str(&val[i], field.sval.data,
+					 field.sval.len, false) != 0)
 				goto error;
 			break;
 		case MP_NIL:
-			sqlVdbeMemSetNull(&val[i]);
+			mem_set_null(&val[i]);
 			break;
 		default:
 			diag_set(ClientError, ER_SQL_EXECUTE,
@@ -355,12 +354,11 @@ port_c_get_vdbemem(struct port *base, uint32_t *size)
 			break;
 		case MP_STR:
 			str = mp_decode_str(&data, &len);
-			if (sqlVdbeMemSetStr(&val[i], str, len,
-					     1, SQL_TRANSIENT) != 0)
+			if (mem_copy_str(&val[i], str, len, false) != 0)
 				goto error;
 			break;
 		case MP_NIL:
-			sqlVdbeMemSetNull(&val[i]);
+			mem_set_null(&val[i]);
 			break;
 		default:
 			diag_set(ClientError, ER_SQL_EXECUTE,
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 95a6fedc9..891693b83 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4004,7 +4004,6 @@ const void *sqlValueText(sql_value *);
 int sqlValueBytes(sql_value *);
 void sqlValueSetStr(sql_value *, int, const void *,
 			void (*)(void *));
-void sqlValueSetNull(sql_value *);
 void sqlValueFree(sql_value *);
 sql_value *sqlValueNew(sql *);
 int sqlValueFromExpr(sql *, Expr *, enum field_type type,
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 3b3b1f01d..8b87165ee 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -116,7 +116,7 @@ int sql_max_blobsize = 0;
 static void
 updateMaxBlobsize(Mem *p)
 {
-	if ((p->flags & (MEM_Str|MEM_Blob))!=0 && p->n>sql_max_blobsize) {
+	if (mem_is_varstring(p) && p->n>sql_max_blobsize) {
 		sql_max_blobsize = p->n;
 	}
 }
@@ -185,7 +185,7 @@ vdbeTakeBranch(int iSrcLine, u8 I, u8 M)
  * already. Return non-zero if a malloc() fails.
  */
 #define Stringify(P)						\
-	if(((P)->flags&(MEM_Str|MEM_Blob))==0 && sqlVdbeMemStringify(P)) \
+	if(!mem_is_varstring(P) && sqlVdbeMemStringify(P)) \
 	{ goto no_mem; }
 
 /*
@@ -260,7 +260,7 @@ allocateCursor(
 int
 mem_apply_numeric_type(struct Mem *record)
 {
-	if ((record->flags & MEM_Str) == 0)
+	if (!mem_is_string(record))
 		return -1;
 	int64_t integer_value;
 	bool is_neg;
@@ -311,17 +311,17 @@ mem_apply_numeric_type(struct Mem *record)
 static int
 mem_apply_type(struct Mem *record, enum field_type type)
 {
-	if ((record->flags & MEM_Null) != 0)
+	if (mem_is_null(record))
 		return 0;
 	assert(type < field_type_MAX);
 	switch (type) {
 	case FIELD_TYPE_INTEGER:
 	case FIELD_TYPE_UNSIGNED:
-		if ((record->flags & (MEM_Bool | MEM_Blob)) != 0)
+		if (mem_is_bool(record) || mem_is_binary(record))
 			return -1;
-		if ((record->flags & MEM_UInt) == MEM_UInt)
+		if (mem_is_pos_int(record))
 			return 0;
-		if ((record->flags & MEM_Real) == MEM_Real) {
+		if (mem_is_double(record)) {
 			double d = record->u.r;
 			if (d >= 0) {
 				if (double_compare_uint64(d, UINT64_MAX,
@@ -337,29 +337,29 @@ mem_apply_type(struct Mem *record, enum field_type type)
 			}
 			return 0;
 		}
-		if ((record->flags & MEM_Str) != 0) {
+		if (mem_is_string(record)) {
 			bool is_neg;
 			int64_t i;
 			if (sql_atoi64(record->z, &i, &is_neg, record->n) != 0)
 				return -1;
 			mem_set_int(record, i, is_neg);
 		}
-		if ((record->flags & MEM_Int) == MEM_Int) {
+		if (mem_is_neg_int(record)) {
 			if (type == FIELD_TYPE_UNSIGNED)
 				return -1;
 			return 0;
 		}
 		return 0;
 	case FIELD_TYPE_BOOLEAN:
-		if ((record->flags & MEM_Bool) == MEM_Bool)
+		if (mem_is_bool(record))
 			return 0;
 		return -1;
 	case FIELD_TYPE_NUMBER:
-		if ((record->flags & (MEM_Real | MEM_Int | MEM_UInt)) != 0)
+		if (mem_is_number(record))
 			return 0;
 		return sqlVdbeMemRealify(record);
 	case FIELD_TYPE_DOUBLE:
-		if ((record->flags & MEM_Real) != 0)
+		if (mem_is_double(record))
 			return 0;
 		return sqlVdbeMemRealify(record);
 	case FIELD_TYPE_STRING:
@@ -368,34 +368,24 @@ mem_apply_type(struct Mem *record, enum field_type type)
 		 * an integer or real representation (BLOB and
 		 * NULL do not get converted).
 		 */
-		if ((record->flags & MEM_Str) == 0 &&
-		    (record->flags & (MEM_Real | MEM_Int | MEM_UInt)) != 0)
+		if (!mem_is_string(record) && mem_is_number(record))
 			sqlVdbeMemStringify(record);
-		record->flags &= ~(MEM_Real | MEM_Int | MEM_UInt);
 		return 0;
 	case FIELD_TYPE_VARBINARY:
-		if ((record->flags & MEM_Blob) == 0)
+		if (!mem_is_binary(record))
 			return -1;
 		return 0;
 	case FIELD_TYPE_SCALAR:
 		/* Can't cast MAP and ARRAY to scalar types. */
-		if ((record->flags & MEM_Subtype) != 0 &&
-		    record->subtype == SQL_SUBTYPE_MSGPACK) {
-			assert(mp_typeof(*record->z) == MP_MAP ||
-			       mp_typeof(*record->z) == MP_ARRAY);
+		if (mem_is_map(record) || mem_is_array(record))
 			return -1;
-		}
 		return 0;
 	case FIELD_TYPE_MAP:
-		if ((record->flags & MEM_Subtype) != 0 &&
-		    record->subtype == SQL_SUBTYPE_MSGPACK &&
-		    mp_typeof(*record->z) == MP_MAP)
+		if (mem_is_map(record))
 			return 0;
 		return -1;
 	case FIELD_TYPE_ARRAY:
-		if ((record->flags & MEM_Subtype) != 0 &&
-		    record->subtype == SQL_SUBTYPE_MSGPACK &&
-		    mp_typeof(*record->z) == MP_ARRAY)
+		if (mem_is_array(record))
 			return 0;
 		return -1;
 	case FIELD_TYPE_ANY:
@@ -442,12 +432,12 @@ mem_is_type_compatible(struct Mem *mem, enum field_type type)
 static int
 mem_convert_to_double(struct Mem *mem)
 {
-	if ((mem->flags & MEM_Real) != 0)
+	if (mem_is_double(mem))
 		return 0;
-	if ((mem->flags & (MEM_Int | MEM_UInt)) == 0)
+	if (!mem_is_integer(mem))
 		return -1;
 	double d;
-	if ((mem->flags & MEM_Int) != 0)
+	if (mem_is_neg_int(mem))
 		d = (double)mem->u.i;
 	else
 		d = (double)mem->u.u;
@@ -464,11 +454,11 @@ mem_convert_to_double(struct Mem *mem)
 static int
 mem_convert_to_unsigned(struct Mem *mem)
 {
-	if ((mem->flags & MEM_UInt) != 0)
+	if (mem_is_pos_int(mem))
 		return 0;
-	if ((mem->flags & MEM_Int) != 0)
+	if (mem_is_neg_int(mem))
 		return -1;
-	if ((mem->flags & MEM_Real) == 0)
+	if (!mem_is_double(mem))
 		return -1;
 	double d = mem->u.r;
 	if (d < 0.0 || d >= (double)UINT64_MAX)
@@ -486,9 +476,9 @@ mem_convert_to_unsigned(struct Mem *mem)
 static int
 mem_convert_to_integer(struct Mem *mem)
 {
-	if ((mem->flags & (MEM_UInt | MEM_Int)) != 0)
+	if (mem_is_integer(mem))
 		return 0;
-	if ((mem->flags & MEM_Real) == 0)
+	if (!mem_is_double(mem))
 		return -1;
 	double d = mem->u.r;
 	if (d >= (double)UINT64_MAX || d < (double)INT64_MIN)
@@ -528,16 +518,13 @@ mem_convert_to_numeric(struct Mem *mem, enum field_type type)
  * numeric type, if has one.  Set the pMem->u.r and pMem->u.i fields
  * accordingly.
  */
-static u16 SQL_NOINLINE computeNumericType(Mem *pMem)
+static bool SQL_NOINLINE computeNumericType(Mem *pMem, int64_t *i, bool *is_neg)
 {
-	assert((pMem->flags & (MEM_Int | MEM_UInt | MEM_Real)) == 0);
-	assert((pMem->flags & (MEM_Str|MEM_Blob))!=0);
-	if (sqlAtoF(pMem->z, &pMem->u.r, pMem->n)==0)
-		return 0;
-	bool is_neg;
-	if (sql_atoi64(pMem->z, (int64_t *) &pMem->u.i, &is_neg, pMem->n) == 0)
-		return is_neg ? MEM_Int : MEM_UInt;
-	return MEM_Real;
+	assert(!mem_is_integer(pMem));
+	assert(mem_is_varstring(pMem));
+	if (sql_atoi64(pMem->z, i, is_neg, pMem->n) == 0)
+		return true;
+	return false;
 }
 
 /*
@@ -547,14 +534,17 @@ static u16 SQL_NOINLINE computeNumericType(Mem *pMem)
  * Unlike mem_apply_numeric_type(), this routine does not modify pMem->flags.
  * But it does set pMem->u.r and pMem->u.i appropriately.
  */
-static u16 numericType(Mem *pMem)
+static bool numericType(Mem *pMem, int64_t *i, bool *is_neg)
 {
-	if ((pMem->flags & (MEM_Int | MEM_UInt | MEM_Real)) != 0)
-		return pMem->flags & (MEM_Int | MEM_UInt | MEM_Real);
-	if (pMem->flags & (MEM_Str|MEM_Blob)) {
-		return computeNumericType(pMem);
+	if (mem_is_integer(pMem)) {
+		*i = pMem->u.i;
+		*is_neg = mem_is_neg_int(pMem);
+		return true;
 	}
-	return 0;
+	if (mem_is_varstring(pMem)) {
+		return computeNumericType(pMem, i, is_neg);
+	}
+	return false;
 }
 
 #ifdef SQL_DEBUG
@@ -568,7 +558,7 @@ sqlVdbeMemPrettyPrint(Mem *pMem, char *zBuf)
 	char *zCsr = zBuf;
 	int f = pMem->flags;
 
-	if (f&MEM_Blob) {
+	if (mem_is_binary(pMem)) {
 		int i;
 		char c;
 		if (f & MEM_Dyn) {
@@ -604,7 +594,7 @@ sqlVdbeMemPrettyPrint(Mem *pMem, char *zBuf)
 			zCsr += sqlStrlen30(zCsr);
 		}
 		*zCsr = '\0';
-	} else if (f & MEM_Str) {
+	} else if (mem_is_string(pMem)) {
 		int j, k;
 		zBuf[0] = ' ';
 		if (f & MEM_Dyn) {
@@ -646,19 +636,17 @@ sqlVdbeMemPrettyPrint(Mem *pMem, char *zBuf)
 static void
 memTracePrint(Mem *p)
 {
-	if (p->flags & MEM_Undefined) {
+	if (mem_is_undefined(p)) {
 		printf(" undefined");
-	} else if (p->flags & MEM_Null) {
+	} else if (mem_is_null(p)) {
 		printf(" NULL");
-	} else if ((p->flags & (MEM_Int|MEM_Str))==(MEM_Int|MEM_Str)) {
-		printf(" si:%lld", p->u.i);
-	} else if (p->flags & MEM_Int) {
+	} else if (mem_is_neg_int(p)) {
 		printf(" i:%lld", p->u.i);
-	} else if (p->flags & MEM_UInt) {
+	} else if (mem_is_pos_int(p)) {
 		printf(" u:%"PRIu64"", p->u.u);
-	} else if (p->flags & MEM_Real) {
+	} else if (mem_is_double(p)) {
 		printf(" r:%g", p->u.r);
-	} else if (p->flags & MEM_Bool) {
+	} else if (mem_is_bool(p)) {
 		printf(" bool:%s", SQL_TOKEN_BOOLEAN(p->u.b));
 	} else {
 		char zBuf[200];
@@ -700,8 +688,7 @@ vdbe_prepare_null_out(struct Vdbe *v, int n)
 	assert(n <= (v->nMem + 1 - v->nCursor));
 	struct Mem *out = &v->aMem[n];
 	memAboutToChange(v, out);
-	sqlVdbeMemSetNull(out);
-	out->field_type = field_type_MAX;
+	mem_set_null(out);
 	return out;
 }
 
@@ -731,35 +718,32 @@ char *
 mem_type_to_str(const struct Mem *p)
 {
 	assert(p != NULL);
-	switch (p->flags & MEM_PURE_TYPE_MASK) {
-	case MEM_Null:
+	if (mem_is_null(p))
 		return "NULL";
-	case MEM_Str:
+	if (mem_is_string(p))
 		return "text";
-	case MEM_Int:
+	if (mem_is_neg_int(p))
 		return "integer";
-	case MEM_UInt:
+	if (mem_is_pos_int(p))
 		return "unsigned";
-	case MEM_Real:
+	if (mem_is_double(p))
 		return "real";
-	case MEM_Blob:
+	if (mem_is_binary(p))
 		return "varbinary";
-	case MEM_Bool:
-		return "boolean";
-	default:
-		unreachable();
-	}
+	assert(mem_is_bool(p));
+	return "boolean";
 }
 
 /* Allocate memory for internal VDBE structure on region. */
 static int
 vdbe_mem_alloc_blob_region(struct Mem *vdbe_mem, uint32_t size)
 {
-	vdbe_mem->n = size;
-	vdbe_mem->z = region_alloc(&fiber()->gc, size);
-	if (vdbe_mem->z == NULL)
+	char *buf = region_alloc(&fiber()->gc, size);
+	if (buf == NULL) {
+		diag_set(OutOfMemory, size, "region_alloc", "buf");
 		return -1;
-	vdbe_mem->flags = MEM_Ephem | MEM_Blob;
+	}
+	mem_set_bin(vdbe_mem, buf, size, MEM_Ephem, false);
 	assert(sqlVdbeCheckMemInvariants(vdbe_mem));
 	return 0;
 }
@@ -879,34 +863,13 @@ vdbe_field_ref_fetch(struct vdbe_field_ref *field_ref, uint32_t fieldno,
 	if (vdbe_decode_msgpack_into_mem(data, dest_mem, &dummy) != 0)
 		return -1;
 
-	/*
-	 * MsgPack map, array or extension (unsupported in sql).
-	 * Wrap it in a blob verbatim.
-	 */
-	if (dest_mem->flags == 0) {
-		dest_mem->z = (char *) data;
-		dest_mem->n = vdbe_field_ref_fetch_data(field_ref,
-							fieldno + 1) - data;
-		dest_mem->flags = MEM_Blob | MEM_Ephem | MEM_Subtype;
-		dest_mem->subtype = SQL_SUBTYPE_MSGPACK;
-	}
-	/*
-	 * Add 0 termination (at most for strings)
-	 * Not sure why do we check MEM_Ephem
-	 */
-	if ((dest_mem->flags & (MEM_Ephem | MEM_Str)) ==
-	    (MEM_Ephem | MEM_Str)) {
-		int len = dest_mem->n;
-		if (dest_mem->szMalloc < len + 1) {
-			if (sqlVdbeMemGrow(dest_mem, len + 1, 1) != 0)
-				return -1;
+	if (mem_is_string(dest_mem) && (dest_mem->flags & MEM_Ephem) != 0) {
+		if (dest_mem->n > 0) {
+			mem_copy_str(dest_mem, dest_mem->z, dest_mem->n,
+				     (dest_mem->flags & MEM_Term) != 0);
 		} else {
-			dest_mem->z =
-				memcpy(dest_mem->zMalloc, dest_mem->z, len);
-			dest_mem->flags &= ~MEM_Ephem;
+			mem_set_str(dest_mem, "", 0, MEM_Static, true);
 		}
-		dest_mem->z[len] = 0;
-		dest_mem->flags |= MEM_Term;
 	}
 	UPDATE_MAX_BLOBSIZE(dest_mem);
 	dest_mem->field_type = vdbe_field_ref_fetch_type(field_ref, fieldno);
@@ -1129,9 +1092,9 @@ case OP_Gosub: {            /* jump */
  */
 case OP_Return: {           /* in1 */
 	pIn1 = &aMem[pOp->p1];
-	assert(pIn1->flags==MEM_UInt);
+	assert(mem_is_pos_int(pIn1));
 	pOp = &aOp[pIn1->u.u];
-	pIn1->flags = MEM_Undefined;
+	mem_set_undefined(pIn1);
 	break;
 }
 
@@ -1168,13 +1131,13 @@ case OP_InitCoroutine: {     /* jump */
 case OP_EndCoroutine: {           /* in1 */
 	VdbeOp *pCaller;
 	pIn1 = &aMem[pOp->p1];
-	assert(pIn1->flags == MEM_UInt);
+	assert(mem_is_pos_int(pIn1));
 	assert(pIn1->u.u < (uint64_t) p->nOp);
 	pCaller = &aOp[pIn1->u.u];
 	assert(pCaller->opcode==OP_Yield);
 	assert(pCaller->p2>=0 && pCaller->p2<p->nOp);
 	pOp = &aOp[pCaller->p2 - 1];
-	pIn1->flags = MEM_Undefined;
+	mem_set_undefined(pIn1);
 	break;
 }
 
@@ -1342,10 +1305,8 @@ case OP_String8: {         /* same as TK_STRING, out2 */
  */
 case OP_String: {          /* out2 */
 	assert(pOp->p4.z!=0);
-	pOut = vdbe_prepare_null_out(p, pOp->p2);
-	pOut->flags = MEM_Str|MEM_Static|MEM_Term;
-	pOut->z = pOp->p4.z;
-	pOut->n = pOp->p1;
+	pOut = &p->aMem[pOp->p2];
+	mem_set_str(pOut, pOp->p4.z, pOp->p1, MEM_Static, true);
 	UPDATE_MAX_BLOBSIZE(pOut);
 	break;
 }
@@ -1364,19 +1325,17 @@ case OP_String: {          /* out2 */
  */
 case OP_Null: {           /* out2 */
 	int cnt;
-	u16 nullFlag;
 	pOut = vdbe_prepare_null_out(p, pOp->p2);
 	cnt = pOp->p3-pOp->p2;
 	assert(pOp->p3<=(p->nMem+1 - p->nCursor));
-	pOut->flags = nullFlag = pOp->p1 ? (MEM_Null|MEM_Cleared) : MEM_Null;
-	pOut->n = 0;
+	if (pOp->p1)
+		pOut->flags |= MEM_Cleared;
 	while( cnt>0) {
 		pOut++;
 		memAboutToChange(p, pOut);
-		sqlVdbeMemSetNull(pOut);
-		pOut->flags = nullFlag;
-		pOut->field_type = field_type_MAX;
-		pOut->n = 0;
+		mem_set_null(pOut);
+		if (pOp->p1)
+			pOut->flags |= MEM_Cleared;
 		cnt--;
 	}
 	break;
@@ -1390,12 +1349,13 @@ case OP_Null: {           /* out2 */
  */
 case OP_Blob: {                /* out2 */
 	assert(pOp->p1 <= SQL_MAX_LENGTH);
-	pOut = vdbe_prepare_null_out(p, pOp->p2);
-	sqlVdbeMemSetStr(pOut, pOp->p4.z, pOp->p1, 0, 0);
-	if (pOp->p3!=0) {
-		pOut->flags |= MEM_Subtype;
-		pOut->subtype = pOp->p3;
-	}
+	pOut = &p->aMem[pOp->p2];
+	if (pOp->p3 == 0)
+		mem_set_bin(pOut, pOp->p4.z, pOp->p1, MEM_Static, false);
+	else if (mp_typeof(*pOp->p4.z) == MP_MAP)
+		mem_set_map(pOut, pOp->p4.z, pOp->p1, MEM_Static);
+	else
+		mem_set_array(pOut, pOp->p4.z, pOp->p1, MEM_Static);
 	UPDATE_MAX_BLOBSIZE(pOut);
 	break;
 }
@@ -1547,7 +1507,7 @@ case OP_ResultRow: {
 		assert(memIsValid(&pMem[i]));
 		Deephemeralize(&pMem[i]);
 		assert((pMem[i].flags & MEM_Ephem)==0
-		       || (pMem[i].flags & (MEM_Str|MEM_Blob))==0);
+		       || !mem_is_varstring(&pMem[i]));
 		sqlVdbeMemNulTerminate(&pMem[i]);
 		REGISTER_TRACE(p, pOp->p1+i, &pMem[i]);
 	}
@@ -1587,7 +1547,7 @@ case OP_Concat: {           /* same as TK_CONCAT, in1, in2, out3 */
 	pIn2 = &aMem[pOp->p2];
 	pOut = vdbe_prepare_null_out(p, pOp->p3);
 	assert(pIn1!=pOut);
-	if ((pIn1->flags | pIn2->flags) & MEM_Null) {
+	if (mem_is_null(pIn1) || mem_is_null(pIn2)) {
 		/* Force NULL be of type STRING. */
 		pOut->field_type = FIELD_TYPE_STRING;
 		break;
@@ -1596,10 +1556,8 @@ case OP_Concat: {           /* same as TK_CONCAT, in1, in2, out3 */
 	 * Concatenation operation can be applied only to
 	 * strings and blobs.
 	 */
-	uint32_t str_type_p1 = pIn1->flags & (MEM_Blob | MEM_Str);
-	uint32_t str_type_p2 = pIn2->flags & (MEM_Blob | MEM_Str);
-	if (str_type_p1 == 0 || str_type_p2 == 0) {
-		char *inconsistent_type = str_type_p1 == 0 ?
+	if (!mem_is_varstring(pIn1) || !mem_is_varstring(pIn2)) {
+		char *inconsistent_type = !mem_is_varstring(pIn1) ?
 					  mem_type_to_str(pIn1) :
 					  mem_type_to_str(pIn2);
 		diag_set(ClientError, ER_INCONSISTENT_TYPES,
@@ -1608,7 +1566,7 @@ case OP_Concat: {           /* same as TK_CONCAT, in1, in2, out3 */
 	}
 
 	/* Moreover, both operands must be of the same type. */
-	if (str_type_p1 != str_type_p2) {
+	if (!mems_have_same_type(pIn1, pIn2)) {
 		diag_set(ClientError, ER_INCONSISTENT_TYPES,
 			 mem_type_to_str(pIn2), mem_type_to_str(pIn1));
 		goto abort_due_to_error;
@@ -1619,21 +1577,19 @@ case OP_Concat: {           /* same as TK_CONCAT, in1, in2, out3 */
 	if (nByte>db->aLimit[SQL_LIMIT_LENGTH]) {
 		goto too_big;
 	}
-	if (sqlVdbeMemGrow(pOut, (int)nByte+2, pOut==pIn2)) {
-		goto no_mem;
+	size_t svp = region_used(&fiber()->gc);
+	char *buf = region_alloc(&fiber()->gc, nByte);
+	if (buf == NULL) {
+		diag_set(OutOfMemory, nByte, "region_alloc", "buf");
+		goto abort_due_to_error;
 	}
-	if (pIn1->flags & MEM_Str)
-		MemSetTypeFlag(pOut, MEM_Str);
+	memcpy(buf, pIn2->z, pIn2->n);
+	memcpy(&buf[pIn2->n], pIn1->z, pIn1->n);
+	if (mem_is_binary(pIn1))
+		mem_copy_bin(pOut, buf, nByte, false);
 	else
-		MemSetTypeFlag(pOut, MEM_Blob);
-	if (pOut!=pIn2) {
-		memcpy(pOut->z, pIn2->z, pIn2->n);
-	}
-	memcpy(&pOut->z[pIn2->n], pIn1->z, pIn1->n);
-	pOut->z[nByte]=0;
-	pOut->z[nByte+1] = 0;
-	pOut->flags |= MEM_Term;
-	pOut->n = (int)nByte;
+		mem_copy_str(pOut, buf, nByte, false);
+	region_truncate(&fiber()->gc, svp);
 	UPDATE_MAX_BLOBSIZE(pOut);
 	break;
 }
@@ -1681,27 +1637,25 @@ case OP_Subtract:              /* same as TK_MINUS, in1, in2, out3 */
 case OP_Multiply:              /* same as TK_STAR, in1, in2, out3 */
 case OP_Divide:                /* same as TK_SLASH, in1, in2, out3 */
 case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
-	u32 flags;      /* Combined MEM_* flags from both inputs */
-	u16 type1;      /* Numeric type of left operand */
-	u16 type2;      /* Numeric type of right operand */
+	bool is_in1_int;      /* Numeric type of left operand */
+	bool is_in2_int;      /* Numeric type of right operand */
 	i64 iA;         /* Integer value of left operand */
 	i64 iB;         /* Integer value of right operand */
+	bool is_lhs_neg;
+	bool is_rhs_neg;
 	double rA;      /* Real value of left operand */
 	double rB;      /* Real value of right operand */
 
 	pIn1 = &aMem[pOp->p1];
-	type1 = numericType(pIn1);
+	is_in1_int = numericType(pIn1, (int64_t *)&iA, &is_lhs_neg);
 	pIn2 = &aMem[pOp->p2];
-	type2 = numericType(pIn2);
+	is_in2_int = numericType(pIn2, (int64_t *)&iB, &is_rhs_neg);
 	pOut = vdbe_prepare_null_out(p, pOp->p3);
-	flags = pIn1->flags | pIn2->flags;
-	if ((flags & MEM_Null)!=0) goto arithmetic_result_is_null;
-	if ((type1 & (MEM_Int | MEM_UInt)) != 0 &&
-	    (type2 & (MEM_Int | MEM_UInt)) != 0) {
-		iA = pIn1->u.i;
-		iB = pIn2->u.i;
-		bool is_lhs_neg = pIn1->flags & MEM_Int;
-		bool is_rhs_neg = pIn2->flags & MEM_Int;
+	if (mem_is_null(pIn1) || mem_is_null(pIn2))
+		goto arithmetic_result_is_null;
+	if (is_in1_int && is_in2_int) {
+		bool is_lhs_neg = mem_is_neg_int(pIn1);
+		bool is_rhs_neg = mem_is_neg_int(pIn2);
 		bool is_res_neg;
 		switch( pOp->opcode) {
 		case OP_Add: {
@@ -1752,7 +1706,6 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
 				 sql_value_to_diag_str(pIn2), "numeric");
 			goto abort_due_to_error;
 		}
-		assert(((type1 | type2) & MEM_Real) != 0);
 		switch( pOp->opcode) {
 		case OP_Add:         rB += rA;       break;
 		case OP_Subtract:    rB -= rA;       break;
@@ -1908,7 +1861,7 @@ case OP_BuiltinFunction: {
 		goto abort_due_to_error;
 
 	/* Copy the result of the function into register P3 */
-	if (pOut->flags & (MEM_Str|MEM_Blob)) {
+	if (mem_is_varstring(pOut)) {
 		if (sqlVdbeMemTooBig(pCtx->pOut)) goto too_big;
 	}
 
@@ -1967,7 +1920,7 @@ case OP_FunctionByName: {
 	 * Copy the result of the function invocation into
 	 * register P3.
 	 */
-	if ((pOut->flags & (MEM_Str | MEM_Blob)) != 0)
+	if (mem_is_varstring(pOut))
 		if (sqlVdbeMemTooBig(pOut)) goto too_big;
 
 	REGISTER_TRACE(p, pOp->p3, pOut);
@@ -2017,7 +1970,7 @@ case OP_ShiftRight: {           /* same as TK_RSHIFT, in1, in2, out3 */
 	pIn1 = &aMem[pOp->p1];
 	pIn2 = &aMem[pOp->p2];
 	pOut = vdbe_prepare_null_out(p, pOp->p3);
-	if ((pIn1->flags | pIn2->flags) & MEM_Null) {
+	if (mem_is_null(pIn1) || mem_is_null(pIn2)) {
 		/* Force NULL be of type INTEGER. */
 		pOut->field_type = FIELD_TYPE_INTEGER;
 		break;
@@ -2076,7 +2029,7 @@ case OP_ShiftRight: {           /* same as TK_RSHIFT, in1, in2, out3 */
 case OP_AddImm: {            /* in1 */
 	pIn1 = &aMem[pOp->p1];
 	memAboutToChange(p, pIn1);
-	assert((pIn1->flags & MEM_UInt) != 0 && pOp->p2 >= 0);
+	assert(mem_is_pos_int(pIn1) && pOp->p2 >= 0);
 	pIn1->u.u += pOp->p2;
 	break;
 }
@@ -2090,9 +2043,9 @@ case OP_AddImm: {            /* in1 */
  */
 case OP_MustBeInt: {            /* jump, in1 */
 	pIn1 = &aMem[pOp->p1];
-	if ((pIn1->flags & (MEM_Int | MEM_UInt)) == 0) {
+	if (!mem_is_integer(pIn1)) {
 		mem_apply_type(pIn1, FIELD_TYPE_INTEGER);
-		if ((pIn1->flags & (MEM_Int | MEM_UInt)) == 0) {
+		if (!mem_is_integer(pIn1)) {
 			if (pOp->p2==0) {
 				diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 					 sql_value_to_diag_str(pIn1), "integer");
@@ -2116,7 +2069,7 @@ case OP_MustBeInt: {            /* jump, in1 */
  */
 case OP_Realify: {                  /* in1 */
 	pIn1 = &aMem[pOp->p1];
-	if ((pIn1->flags & (MEM_Int | MEM_UInt)) != 0) {
+	if (mem_is_integer(pIn1)) {
 		sqlVdbeMemRealify(pIn1);
 	}
 	break;
@@ -2248,16 +2201,10 @@ case OP_Le:               /* same as TK_LE, jump, in1, in3 */
 case OP_Gt:               /* same as TK_GT, jump, in1, in3 */
 case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 	int res, res2;      /* Result of the comparison of pIn1 against pIn3 */
-	u32 flags1;         /* Copy of initial value of pIn1->flags */
-	u32 flags3;         /* Copy of initial value of pIn3->flags */
 
 	pIn1 = &aMem[pOp->p1];
 	pIn3 = &aMem[pOp->p3];
-	flags1 = pIn1->flags;
-	flags3 = pIn3->flags;
-	enum field_type ft_p1 = pIn1->field_type;
-	enum field_type ft_p3 = pIn3->field_type;
-	if ((flags1 | flags3)&MEM_Null) {
+	if (mem_is_null(pIn1) || mem_is_null(pIn3)) {
 		/* One or both operands are NULL */
 		if (pOp->p5 & SQL_NULLEQ) {
 			/* If SQL_NULLEQ is set (which will only happen if the operator is
@@ -2265,10 +2212,10 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 			 * or not both operands are null.
 			 */
 			assert(pOp->opcode==OP_Eq || pOp->opcode==OP_Ne);
-			assert((flags1 & MEM_Cleared)==0);
+			assert((pIn1->flags & MEM_Cleared)==0);
 			assert((pOp->p5 & SQL_JUMPIFNULL)==0);
-			if ((flags1&flags3&MEM_Null)!=0
-			    && (flags3&MEM_Cleared)==0
+			if (mem_is_null(pIn1) && mem_is_null(pIn3)
+			    && (pIn3->flags & MEM_Cleared)==0
 				) {
 				res = 0;  /* Operands are equal */
 			} else {
@@ -2291,22 +2238,22 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 			}
 			break;
 		}
-	} else if (((flags1 | flags3) & MEM_Bool) != 0 ||
-		   ((flags1 | flags3) & MEM_Blob) != 0) {
+	} else if (mem_is_bool(pIn1) || mem_is_bool(pIn3) ||
+		   mem_is_binary(pIn1) || mem_is_binary(pIn3)) {
 		/*
 		 * If one of values is of type BOOLEAN or VARBINARY,
 		 * then the second one must be of the same type as
 		 * well. Otherwise an error is raised.
 		 */
-		int type_arg1 = flags1 & (MEM_Bool | MEM_Blob);
-		int type_arg3 = flags3 & (MEM_Bool | MEM_Blob);
-		if (type_arg1 != type_arg3) {
-			char *inconsistent_type = type_arg1 != 0 ?
+		if (!mems_have_same_type(pIn1, pIn3)) {
+			char *inconsistent_type = mem_is_bool(pIn1) ||
+						  mem_is_binary(pIn1) ?
 						  mem_type_to_str(pIn3) :
 						  mem_type_to_str(pIn1);
-			char *expected_type     = type_arg1 != 0 ?
-						  mem_type_to_str(pIn1) :
-						  mem_type_to_str(pIn3);
+			char *expected_type = mem_is_bool(pIn1) ||
+					      mem_is_binary(pIn1) ?
+					      mem_type_to_str(pIn1) :
+					      mem_type_to_str(pIn3);
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 				 inconsistent_type, expected_type);
 			goto abort_due_to_error;
@@ -2314,18 +2261,25 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 		res = sqlMemCompare(pIn3, pIn1, NULL);
 	} else {
 		enum field_type type = pOp->p5 & FIELD_TYPE_MASK;
+		struct Mem tmp_mem1, tmp_mem2, *mem1, *mem2;
+		mem1 = pIn1;
+		mem2 = pIn3;
 		if (sql_type_is_numeric(type)) {
-			if ((flags1 | flags3)&MEM_Str) {
-				if ((flags1 & MEM_Str) == MEM_Str) {
-					mem_apply_numeric_type(pIn1);
-					testcase( flags3!=pIn3->flags); /* Possible if pIn1==pIn3 */
-					flags3 = pIn3->flags;
+			if (mem_is_string(mem1) || mem_is_string(mem2)) {
+				if (mem_is_string(mem1)) {
+					mem_init(&tmp_mem1);
+					memcpy(&tmp_mem1, mem1, sizeof(*mem1));
+					mem1 = &tmp_mem1;
+					mem_apply_numeric_type(mem1);
 				}
-				if ((flags3 & MEM_Str) == MEM_Str) {
-					if (mem_apply_numeric_type(pIn3) != 0) {
+				if (mem_is_string(mem2)) {
+					mem_init(&tmp_mem2);
+					memcpy(&tmp_mem2, mem2, sizeof(*mem2));
+					mem2 = &tmp_mem2;
+					if (mem_apply_numeric_type(mem2) != 0) {
 						diag_set(ClientError,
 							 ER_SQL_TYPE_MISMATCH,
-							 sql_value_to_diag_str(pIn3),
+							 sql_value_to_diag_str(mem2),
 							 "numeric");
 						goto abort_due_to_error;
 					}
@@ -2335,27 +2289,29 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 			/* Handle the common case of integer comparison here, as an
 			 * optimization, to avoid a call to sqlMemCompare()
 			 */
-			if ((pIn1->flags & pIn3->flags & (MEM_Int | MEM_UInt)) != 0) {
-				if ((pIn1->flags & pIn3->flags & MEM_Int) != 0) {
-					if (pIn3->u.i > pIn1->u.i)
+			if (mem_is_integer(mem1) && mem_is_integer(mem2)) {
+				if (mem_is_neg_int(mem1) &&
+				    mem_is_neg_int(mem2)) {
+					if (mem2->u.i > mem1->u.i)
 						res = +1;
-					else if (pIn3->u.i < pIn1->u.i)
+					else if (mem2->u.i < mem1->u.i)
 						res = -1;
 					else
 						res = 0;
 					goto compare_op;
 				}
-				if ((pIn1->flags & pIn3->flags & MEM_UInt) != 0) {
-					if (pIn3->u.u > pIn1->u.u)
+				if (mem_is_pos_int(mem1) &&
+				    mem_is_pos_int(mem2)) {
+					if (mem2->u.u > mem1->u.u)
 						res = +1;
-					else if (pIn3->u.u < pIn1->u.u)
+					else if (mem2->u.u < mem1->u.u)
 						res = -1;
 					else
 						res = 0;
 					goto compare_op;
 				}
-				if ((pIn1->flags & MEM_UInt) != 0 &&
-				    (pIn3->flags & MEM_Int) != 0) {
+				if (mem_is_pos_int(mem1) &&
+				    mem_is_neg_int(mem2)) {
 					res = -1;
 					goto compare_op;
 				}
@@ -2363,26 +2319,22 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 				goto compare_op;
 			}
 		} else if (type == FIELD_TYPE_STRING) {
-			if ((flags1 & MEM_Str) == 0 &&
-			    (flags1 & (MEM_Int | MEM_UInt | MEM_Real)) != 0) {
-				testcase( pIn1->flags & MEM_Int);
-				testcase( pIn1->flags & MEM_Real);
-				sqlVdbeMemStringify(pIn1);
-				testcase( (flags1&MEM_Dyn) != (pIn1->flags&MEM_Dyn));
-				flags1 = (pIn1->flags & ~MEM_TypeMask) | (flags1 & MEM_TypeMask);
-				assert(pIn1!=pIn3);
+			if (mem_is_number(mem1)) {
+				mem_init(&tmp_mem1);
+				memcpy(&tmp_mem1, mem1, sizeof(*mem1));
+				mem1 = &tmp_mem1;
+				sqlVdbeMemStringify(mem1);
+				assert(mem1!=mem2);
 			}
-			if ((flags3 & MEM_Str) == 0 &&
-			    (flags3 & (MEM_Int | MEM_UInt | MEM_Real)) != 0) {
-				testcase( pIn3->flags & MEM_Int);
-				testcase( pIn3->flags & MEM_Real);
-				sqlVdbeMemStringify(pIn3);
-				testcase( (flags3&MEM_Dyn) != (pIn3->flags&MEM_Dyn));
-				flags3 = (pIn3->flags & ~MEM_TypeMask) | (flags3 & MEM_TypeMask);
+			if (mem_is_number(mem2)) {
+				mem_init(&tmp_mem2);
+				memcpy(&tmp_mem2, mem2, sizeof(*mem2));
+				mem2 = &tmp_mem2;
+				sqlVdbeMemStringify(mem2);
 			}
 		}
 		assert(pOp->p4type==P4_COLLSEQ || pOp->p4.pColl==0);
-		res = sqlMemCompare(pIn3, pIn1, pOp->p4.pColl);
+		res = sqlMemCompare(mem2, mem1, pOp->p4.pColl);
 	}
 			compare_op:
 	switch( pOp->opcode) {
@@ -2394,14 +2346,6 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 	default:       res2 = res>=0;     break;
 	}
 
-	/* Undo any changes made by mem_apply_type() to the input registers. */
-	assert((pIn1->flags & MEM_Dyn) == (flags1 & MEM_Dyn));
-	pIn1->flags = flags1;
-	pIn1->field_type = ft_p1;
-	assert((pIn3->flags & MEM_Dyn) == (flags3 & MEM_Dyn));
-	pIn3->flags = flags3;
-	pIn3->field_type = ft_p3;
-
 	if (pOp->p5 & SQL_STOREP2) {
 		iCompare = res;
 		res2 = res2!=0;  /* For this path res2 must be exactly 0 or 1 */
@@ -2585,9 +2529,9 @@ case OP_Or: {             /* same as TK_OR, in1, in2, out3 */
 	int v2;    /* Right operand: 0==FALSE, 1==TRUE, 2==UNKNOWN or NULL */
 
 	pIn1 = &aMem[pOp->p1];
-	if (pIn1->flags & MEM_Null) {
+	if (mem_is_null(pIn1)) {
 		v1 = 2;
-	} else if ((pIn1->flags & MEM_Bool) != 0) {
+	} else if (mem_is_bool(pIn1)) {
 		v1 = pIn1->u.b;
 	} else {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
@@ -2595,9 +2539,9 @@ case OP_Or: {             /* same as TK_OR, in1, in2, out3 */
 		goto abort_due_to_error;
 	}
 	pIn2 = &aMem[pOp->p2];
-	if (pIn2->flags & MEM_Null) {
+	if (mem_is_null(pIn2)) {
 		v2 = 2;
-	} else if ((pIn2->flags & MEM_Bool) != 0) {
+	} else if (mem_is_bool(pIn2)) {
 		v2 = pIn2->u.b;
 	} else {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
@@ -2628,8 +2572,8 @@ case OP_Not: {                /* same as TK_NOT, in1, out2 */
 	pIn1 = &aMem[pOp->p1];
 	pOut = vdbe_prepare_null_out(p, pOp->p2);
 	pOut->field_type = FIELD_TYPE_BOOLEAN;
-	if ((pIn1->flags & MEM_Null)==0) {
-		if ((pIn1->flags & MEM_Bool) == 0) {
+	if (!mem_is_null(pIn1)) {
+		if (!mem_is_bool(pIn1)) {
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 				 sql_value_to_diag_str(pIn1), "boolean");
 			goto abort_due_to_error;
@@ -2651,7 +2595,7 @@ case OP_BitNot: {             /* same as TK_BITNOT, in1, out2 */
 	pOut = vdbe_prepare_null_out(p, pOp->p2);
 	/* Force NULL be of type INTEGER. */
 	pOut->field_type = FIELD_TYPE_INTEGER;
-	if ((pIn1->flags & MEM_Null)==0) {
+	if (!mem_is_null(pIn1)) {
 		int64_t i;
 		bool is_neg;
 		if (sqlVdbeIntValue(pIn1, &i, &is_neg) != 0) {
@@ -2696,9 +2640,9 @@ case OP_If:                 /* jump, in1 */
 case OP_IfNot: {            /* jump, in1 */
 	int c;
 	pIn1 = &aMem[pOp->p1];
-	if (pIn1->flags & MEM_Null) {
+	if (mem_is_null(pIn1)) {
 		c = pOp->p3;
-	} else if ((pIn1->flags & MEM_Bool) != 0) {
+	} else if (mem_is_bool(pIn1)) {
 		c = pOp->opcode == OP_IfNot ? ! pIn1->u.b : pIn1->u.b;
 	} else {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
@@ -2719,8 +2663,8 @@ case OP_IfNot: {            /* jump, in1 */
  */
 case OP_IsNull: {            /* same as TK_ISNULL, jump, in1 */
 	pIn1 = &aMem[pOp->p1];
-	VdbeBranchTaken( (pIn1->flags & MEM_Null)!=0, 2);
-	if ((pIn1->flags & MEM_Null)!=0) {
+	VdbeBranchTaken(mem_is_null(pIn1), 2);
+	if (mem_is_null(pIn1)) {
 		goto jump_to_p2;
 	}
 	break;
@@ -2733,8 +2677,8 @@ case OP_IsNull: {            /* same as TK_ISNULL, jump, in1 */
  */
 case OP_NotNull: {            /* same as TK_NOTNULL, jump, in1 */
 	pIn1 = &aMem[pOp->p1];
-	VdbeBranchTaken( (pIn1->flags & MEM_Null)==0, 2);
-	if ((pIn1->flags & MEM_Null)==0) {
+	VdbeBranchTaken(!mem_is_null(pIn1), 2);
+	if (!mem_is_null(pIn1)) {
 		goto jump_to_p2;
 	}
 	break;
@@ -2788,7 +2732,7 @@ case OP_Column: {
 			if (pC->eCurType==CURTYPE_PSEUDO) {
 				assert(pC->uc.pseudoTableReg>0);
 				pReg = &aMem[pC->uc.pseudoTableReg];
-				assert(pReg->flags & MEM_Blob);
+				assert(mem_is_binary(pReg));
 				assert(memIsValid(pReg));
 				vdbe_field_ref_prepare_data(&pC->field_ref,
 							    pReg->z, pReg->n);
@@ -2817,7 +2761,7 @@ case OP_Column: {
 	if (vdbe_field_ref_fetch(&pC->field_ref, p2, pDest) != 0)
 		goto abort_due_to_error;
 
-	if ((pDest->flags & MEM_Null) &&
+	if (mem_is_null(pDest) &&
 	    (uint32_t) p2  >= pC->field_ref.field_count &&
 	    default_val_mem != NULL) {
 		sqlVdbeMemShallowCopy(pDest, default_val_mem, MEM_Static);
@@ -2968,21 +2912,15 @@ case OP_MakeRecord: {
 	 * routine.
 	 */
 	if (bIsEphemeral) {
-		if (sqlVdbeMemClearAndResize(pOut, tuple_size) != 0)
+		if (mem_copy_bin(pOut, tuple, tuple_size, false) != 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
 		 * to be passed to Tarantool. Before that, make
 		 * sure previously allocated memory has gone.
 		 */
-		sqlVdbeMemRelease(pOut);
-		pOut->flags = MEM_Blob | MEM_Ephem;
-		pOut->n = tuple_size;
-		pOut->z = tuple;
+		mem_set_bin(pOut, tuple, tuple_size, MEM_Ephem, false);
 	}
 	assert(sqlVdbeCheckMemInvariants(pOut));
 	assert(pOp->p3>0 && pOp->p3<=(p->nMem+1 - p->nCursor));
@@ -3260,8 +3198,7 @@ case OP_OpenTEphemeral: {
 
 	if (space == NULL)
 		goto abort_due_to_error;
-	aMem[pOp->p1].u.p = space;
-	aMem[pOp->p1].flags = MEM_Ptr;
+	mem_set_ptr(&aMem[pOp->p1], (void *)space);
 	break;
 }
 
@@ -3490,18 +3427,18 @@ case OP_SeekGT: {       /* jump, in3 */
 		 * the seek, so convert it.
 		 */
 		pIn3 = &aMem[int_field];
-		if ((pIn3->flags & MEM_Null) != 0)
+		if (mem_is_null(pIn3))
 			goto skip_truncate;
-		if ((pIn3->flags & MEM_Str) != 0)
+		if (mem_is_string(pIn3))
 			mem_apply_numeric_type(pIn3);
 		int64_t i;
-		if ((pIn3->flags & MEM_Int) == MEM_Int) {
+		if (mem_is_neg_int(pIn3)) {
 			i = pIn3->u.i;
 			is_neg = true;
-		} else if ((pIn3->flags & MEM_UInt) == MEM_UInt) {
+		} else if (mem_is_pos_int(pIn3)) {
 			i = pIn3->u.u;
 			is_neg = false;
-		} else if ((pIn3->flags & MEM_Real) == MEM_Real) {
+		} else if (mem_is_double(pIn3)) {
 			if (pIn3->u.r > (double)INT64_MAX)
 				i = INT64_MAX;
 			else if (pIn3->u.r < (double)INT64_MIN)
@@ -3519,8 +3456,8 @@ case OP_SeekGT: {       /* jump, in3 */
 		/* If the P3 value could not be converted into an integer without
 		 * loss of information, then special processing is required...
 		 */
-		if ((pIn3->flags & (MEM_Int | MEM_UInt)) == 0) {
-			if ((pIn3->flags & MEM_Real)==0) {
+		if (!mem_is_integer(pIn3)) {
+			if (!mem_is_double(pIn3)) {
 				/* If the P3 value cannot be converted into any kind of a number,
 				 * then the seek is not possible, so jump to P2
 				 */
@@ -3736,7 +3673,7 @@ case OP_Found: {        /* jump, in3 */
 	} else {
 		pFree = pIdxKey = sqlVdbeAllocUnpackedRecord(db, pC->key_def);
 		if (pIdxKey==0) goto no_mem;
-		assert(pIn3->flags & MEM_Blob );
+		assert(mem_is_binary(pIn3));
 		(void)ExpandBlob(pIn3);
 		sqlVdbeRecordUnpackMsgpack(pC->key_def,
 					       pIn3->z, pIdxKey);
@@ -3750,7 +3687,7 @@ case OP_Found: {        /* jump, in3 */
 		 * conflict
 		 */
 		for(ii=0; ii<pIdxKey->nField; ii++) {
-			if (pIdxKey->aMem[ii].flags & MEM_Null) {
+			if (mem_is_null(&pIdxKey->aMem[ii])) {
 				takeJump = 1;
 				break;
 			}
@@ -3860,14 +3797,14 @@ case OP_FCopy: {     /* out2 */
 		pIn1 = &aMem[pOp->p1];
 	}
 
-	if ((pOp->p3 & OPFLAG_NOOP_IF_NULL) && (pIn1->flags & MEM_Null)) {
+	if ((pOp->p3 & OPFLAG_NOOP_IF_NULL) && mem_is_null(pIn1)) {
 		pOut = vdbe_prepare_null_out(p, pOp->p2);
 	} else {
 		assert(memIsValid(pIn1));
-		assert((pIn1->flags & (MEM_Int | MEM_UInt)) != 0);
+		assert(mem_is_integer(pIn1));
 
 		pOut = vdbe_prepare_null_out(p, pOp->p2);
-		mem_set_int(pOut, pIn1->u.i, pIn1->flags == MEM_Int);
+		mem_set_int(pOut, pIn1->u.i, mem_is_neg_int(pIn1));
 		pOut->field_type = pIn1->field_type;
 	}
 	break;
@@ -3999,7 +3936,7 @@ case OP_SorterData: {
 	assert(isSorter(pC));
 	if (sqlVdbeSorterRowkey(pC, pOut) != 0)
 		goto abort_due_to_error;
-	assert(pOut->flags & MEM_Blob);
+	assert(mem_is_binary(pOut));
 	assert(pOp->p1>=0 && pOp->p1<p->nCursor);
 	p->apCsr[pOp->p3]->cacheStatus = CACHE_STALE;
 	break;
@@ -4357,7 +4294,7 @@ case OP_SorterInsert: {      /* in2 */
 	assert(cursor != NULL);
 	assert(isSorter(cursor));
 	pIn2 = &aMem[pOp->p2];
-	assert((pIn2->flags & MEM_Blob) != 0);
+	assert(mem_is_binary(pIn2));
 	if (ExpandBlob(pIn2) != 0 ||
 	    sqlVdbeSorterWrite(cursor, pIn2) != 0)
 		goto abort_due_to_error;
@@ -4391,7 +4328,7 @@ case OP_SorterInsert: {      /* in2 */
 case OP_IdxReplace:
 case OP_IdxInsert: {
 	pIn2 = &aMem[pOp->p1];
-	assert((pIn2->flags & MEM_Blob) != 0);
+	assert(mem_is_binary(pIn2));
 	if (ExpandBlob(pIn2) != 0)
 		goto abort_due_to_error;
 	struct space *space;
@@ -4436,7 +4373,7 @@ case OP_IdxInsert: {
 	}
 	if ((pOp->p5 & OPFLAG_NCHANGE) != 0)
 		p->nChange++;
-	if (pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
+	if (pOp->p3 > 0 && mem_is_null(&aMem[pOp->p3])) {
 		assert(space->sequence != NULL);
 		int64_t value;
 		if (sequence_get_value(space->sequence, &value) != 0)
@@ -4482,10 +4419,10 @@ case OP_Update: {
 	assert(pOp->p4type == P4_SPACEPTR);
 
 	struct Mem *key_mem = &aMem[pOp->p2];
-	assert((key_mem->flags & MEM_Blob) != 0);
+	assert(mem_is_binary(key_mem));
 
 	struct Mem *upd_fields_mem = &aMem[pOp->p3];
-	assert((upd_fields_mem->flags & MEM_Blob) != 0);
+	assert(mem_is_binary(upd_fields_mem));
 	uint32_t *upd_fields = (uint32_t *)upd_fields_mem->z;
 	uint32_t upd_fields_cnt = upd_fields_mem->n / sizeof(uint32_t);
 
@@ -4914,7 +4851,7 @@ case OP_Program: {        /* jump */
 	 * the trigger program. If this trigger has been fired before, then pRt
 	 * is already allocated. Otherwise, it must be initialized.
 	 */
-	if ((pRt->flags&MEM_Frame)==0) {
+	if (!mem_is_frame(pRt)) {
 		/* SubProgram.nMem is set to the number of memory cells used by the
 		 * program stored in SubProgram.aOp. As well as these, one memory
 		 * cell is required for each cursor used by the program. Set local
@@ -4930,9 +4867,7 @@ case OP_Program: {        /* jump */
 		if (!pFrame) {
 			goto no_mem;
 		}
-		sqlVdbeMemRelease(pRt);
-		pRt->flags = MEM_Frame;
-		pRt->u.pFrame = pFrame;
+		mem_set_frame(pRt, pFrame);
 
 		pFrame->v = p;
 		pFrame->nChildMem = nMem;
@@ -4948,7 +4883,7 @@ case OP_Program: {        /* jump */
 
 		pEnd = &VdbeFrameMem(pFrame)[pFrame->nChildMem];
 		for(pMem=VdbeFrameMem(pFrame); pMem!=pEnd; pMem++) {
-			pMem->flags = MEM_Undefined;
+			mem_set_undefined(pMem);
 			pMem->db = db;
 		}
 	} else {
@@ -5054,8 +4989,8 @@ case OP_FkIfZero: {         /* jump */
  */
 case OP_IfPos: {        /* jump, in1 */
 	pIn1 = &aMem[pOp->p1];
-	assert((pIn1->flags & (MEM_Int | MEM_UInt)) != 0);
-	if ((pIn1->flags & MEM_UInt) != 0 && pIn1->u.u != 0) {
+	assert(mem_is_integer(pIn1));
+	if (mem_is_pos_int(pIn1) && pIn1->u.u != 0) {
 		assert(pOp->p3 >= 0);
 		uint64_t res = pIn1->u.u - (uint64_t) pOp->p3;
 		/*
@@ -5089,8 +5024,8 @@ case OP_OffsetLimit: {    /* in1, out2, in3 */
 	pIn3 = &aMem[pOp->p3];
 	pOut = vdbe_prepare_null_out(p, pOp->p2);
 
-	assert((pIn1->flags & MEM_UInt) != 0);
-	assert((pIn3->flags & MEM_UInt) != 0);
+	assert(mem_is_pos_int(pIn1));
+	assert(mem_is_pos_int(pIn3));
 	uint64_t x = pIn1->u.u;
 	uint64_t rhs = pIn3->u.u;
 	bool unused;
@@ -5113,7 +5048,7 @@ case OP_OffsetLimit: {    /* in1, out2, in3 */
  */
 case OP_IfNotZero: {        /* jump, in1 */
 	pIn1 = &aMem[pOp->p1];
-	assert((pIn1->flags & MEM_UInt) != 0);
+	assert(mem_is_pos_int(pIn1));
 	if (pIn1->u.u > 0) {
 		pIn1->u.u--;
 		goto jump_to_p2;
@@ -5129,7 +5064,7 @@ case OP_IfNotZero: {        /* jump, in1 */
  */
 case OP_DecrJumpZero: {      /* jump, in1 */
 	pIn1 = &aMem[pOp->p1];
-	assert((pIn1->flags & MEM_UInt) != 0);
+	assert(mem_is_pos_int(pIn1));
 	if (pIn1->u.u > 0)
 		pIn1->u.u--;
 	if (pIn1->u.u == 0) goto jump_to_p2;
@@ -5215,7 +5150,7 @@ case OP_AggStep: {
 #endif
 
 	pMem->n++;
-	sqlVdbeMemInit(&t, db, MEM_Null);
+	mem_init(&t);
 	pCtx->pOut = &t;
 	pCtx->is_aborted = false;
 	pCtx->skipFlag = 0;
@@ -5226,7 +5161,7 @@ case OP_AggStep: {
 		sqlVdbeMemRelease(&t);
 		goto abort_due_to_error;
 	}
-	assert(t.flags==MEM_Null);
+	assert(mem_is_null(&t));
 	if (pCtx->skipFlag) {
 		assert(pOp[-1].opcode==OP_CollSeq);
 		i = pOp[-1].p1;
@@ -5252,7 +5187,7 @@ case OP_AggFinal: {
 	Mem *pMem;
 	assert(pOp->p1>0 && pOp->p1<=(p->nMem+1 - p->nCursor));
 	pMem = &aMem[pOp->p1];
-	assert((pMem->flags & ~(MEM_Null|MEM_Agg))==0);
+	assert(mem_is_null(pMem) || (pMem->flags & MEM_Agg) != 0);
 	if (sql_vdbemem_finalize(pMem, pOp->p4.func) != 0)
 		goto abort_due_to_error;
 	UPDATE_MAX_BLOBSIZE(pMem);
@@ -5354,11 +5289,11 @@ case OP_Init: {          /* jump */
  */
 case OP_IncMaxid: {
 	assert(pOp->p1 > 0);
-	pOut = vdbe_prepare_null_out(p, pOp->p1);
-
-	if (tarantoolsqlIncrementMaxid(&pOut->u.u) != 0)
+	pOut = &p->aMem[pOp->p1];
+	uint64_t id;
+	if (tarantoolsqlIncrementMaxid(&id) != 0)
 		goto abort_due_to_error;
-	pOut->flags = MEM_UInt;
+	mem_set_u64(pOut, id);
 	break;
 }
 
@@ -5379,7 +5314,7 @@ case OP_SetSession: {
 	struct session_setting *setting = &session_settings[sid];
 	switch (setting->field_type) {
 	case FIELD_TYPE_BOOLEAN: {
-		if ((pIn1->flags & MEM_Bool) == 0)
+		if (!mem_is_bool(pIn1))
 			goto invalid_type;
 		bool value = pIn1->u.b;
 		size_t size = mp_sizeof_bool(value);
@@ -5390,7 +5325,7 @@ case OP_SetSession: {
 		break;
 	}
 	case FIELD_TYPE_STRING: {
-		if ((pIn1->flags & MEM_Str) == 0)
+		if (!mem_is_string(pIn1))
 			goto invalid_type;
 		const char *str = pIn1->z;
 		uint32_t size = mp_sizeof_str(pIn1->n);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 7205f1af3..a80a7b511 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -484,41 +484,455 @@ void sqlVdbeMemMove(Mem *, Mem *);
 int sqlVdbeMemNulTerminate(Mem *);
 int sqlVdbeMemSetStr(Mem *, const char *, int, u8, void (*)(void *));
 
+/**
+ * Memory cell mem contains the context of an aggregate function.
+ * This routine calls the finalize method for that function. The
+ * result of the aggregate is stored back into mem.
+ *
+ * Returns -1 if the finalizer reports an error. 0 otherwise.
+ */
+int
+sql_vdbemem_finalize(struct Mem *mem, struct func *func);
+
+/*
+ * If the memory cell contains a value that must be freed by
+ * invoking the external callback in Mem.xDel, then this routine
+ * will free that value.  It also sets Mem.flags to MEM_Null.
+ */
 void
-mem_set_bool(struct Mem *mem, bool value);
+vdbeMemClearExternAndSetNull(Mem * p);
+
+/*
+ * Change the pMem->zMalloc allocation to be at least szNew bytes.
+ * If pMem->zMalloc already meets or exceeds the requested size, this
+ * routine is a no-op.
+ *
+ * Any prior string or blob content in the pMem object may be discarded.
+ * The pMem->xDel destructor is called, if it exists.  Though MEM_Str
+ * and MEM_Blob values may be discarded, MEM_Int, MEM_Real, and MEM_Null
+ * values are preserved.
+ *
+ * Return 0 on success or -1 if unable to complete the resizing.
+ */
+int
+sqlVdbeMemClearAndResize(Mem * pMem, int n);
+
+/**
+ * Initialize a new mem. After initializing the mem will hold a NULL value.
+ *
+ * @param mem VDBE memory register to initialize.
+ */
+static inline void
+mem_init(struct Mem *mem)
+{
+	memset(mem, 0, sizeof(*mem));
+	mem->db = sql_get();
+	mem->flags = MEM_Null;
+	mem->field_type = field_type_MAX;
+}
+
+/**
+ * Set VDBE memory register as NULL.
+ *
+ * @param mem VDBE memory register to update.
+ */
+static inline void
+mem_set_null(struct Mem *mem)
+{
+	if (unlikely(VdbeMemDynamic(mem)))
+		vdbeMemClearExternAndSetNull(mem);
+	else
+		mem->flags = MEM_Null;
+	mem->field_type = field_type_MAX;
+}
+
+/**
+ * Set VDBE memory register as Undefined.
+ *
+ * @param mem VDBE memory register to update.
+ */
+static inline void
+mem_set_undefined(struct Mem *mem)
+{
+	if (unlikely(VdbeMemDynamic(mem)))
+		vdbeMemClearExternAndSetNull(mem);
+	mem->flags = MEM_Undefined;
+	mem->field_type = field_type_MAX;
+}
 
 /**
  * Set VDBE memory register with given pointer as a data.
+ *
  * @param mem VDBE memory register to update.
- * @param ptr Pointer to use.
+ * @param ptr Pointer to set as the value.
  */
-void
-mem_set_ptr(struct Mem *mem, void *ptr);
+static inline void
+mem_set_ptr(struct Mem *mem, void *ptr)
+{
+	if (unlikely(VdbeMemDynamic(mem)))
+		vdbeMemClearExternAndSetNull(mem);
+	mem->flags = MEM_Ptr;
+	mem->u.p = ptr;
+	mem->field_type = FIELD_TYPE_BOOLEAN;
+}
 
 /**
- * Set integer value. Depending on its sign MEM_Int (in case
- * of negative value) or MEM_UInt flag is set.
+ * Set VDBE memory register as frame.
+ *
+ * @param mem VDBE memory register to update.
+ * @param frame Frame to set as the value.
  */
-void
-mem_set_i64(struct Mem *mem, int64_t value);
+static inline void
+mem_set_frame(struct Mem *mem, struct VdbeFrame *frame)
+{
+	if (unlikely(VdbeMemDynamic(mem)))
+		vdbeMemClearExternAndSetNull(mem);
+	mem->u.pFrame = frame;
+	mem->flags = MEM_Frame;
+	mem->field_type = field_type_MAX;
+}
 
-/** Set unsigned value and MEM_UInt flag. */
-void
-mem_set_u64(struct Mem *mem, uint64_t value);
+/**
+ * Set integer value. Depending on its sign MEM_Int (in case of negative value)
+ * or MEM_UInt flag is set.
+ *
+ * @param mem VDBE memory register to update.
+ * @param value Integer to set as the value.
+ */
+static inline void
+mem_set_i64(struct Mem *mem, int64_t value)
+{
+	if (unlikely(VdbeMemDynamic(mem)))
+		vdbeMemClearExternAndSetNull(mem);
+	mem->u.i = value;
+	mem->flags = value < 0 ? MEM_Int : MEM_UInt;
+	mem->field_type = FIELD_TYPE_INTEGER;
+}
 
 /**
- * Set integer value. According to is_neg flag value is considered
- * to be signed or unsigned.
+ * Set unsigned value.
+ *
+ * @param mem VDBE memory register to update.
+ * @param value Unsigned to set as the value.
  */
-void
-mem_set_int(struct Mem *mem, int64_t value, bool is_neg);
+static inline void
+mem_set_u64(struct Mem *mem, uint64_t value)
+{
+	if (unlikely(VdbeMemDynamic(mem)))
+		vdbeMemClearExternAndSetNull(mem);
+	mem->u.u = value;
+	mem->flags = MEM_UInt;
+	mem->field_type = FIELD_TYPE_UNSIGNED;
+}
 
-/** Set double value and MEM_Real flag. */
-void
-mem_set_double(struct Mem *mem, double value);
+/**
+ * Set integer value. According to is_neg flag value is considered to be signed
+ * or unsigned.
+ *
+ * @param mem VDBE memory register to update.
+ * @param value Integer to set as the value.
+ * @param is_neg Flag to determine sign of the value.
+ */
+static inline void
+mem_set_int(struct Mem *mem, int64_t value, bool is_neg)
+{
+	if (unlikely(VdbeMemDynamic(mem)))
+		vdbeMemClearExternAndSetNull(mem);
+	mem->u.i = value;
+	mem->flags = is_neg ? MEM_Int : MEM_UInt;
+	mem->field_type = FIELD_TYPE_INTEGER;
+}
+
+/**
+ * Set double value.
+ *
+ * @param mem VDBE memory register to update.
+ * @param value Double to set as the value.
+ */
+static inline void
+mem_set_double(struct Mem *mem, double value)
+{
+	if (unlikely(VdbeMemDynamic(mem)))
+		vdbeMemClearExternAndSetNull(mem);
+	if (!sqlIsNaN(value))
+		mem->flags = MEM_Real;
+	else
+		mem->flags = MEM_Null;
+	mem->u.r = value;
+	mem->field_type = FIELD_TYPE_DOUBLE;
+}
+
+/**
+ * Set boolean value.
+ *
+ * @param mem VDBE memory register to update.
+ * @param value Bool to set as the value.
+ */
+static inline void
+mem_set_bool(struct Mem *mem, bool value)
+{
+	if (unlikely(VdbeMemDynamic(mem)))
+		vdbeMemClearExternAndSetNull(mem);
+	mem->u.b = value;
+	mem->flags = MEM_Bool;
+	mem->field_type = FIELD_TYPE_BOOLEAN;
+}
+
+/**
+ * Set string value. Given value is not freed by MEM.
+ *
+ * @param mem VDBE memory register to update.
+ * @param value String to set.
+ * @param len Length of the string, not including '\0'.
+ * @param alloc_type Type of allocation, either MEM_Static or MEM_Ephem.
+ * @param is_null_terminated True if string is NULL-terminated.
+ */
+static inline void
+mem_set_str(struct Mem *mem, char *value, uint32_t len, int alloc_type,
+	    bool is_null_terminated)
+{
+	if (unlikely(VdbeMemDynamic(mem)))
+		vdbeMemClearExternAndSetNull(mem);
+	assert(alloc_type == MEM_Ephem || alloc_type == MEM_Static);
+	mem->z = value;
+	mem->n = len;
+	mem->flags = MEM_Str | alloc_type;
+	if (is_null_terminated)
+		mem->flags |= MEM_Term;
+	mem->field_type = FIELD_TYPE_STRING;
+}
+
+/**
+ * Set string value by allocating memory and copying given value. Allocated
+ * memory should be freed by MEM.
+ *
+ * @param mem VDBE memory register to update.
+ * @param value String to set.
+ * @param len Length of the string, not including '\0'.
+ * @param is_null_terminated True if string is NULL-terminated.
+ */
+static inline int
+mem_copy_str(struct Mem *mem, const char *value, uint32_t len,
+	     bool is_null_terminated)
+{
+	uint32_t size = is_null_terminated ? len + 1 : len;
+	if (sqlVdbeMemClearAndResize(mem, size) != 0)
+		return -1;
+	memcpy(mem->z, value, size);
+	mem->n = len;
+	mem->flags = MEM_Str;
+	if (is_null_terminated)
+		mem->flags |= MEM_Term;
+	mem->field_type = FIELD_TYPE_STRING;
+	return 0;
+}
+
+/**
+ * Set binary string value. Given value is not freed by MEM. If is_zero is true
+ * then binary received from this MEM may be expanded to length that more than
+ * size of the value, however this is done outside of this MEM.
+ *
+ * @param mem VDBE memory register to update.
+ * @param value Binary string to set.
+ * @param size Length of the binary string.
+ * @param alloc_type Type of allocation, either MEM_Static or MEM_Ephem.
+ * @param is_zero True if binary string may be expanded with zeroes.
+ */
+static inline void
+mem_set_bin(struct Mem *mem, char *value, uint32_t size, int alloc_type,
+	    bool is_zero)
+{
+	if (unlikely(VdbeMemDynamic(mem)))
+		vdbeMemClearExternAndSetNull(mem);
+	assert(alloc_type == MEM_Ephem || alloc_type == MEM_Static);
+	mem->z = value;
+	mem->n = size;
+	mem->flags = MEM_Blob | alloc_type;
+	if (is_zero)
+		mem->flags |= MEM_Zero;
+	mem->field_type = FIELD_TYPE_VARBINARY;
+}
+
+/**
+ * Set binary string value by allocating memory and copying given value.
+ * Allocated memory should be freed by MEM.
+ *
+ * @param mem VDBE memory register to update.
+ * @param value Binary string to set.
+ * @param size Length of the binary string.
+ * @param is_zero True if binary string may be expanded with zeroes.
+ */
+static inline int
+mem_copy_bin(struct Mem *mem, const char *value, uint32_t size, bool is_zero)
+{
+	if (sqlVdbeMemClearAndResize(mem, size) != 0)
+		return -1;
+	memcpy(mem->z, value, size);
+	mem->n = size;
+	mem->flags = MEM_Blob;
+	if (is_zero)
+		mem->flags |= MEM_Zero;
+	mem->field_type = FIELD_TYPE_VARBINARY;
+	return 0;
+}
+
+/**
+ * Set VDBE memory register as MAP.
+ *
+ * @param mem VDBE memory register to update.
+ * @param value Binary string that contains msgpack with type MP_MAP to set.
+ * @param len Length of the binary string.
+ * @param alloc_type Type of allocation, either MEM_Static or MEM_Ephem.
+ */
+static inline void
+mem_set_map(struct Mem *mem, char *value, uint32_t size, int alloc_type)
+{
+	if (unlikely(VdbeMemDynamic(mem)))
+		vdbeMemClearExternAndSetNull(mem);
+	assert(alloc_type == MEM_Ephem || alloc_type == MEM_Static);
+	mem->z = value;
+	mem->n = size;
+	mem->flags = MEM_Blob | MEM_Subtype | alloc_type;
+	mem->subtype = SQL_SUBTYPE_MSGPACK;
+	mem->field_type = FIELD_TYPE_MAP;
+}
+
+/**
+ * Set VDBE memory register as ARRAY.
+ *
+ * @param mem VDBE memory register to update.
+ * @param value Binary string that contains msgpack with type MP_ARRAY to set.
+ * @param len Length of the binary string.
+ * @param alloc_type Type of allocation, either MEM_Static or MEM_Ephem.
+ */
+static inline void
+mem_set_array(struct Mem *mem, char *value, uint32_t size, int alloc_type)
+{
+	if (unlikely(VdbeMemDynamic(mem)))
+		vdbeMemClearExternAndSetNull(mem);
+	assert(alloc_type == MEM_Ephem || alloc_type == MEM_Static);
+	mem->z = value;
+	mem->n = size;
+	mem->flags = MEM_Blob | MEM_Subtype | alloc_type;
+	mem->subtype = SQL_SUBTYPE_MSGPACK;
+	mem->field_type = FIELD_TYPE_ARRAY;
+}
+
+static inline bool
+mem_is_null(const struct Mem *mem)
+{
+	return (mem->flags & MEM_Null) != 0;
+}
+
+static inline bool
+mem_is_undefined(const struct Mem *mem)
+{
+	return (mem->flags & MEM_Undefined) != 0;
+}
+
+static inline bool
+mem_is_frame(const struct Mem *mem)
+{
+	return (mem->flags & MEM_Frame) != 0;
+}
+
+static inline bool
+mem_is_neg_int(const struct Mem *mem)
+{
+	return (mem->flags & MEM_Int) != 0;
+}
+
+static inline bool
+mem_is_pos_int(const struct Mem *mem)
+{
+	return (mem->flags & MEM_UInt) != 0;
+}
+
+static inline bool
+mem_is_integer(const struct Mem *mem)
+{
+	return (mem->flags & (MEM_Int | MEM_UInt)) != 0;
+}
+
+static inline bool
+mem_is_double(const struct Mem *mem)
+{
+	return (mem->flags & MEM_Real) != 0;
+}
+
+static inline bool
+mem_is_number(const struct Mem *mem)
+{
+	return (mem->flags & (MEM_Int | MEM_UInt | MEM_Real)) != 0;
+}
+
+static inline bool
+mem_is_string(const struct Mem *mem)
+{
+	return (mem->flags & MEM_Str) != 0;
+}
+
+static inline bool
+mem_is_binary(const struct Mem *mem)
+{
+	return (mem->flags & MEM_Blob) != 0;
+}
+
+static inline bool
+mem_is_varstring(const struct Mem *mem)
+{
+	return (mem->flags & (MEM_Str | MEM_Blob)) != 0;
+}
+
+static inline bool
+mem_is_map(const struct Mem *mem)
+{
+	return mem_is_binary(mem) && ((mem->flags & MEM_Subtype) != 0) &&
+	       mem->subtype == SQL_SUBTYPE_MSGPACK &&
+	       mp_typeof(*mem->z) == MP_MAP;
+}
+
+static inline bool
+mem_is_array(const struct Mem *mem)
+{
+	return mem_is_binary(mem) && ((mem->flags & MEM_Subtype) != 0) &&
+	       mem->subtype == SQL_SUBTYPE_MSGPACK &&
+	       mp_typeof(*mem->z) == MP_ARRAY;
+}
+
+static inline bool
+mem_is_bool(const struct Mem *mem)
+{
+	return (mem->flags & MEM_Bool) != 0;
+}
+
+static inline bool
+mems_have_same_type(const struct Mem *mem1, const struct Mem *mem2)
+{
+	return (mem1->flags & MEM_PURE_TYPE_MASK) ==
+	       (mem2->flags & MEM_PURE_TYPE_MASK);
+}
+
+/**
+ * Cast MEM to varbinary according to explicit cast rules.
+ *
+ * @param mem VDBE memory register to convert.
+ */
+static inline int
+mem_convert_to_binary(struct Mem *mem)
+{
+	if (mem_is_null(mem) || mem_is_binary(mem))
+		return 0;
+	if (mem_is_string(mem)) {
+		mem->flags = (mem->flags & (MEM_Dyn | MEM_Static | MEM_Ephem)) |
+			     MEM_Blob;
+		return 0;
+	}
+	diag_set(ClientError, ER_SQL_TYPE_MISMATCH, sql_value_to_diag_str(mem),
+		 "varbinary");
+	return -1;
+}
 
-void sqlVdbeMemInit(Mem *, sql *, u32);
-void sqlVdbeMemSetNull(Mem *);
 void sqlVdbeMemSetZeroBlob(Mem *, int);
 int sqlVdbeMemMakeWriteable(Mem *);
 int sqlVdbeMemStringify(Mem *);
@@ -566,19 +980,8 @@ mem_mp_type(struct Mem *mem);
 #define mp_type_is_numeric(X) ((X) == MP_INT || (X) == MP_UINT ||\
 			       (X) == MP_DOUBLE)
 
-/**
- * Memory cell mem contains the context of an aggregate function.
- * This routine calls the finalize method for that function. The
- * result of the aggregate is stored back into mem.
- *
- * Returns -1 if the finalizer reports an error. 0 otherwise.
- */
-int
-sql_vdbemem_finalize(struct Mem *mem, struct func *func);
-
 const char *sqlOpcodeName(int);
 int sqlVdbeMemGrow(Mem * pMem, int n, int preserve);
-int sqlVdbeMemClearAndResize(Mem * pMem, int n);
 int sqlVdbeCloseStatement(Vdbe *, int);
 void sqlVdbeFrameDelete(VdbeFrame *);
 int sqlVdbeFrameRestore(VdbeFrame *);
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 7c59ef83f..338853495 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -111,7 +111,7 @@ sql_clear_bindings(sql_stmt * pStmt)
 	Vdbe *p = (Vdbe *) pStmt;
 	for (i = 0; i < p->nVar; i++) {
 		sqlVdbeMemRelease(&p->aVar[i]);
-		p->aVar[i].flags = MEM_Null;
+		mem_set_null(&p->aVar[i]);
 	}
 	return rc;
 }
@@ -130,12 +130,12 @@ const void *
 sql_value_blob(sql_value * pVal)
 {
 	Mem *p = (Mem *) pVal;
-	if (p->flags & (MEM_Blob | MEM_Str)) {
+	if (mem_is_varstring(p)) {
 		if (ExpandBlob(p) != 0) {
-			assert(p->flags == MEM_Null && p->z == 0);
+			assert(mem_is_null(p) && p->z == NULL);
 			return 0;
 		}
-		p->flags |= MEM_Blob;
+		mem_convert_to_binary(p);
 		return p->n ? p->z : 0;
 	} else {
 		return sql_value_text(pVal);
@@ -232,7 +232,7 @@ sql_value_dup(const sql_value * pOrig)
 	memcpy(pNew, pOrig, MEMCELLSIZE);
 	pNew->flags &= ~MEM_Dyn;
 	pNew->db = 0;
-	if (pNew->flags & (MEM_Str | MEM_Blob)) {
+	if (mem_is_varstring(pNew)) {
 		pNew->flags &= ~(MEM_Static | MEM_Dyn);
 		pNew->flags |= MEM_Ephem;
 		if (sqlVdbeMemMakeWriteable(pNew) != 0) {
@@ -346,7 +346,7 @@ sql_result_bool(struct sql_context *ctx, bool value)
 void
 sql_result_null(sql_context * pCtx)
 {
-	sqlVdbeMemSetNull(pCtx->pOut);
+	mem_set_null(pCtx->pOut);
 }
 
 void
@@ -527,7 +527,7 @@ createAggContext(sql_context * p, int nByte)
 	Mem *pMem = p->pMem;
 	assert((pMem->flags & MEM_Agg) == 0);
 	if (nByte <= 0) {
-		sqlVdbeMemSetNull(pMem);
+		mem_set_null(pMem);
 		pMem->z = 0;
 	} else {
 		sqlVdbeMemClearAndResize(pMem, nByte);
@@ -588,39 +588,17 @@ sql_data_count(sql_stmt * pStmt)
 static const Mem *
 columnNullValue(void)
 {
-	/* Even though the Mem structure contains an element
-	 * of type i64, on certain architectures (x86) with certain compiler
-	 * switches (-Os), gcc may align this Mem object on a 4-byte boundary
-	 * instead of an 8-byte one. This all works fine, except that when
-	 * running with SQL_DEBUG defined the sql code sometimes assert()s
-	 * that a Mem structure is located on an 8-byte boundary. To prevent
-	 * these assert()s from failing, when building with SQL_DEBUG defined
-	 * using gcc, we force nullMem to be 8-byte aligned using the magical
-	 * __attribute__((aligned(8))) macro.
-	 */
-	static const Mem nullMem
 #if defined(SQL_DEBUG) && defined(__GNUC__)
-	    __attribute__ ((aligned(8)))
-#endif
-	    = {
-		/* .u          = */  {
-		0},
-		    /* .flags      = */ (u16) MEM_Null,
-		    /* .eSubtype   = */ (u8) 0,
-		    /* .field_type = */ field_type_MAX,
-		    /* .n          = */ (int)0,
-		    /* .z          = */ (char *)0,
-		    /* .zMalloc    = */ (char *)0,
-		    /* .szMalloc   = */ (int)0,
-		    /* .uTemp      = */ (u32) 0,
-		    /* .db         = */ (sql *) 0,
-		    /* .xDel       = */ (void (*)(void *))0,
-#ifdef SQL_DEBUG
-		    /* .pScopyFrom = */ (Mem *) 0,
-		    /* .pFiller    = */ (void *)0,
+	static struct Mem nullMem __attribute__ ((aligned(8)));
+#else
+	static struct Mem nullMem;
 #endif
-	};
-	return &nullMem;
+	static struct Mem *null_mem_ptr = NULL;
+	if (null_mem_ptr == NULL) {
+		mem_init(&nullMem);
+		null_mem_ptr = &nullMem;
+	}
+	return null_mem_ptr;
 }
 
 /*
@@ -879,8 +857,7 @@ vdbeUnbind(Vdbe * p, int i)
 	i--;
 	pVar = &p->aVar[i];
 	sqlVdbeMemRelease(pVar);
-	pVar->flags = MEM_Null;
-	pVar->field_type = field_type_MAX;
+	mem_set_null(pVar);
 	return 0;
 }
 
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 5c2706e49..fb55f82a6 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1108,18 +1108,18 @@ displayP4(Op * pOp, char *zTemp, int nTemp)
 		}
 	case P4_MEM:{
 			Mem *pMem = pOp->p4.pMem;
-			if (pMem->flags & MEM_Str) {
+			if (mem_is_string(pMem)) {
 				zP4 = pMem->z;
-			} else if (pMem->flags & MEM_Int) {
+			} else if (mem_is_neg_int(pMem)) {
 				sqlXPrintf(&x, "%lld", pMem->u.i);
-			} else if (pMem->flags & MEM_UInt) {
+			} else if (mem_is_pos_int(pMem)) {
 				sqlXPrintf(&x, "%llu", pMem->u.u);
-			} else if (pMem->flags & MEM_Real) {
+			} else if (mem_is_double(pMem)) {
 				sqlXPrintf(&x, "%.16g", pMem->u.r);
-			} else if (pMem->flags & MEM_Null) {
+			} else if (mem_is_null(pMem)) {
 				zP4 = "NULL";
 			} else {
-				assert(pMem->flags & MEM_Blob);
+				assert(mem_is_binary(pMem));
 				zP4 = "(binary string)";
 			}
 			break;
@@ -1198,17 +1198,13 @@ sqlVdbePrintOp(FILE * pOut, int pc, Op * pOp)
  * Initialize an array of N Mem element.
  */
 static void
-initMemArray(Mem * p, int N, sql * db, u32 flags)
+initMemArray(Mem *p, int N, bool is_undefined)
 {
-	while ((N--) > 0) {
-		p->db = db;
-		p->flags = flags;
-		p->szMalloc = 0;
-		p->field_type = field_type_MAX;
-#ifdef SQL_DEBUG
-		p->pScopyFrom = 0;
-#endif
-		p++;
+	for (int i = 0; i < N; ++i) {
+		struct Mem *mem = &p[i];
+		mem_init(mem);
+		if (is_undefined)
+			mem_set_undefined(mem);
 	}
 }
 
@@ -1239,16 +1235,14 @@ releaseMemArray(Mem * p, int N)
 			 */
 			testcase(p->flags & MEM_Agg);
 			testcase(p->flags & MEM_Dyn);
-			testcase(p->flags & MEM_Frame);
-			if (p->
-			    flags & (MEM_Agg | MEM_Dyn | MEM_Frame)) {
+			if (p->flags & (MEM_Agg | MEM_Dyn) || mem_is_frame(p)) {
 				sqlVdbeMemRelease(p);
 			} else if (p->szMalloc) {
 				sqlDbFree(db, p->zMalloc);
 				p->szMalloc = 0;
 			}
 
-			p->flags = MEM_Undefined;
+			mem_set_undefined(p);
 		} while ((++p) < pEnd);
 	}
 }
@@ -1322,7 +1316,7 @@ sqlVdbeList(Vdbe * p)
 		 */
 		assert(p->nMem > 9);
 		pSub = &p->aMem[9];
-		if (pSub->flags & MEM_Blob) {
+		if (mem_is_binary(pSub)) {
 			/* On the first call to sql_step(), pSub will hold a NULL.  It is
 			 * initialized to a BLOB by the P4_SUBPROGRAM processing logic below
 			 */
@@ -1364,10 +1358,10 @@ sqlVdbeList(Vdbe * p)
 
 			pMem++;
 
-			pMem->flags = MEM_Static | MEM_Str | MEM_Term;
-			pMem->z = (char *)sqlOpcodeName(pOp->opcode);	/* Opcode */
+			char *str = (char *)sqlOpcodeName(pOp->opcode);
+			uint32_t len = strlen(str);
+			mem_set_str(pMem, str, len, MEM_Static, true);
 			assert(pMem->z != 0);
-			pMem->n = sqlStrlen30(pMem->z);
 			pMem++;
 
 			/* When an OP_Program opcode is encounter (the only opcode that has
@@ -1382,13 +1376,25 @@ sqlVdbeList(Vdbe * p)
 					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 (j == nSub) {
+					size_t svp = region_used(&fiber()->gc);
+					struct SubProgram **buf = (SubProgram **)
+						region_aligned_alloc(&fiber()->gc,
+								     nByte,
+								     alignof(struct SubProgram));
+					if (buf == NULL) {
+						diag_set(OutOfMemory, nByte,
+							 "region_aligned_alloc",
+							 "buf");
+						p->is_aborted = true;
+						return -1;
+					}
+					if (nSub > 0)
+						memcpy(buf, pSub->z, pSub->n);
+					buf[nSub++] = pOp->p4.pProgram;
+					mem_copy_bin(pSub, (char *)buf, nByte,
+						    false);
+					region_truncate(&fiber()->gc, svp);
 				}
 			}
 		}
@@ -1402,41 +1408,39 @@ sqlVdbeList(Vdbe * p)
 		mem_set_i64(pMem, pOp->p3);
 		pMem++;
 
-		if (sqlVdbeMemClearAndResize(pMem, 256)) {
-			assert(p->db->mallocFailed);
+		size_t size = 256;
+		size_t svp = region_used(&fiber()->gc);
+		char *buf = (char *)region_alloc(&fiber()->gc, size);
+		if (buf == NULL) {
+			diag_set(OutOfMemory, size, "region_alloc", "buf");
+			p->is_aborted = true;
 			return -1;
 		}
-		pMem->flags = MEM_Str | MEM_Term;
-		zP4 = displayP4(pOp, pMem->z, pMem->szMalloc);
-
-		if (zP4 != pMem->z) {
-			pMem->n = 0;
-			sqlVdbeMemSetStr(pMem, zP4, -1, 1, 0);
-		} else {
-			assert(pMem->z != 0);
-			pMem->n = sqlStrlen30(pMem->z);
-		}
+		zP4 = displayP4(pOp, buf, size);
+		mem_copy_str(pMem, zP4, strlen(zP4), true);
+		region_truncate(&fiber()->gc, svp);
 		pMem++;
 
 		if (p->explain == 1) {
-			if (sqlVdbeMemClearAndResize(pMem, 4)) {
-				assert(p->db->mallocFailed);
-				return -1;
-			}
-			pMem->flags = MEM_Str | MEM_Term;
-			pMem->n = 2;
-			sql_snprintf(3, pMem->z, "%.2x", pOp->p5);	/* P5 */
+			char *str = (char *)tt_sprintf("%02hu", pOp->p5);
+			mem_copy_str(pMem, str, strlen(str), true);
 			pMem++;
 
 #ifdef SQL_ENABLE_EXPLAIN_COMMENTS
-			if (sqlVdbeMemClearAndResize(pMem, 500)) {
-				assert(p->db->mallocFailed);
+			size = 512;
+			svp = region_used(&fiber()->gc);
+			buf = (char *)region_alloc(&fiber()->gc, size);
+			if (buf == NULL) {
+				diag_set(OutOfMemory, size, "region_alloc",
+					 "buf");
+				p->is_aborted = true;
 				return -1;
 			}
-			pMem->flags = MEM_Str | MEM_Term;
-			pMem->n = displayComment(pOp, zP4, pMem->z, 500);
+			displayComment(pOp, zP4, buf, size);
+			mem_copy_str(pMem, buf, strlen(buf), true);
+			region_truncate(&fiber()->gc, svp);
 #else
-			pMem->flags = MEM_Null;	/* Comment */
+			mem_set_null(pMem);
 #endif
 		}
 
@@ -1658,9 +1662,9 @@ sqlVdbeMakeReady(Vdbe * p,	/* The VDBE */
 	} else {
 		p->nCursor = nCursor;
 		p->nVar = (ynVar) nVar;
-		initMemArray(p->aVar, nVar, db, MEM_Null);
+		initMemArray(p->aVar, nVar, false);
 		p->nMem = nMem;
-		initMemArray(p->aMem, nMem, db, MEM_Undefined);
+		initMemArray(p->aMem, nMem, true);
 		memset(p->apCsr, 0, nCursor * sizeof(VdbeCursor *));
 	}
 	sqlVdbeRewind(p);
@@ -1787,7 +1791,7 @@ Cleanup(Vdbe * p)
 			assert(p->apCsr[i] == 0);
 	if (p->aMem) {
 		for (i = 0; i < p->nMem; i++)
-			assert(p->aMem[i].flags == MEM_Undefined);
+			assert(mem_is_undefined(&p->aMem[i]));
 	}
 #endif
 
@@ -2330,7 +2334,7 @@ sqlVdbeAllocUnpackedRecord(struct sql *db, struct key_def *key_def)
 		return 0;
 	p->aMem = (Mem *) & ((char *)p)[ROUND8(sizeof(UnpackedRecord))];
 	for (uint32_t i = 0; i < key_def->part_count + 1; ++i)
-		sqlVdbeMemInit(&p->aMem[i], sql_get(), MEM_Null);
+		mem_init(&p->aMem[i]);
 	p->key_def = key_def;
 	p->nField = key_def->part_count + 1;
 	return p;
@@ -2418,80 +2422,72 @@ sqlBlobCompare(const Mem * pB1, const Mem * pB2)
 int
 sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl)
 {
-	int f1, f2;
-	int combined_flags;
-
-	f1 = pMem1->flags;
-	f2 = pMem2->flags;
-	combined_flags = f1 | f2;
-
 	/* If one value is NULL, it is less than the other. If both values
 	 * are NULL, return 0.
 	 */
-	if (combined_flags & MEM_Null) {
-		return (f2 & MEM_Null) - (f1 & MEM_Null);
-	}
+	if (mem_is_null(pMem1) || mem_is_null(pMem2))
+		return (int)mem_is_null(pMem2) - (int)mem_is_null(pMem1);
 
-	if ((combined_flags & MEM_Bool) != 0) {
-		if ((f1 & f2 & MEM_Bool) != 0) {
+	if (mem_is_bool(pMem1) || mem_is_bool(pMem2)) {
+		if (mem_is_bool(pMem1) && mem_is_bool(pMem2)) {
 			if (pMem1->u.b == pMem2->u.b)
 				return 0;
 			if (pMem1->u.b)
 				return 1;
 			return -1;
 		}
-		if ((f2 & MEM_Bool) != 0)
+		if (mem_is_bool(pMem2))
 			return +1;
 		return -1;
 	}
 
 	/* At least one of the two values is a number
 	 */
-	if ((combined_flags & (MEM_Int | MEM_UInt | MEM_Real)) != 0) {
-		if ((f1 & f2 & MEM_Int) != 0) {
+	if (mem_is_number(pMem1) || mem_is_number(pMem2)) {
+		if (mem_is_neg_int(pMem1) && mem_is_neg_int(pMem2)) {
 			if (pMem1->u.i < pMem2->u.i)
 				return -1;
 			if (pMem1->u.i > pMem2->u.i)
 				return +1;
 			return 0;
 		}
-		if ((f1 & f2 & MEM_UInt) != 0) {
+		if (mem_is_pos_int(pMem1) && mem_is_pos_int(pMem2)) {
 			if (pMem1->u.u < pMem2->u.u)
 				return -1;
 			if (pMem1->u.u > pMem2->u.u)
 				return +1;
 			return 0;
 		}
-		if ((f1 & f2 & MEM_Real) != 0) {
+		if (mem_is_double(pMem1) && mem_is_double(pMem2)) {
 			if (pMem1->u.r < pMem2->u.r)
 				return -1;
 			if (pMem1->u.r > pMem2->u.r)
 				return +1;
 			return 0;
 		}
-		if ((f1 & MEM_Int) != 0) {
-			if ((f2 & MEM_Real) != 0) {
+		if (mem_is_neg_int(pMem1)) {
+			if (mem_is_double(pMem2)) {
 				return double_compare_nint64(pMem2->u.r,
 							     pMem1->u.i, -1);
 			} else {
 				return -1;
 			}
 		}
-		if ((f1 & MEM_UInt) != 0) {
-			if ((f2 & MEM_Real) != 0) {
+		if (mem_is_pos_int(pMem1)) {
+			if (mem_is_double(pMem2)) {
 				return double_compare_uint64(pMem2->u.r,
 							     pMem1->u.u, -1);
-			} else if ((f2 & MEM_Int) != 0) {
+			} else if (mem_is_neg_int(pMem2)) {
 				return +1;
 			} else {
 				return -1;
 			}
 		}
-		if ((f1 & MEM_Real) != 0) {
-			if ((f2 & MEM_Int) != 0) {
+		if (mem_is_double(pMem1)) {
+			if (mem_is_neg_int(pMem2)) {
 				return double_compare_nint64(pMem1->u.r,
 							     pMem2->u.i, 1);
-			} else if ((f2 & MEM_UInt) != 0) {
+			} else if (mem_is_pos_int(pMem2)) {
 				return double_compare_uint64(pMem1->u.r,
 							     pMem2->u.u, 1);
 			} else {
@@ -2504,11 +2500,11 @@ sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl)
 	/* If one value is a string and the other is a blob, the string is less.
 	 * If both are strings, compare using the collating functions.
 	 */
-	if (combined_flags & MEM_Str) {
-		if ((f1 & MEM_Str) == 0) {
+	if (mem_is_string(pMem1) || mem_is_string(pMem2)) {
+		if (!mem_is_string(pMem1)) {
 			return 1;
 		}
-		if ((f2 & MEM_Str) == 0) {
+		if (!mem_is_string(pMem2)) {
 			return -1;
 		}
 		/* The collation sequence must be defined at this point, even if
@@ -2595,7 +2591,7 @@ sqlVdbeGetBoundValue(Vdbe * v, int iVar, u8 aff)
 	assert(iVar > 0);
 	if (v) {
 		Mem *pMem = &v->aVar[iVar - 1];
-		if (0 == (pMem->flags & MEM_Null)) {
+		if (!mem_is_null(pMem)) {
 			sql_value *pRet = sqlValueNew(v->db);
 			if (pRet) {
 				sqlVdbeMemCopy((Mem *) pRet, pMem);
@@ -2613,7 +2609,12 @@ sqlVdbeCompareMsgpack(const char **key1,
 {
 	const char *aKey1 = *key1;
 	Mem *pKey2 = unpacked->aMem + key2_idx;
-	Mem mem1;
+	bool b;
+	uint64_t u;
+	int64_t i;
+	double r;
+	const char *s;
+	size_t size;
 	int rc = 0;
 	switch (mp_typeof(*aKey1)) {
 	default:{
@@ -2622,35 +2623,34 @@ sqlVdbeCompareMsgpack(const char **key1,
 			break;
 		}
 	case MP_NIL:{
-			rc = -((pKey2->flags & MEM_Null) == 0);
+			rc = -(int)!mem_is_null(pKey2);
 			mp_decode_nil(&aKey1);
 			break;
 		}
 	case MP_BOOL:{
-			mem1.u.b = mp_decode_bool(&aKey1);
-			if ((pKey2->flags & MEM_Bool) != 0) {
-				if (mem1.u.b != pKey2->u.b)
-					rc = mem1.u.b ? 1 : -1;
+			b = mp_decode_bool(&aKey1);
+			if (mem_is_bool(pKey2)) {
+				if (b != pKey2->u.b)
+					rc = b ? 1 : -1;
 			} else {
-				rc = (pKey2->flags & MEM_Null) != 0 ? 1 : -1;
+				rc = mem_is_null(pKey2) ? 1 : -1;
 			}
 			break;
 		}
 	case MP_UINT:{
-			mem1.u.u = mp_decode_uint(&aKey1);
-			if ((pKey2->flags & MEM_Int) != 0) {
+			u = mp_decode_uint(&aKey1);
+			if (mem_is_neg_int(pKey2)) {
 				rc = +1;
-			} else if ((pKey2->flags & MEM_UInt) != 0) {
-				if (mem1.u.u < pKey2->u.u)
+			} else if (mem_is_pos_int(pKey2)) {
+				if (u < pKey2->u.u)
 					rc = -1;
-				else if (mem1.u.u > pKey2->u.u)
+				else if (u > pKey2->u.u)
 					rc = +1;
-			} else if ((pKey2->flags & MEM_Real) != 0) {
-				rc = double_compare_uint64(pKey2->u.r,
-							   mem1.u.u, -1);
-			} else if ((pKey2->flags & MEM_Null) != 0) {
+			} else if (mem_is_double(pKey2)) {
+				rc = double_compare_uint64(pKey2->u.r, u, -1);
+			} else if (mem_is_null(pKey2)) {
 				rc = 1;
-			} else if ((pKey2->flags & MEM_Bool) != 0) {
+			} else if (mem_is_bool(pKey2)) {
 				rc = 1;
 			} else {
 				rc = -1;
@@ -2658,21 +2658,20 @@ sqlVdbeCompareMsgpack(const char **key1,
 			break;
 		}
 	case MP_INT:{
-			mem1.u.i = mp_decode_int(&aKey1);
-			if ((pKey2->flags & MEM_UInt) != 0) {
+			i = mp_decode_int(&aKey1);
+			if (mem_is_pos_int(pKey2)) {
 				rc = -1;
-			} else if ((pKey2->flags & MEM_Int) != 0) {
-				if (mem1.u.i < pKey2->u.i) {
+			} else if (mem_is_neg_int(pKey2)) {
+				if (i < pKey2->u.i) {
 					rc = -1;
-				} else if (mem1.u.i > pKey2->u.i) {
+				} else if (i > pKey2->u.i) {
 					rc = +1;
 				}
-			} else if (pKey2->flags & MEM_Real) {
-				rc = double_compare_nint64(pKey2->u.r, mem1.u.i,
-							   -1);
-			} else if ((pKey2->flags & MEM_Null) != 0) {
+			} else if (mem_is_double(pKey2)) {
+				rc = double_compare_nint64(pKey2->u.r, i, -1);
+			} else if (mem_is_null(pKey2)) {
 				rc = 1;
-			} else if ((pKey2->flags & MEM_Bool) != 0) {
+			} else if (mem_is_bool(pKey2)) {
 				rc = 1;
 			} else {
 				rc = -1;
@@ -2680,27 +2679,25 @@ sqlVdbeCompareMsgpack(const char **key1,
 			break;
 		}
 	case MP_FLOAT:{
-			mem1.u.r = mp_decode_float(&aKey1);
+			r = mp_decode_float(&aKey1);
 			goto do_float;
 		}
 	case MP_DOUBLE:{
-			mem1.u.r = mp_decode_double(&aKey1);
+			r = mp_decode_double(&aKey1);
  do_float:
-			if ((pKey2->flags & MEM_Int) != 0) {
-				rc = double_compare_nint64(mem1.u.r, pKey2->u.i,
-							   1);
-			} else if (pKey2->flags & MEM_UInt) {
-				rc = double_compare_uint64(mem1.u.r,
-							   pKey2->u.u, 1);
-			} else if (pKey2->flags & MEM_Real) {
-				if (mem1.u.r < pKey2->u.r) {
+			if (mem_is_neg_int(pKey2)) {
+				rc = double_compare_nint64(r, pKey2->u.i, 1);
+			} else if (mem_is_pos_int(pKey2)) {
+				rc = double_compare_uint64(r, pKey2->u.u, 1);
+			} else if (mem_is_double(pKey2)) {
+				if (r < pKey2->u.r) {
 					rc = -1;
-				} else if (mem1.u.r > pKey2->u.r) {
+				} else if (r > pKey2->u.r) {
 					rc = +1;
 				}
-			} else if ((pKey2->flags & MEM_Null) != 0) {
+			} else if (mem_is_null(pKey2)) {
 				rc = 1;
-			} else if ((pKey2->flags & MEM_Bool) != 0) {
+			} else if (mem_is_bool(pKey2)) {
 				rc = 1;
 			} else {
 				rc = -1;
@@ -2708,45 +2705,43 @@ sqlVdbeCompareMsgpack(const char **key1,
 			break;
 		}
 	case MP_STR:{
-			if (pKey2->flags & MEM_Str) {
+			if (mem_is_string(pKey2)) {
 				struct key_def *key_def = unpacked->key_def;
-				mem1.n = mp_decode_strl(&aKey1);
-				mem1.z = (char *)aKey1;
-				aKey1 += mem1.n;
+				size = mp_decode_strl(&aKey1);
+				s = aKey1;
+				aKey1 += size;
 				struct coll *coll =
 					key_def->parts[key2_idx].coll;
 				if (coll != NULL) {
-					mem1.flags = MEM_Str;
-					rc = vdbeCompareMemString(&mem1, pKey2,
-								  coll);
+					rc = coll->cmp(s, size, pKey2->z,
+						       (size_t)pKey2->n, coll);
 				} else {
 					goto do_bin_cmp;
 				}
 			} else {
-				rc = (pKey2->flags & MEM_Blob) ? -1 : +1;
+				rc = mem_is_binary(pKey2) ? -1 : +1;
 			}
 			break;
 		}
 	case MP_BIN:{
-			mem1.n = mp_decode_binl(&aKey1);
-			mem1.z = (char *)aKey1;
-			aKey1 += mem1.n;
+			size = mp_decode_binl(&aKey1);
+			s = aKey1;
+			aKey1 += size;
  do_blob:
-			if (pKey2->flags & MEM_Blob) {
+			if (mem_is_binary(pKey2)) {
 				if (pKey2->flags & MEM_Zero) {
-					if (!isAllZero
-					    ((const char *)mem1.z, mem1.n)) {
+					if (!isAllZero(s, size)) {
 						rc = 1;
 					} else {
-						rc = mem1.n - pKey2->u.nZero;
+						rc = size - pKey2->u.nZero;
 					}
 				} else {
 					int nCmp;
  do_bin_cmp:
-					nCmp = MIN(mem1.n, pKey2->n);
-					rc = memcmp(mem1.z, pKey2->z, nCmp);
+					nCmp = MIN((int)size, pKey2->n);
+					rc = memcmp(s, pKey2->z, nCmp);
 					if (rc == 0)
-						rc = mem1.n - pKey2->n;
+						rc = size - pKey2->n;
 				}
 			} else {
 				rc = 1;
@@ -2756,9 +2751,9 @@ sqlVdbeCompareMsgpack(const char **key1,
 	case MP_ARRAY:
 	case MP_MAP:
 	case MP_EXT:{
-			mem1.z = (char *)aKey1;
+			s = aKey1;
 			mp_next(&aKey1);
-			mem1.n = aKey1 - (char *)mem1.z;
+			size = aKey1 - s;
 			goto do_blob;
 		}
 	}
@@ -2795,59 +2790,63 @@ vdbe_decode_msgpack_into_mem(const char *buf, struct Mem *mem, uint32_t *len)
 {
 	const char *start_buf = buf;
 	switch (mp_typeof(*buf)) {
-	case MP_ARRAY:
-	case MP_MAP:
-	case MP_EXT:
-	default: {
-		mem->flags = 0;
+	case MP_ARRAY: {
+		char *data = (char *)buf;
+		mp_next(&buf);
+		mem_set_array(mem, data, buf - data, MEM_Ephem);
+		break;
+	}
+	case MP_MAP: {
+		char *data = (char *)buf;
+		mp_next(&buf);
+		mem_set_map(mem, data, buf - data, MEM_Ephem);
+		break;
+	}
+	case MP_EXT: {
+		char *data = (char *)buf;
+		mp_next(&buf);
+		mem_set_bin(mem, data, buf - data, MEM_Ephem, false);
 		break;
 	}
 	case MP_NIL: {
 		mp_decode_nil(&buf);
-		mem->flags = MEM_Null;
+		mem_set_null(mem);
 		break;
 	}
 	case MP_BOOL: {
-		mem->u.b = mp_decode_bool(&buf);
-		mem->flags = MEM_Bool;
+		mem_set_bool(mem, mp_decode_bool(&buf));
 		break;
 	}
 	case MP_UINT: {
-		uint64_t v = mp_decode_uint(&buf);
-		mem->u.u = v;
-		mem->flags = MEM_UInt;
+		mem_set_u64(mem, mp_decode_uint(&buf));
 		break;
 	}
 	case MP_INT: {
-		mem->u.i = mp_decode_int(&buf);
-		mem->flags = MEM_Int;
+		mem_set_i64(mem, mp_decode_int(&buf));
 		break;
 	}
 	case MP_STR: {
-		/* XXX u32->int */
-		mem->n = (int) mp_decode_strl(&buf);
-		mem->flags = MEM_Str | MEM_Ephem;
-install_blob:
-		mem->z = (char *)buf;
-		buf += mem->n;
+		uint32_t len = mp_decode_strl(&buf);
+		mem_set_str(mem, (char *)buf, len, MEM_Ephem, false);
+		buf += len;
 		break;
 	}
 	case MP_BIN: {
-		/* XXX u32->int */
-		mem->n = (int) mp_decode_binl(&buf);
-		mem->flags = MEM_Blob | MEM_Ephem;
-		goto install_blob;
+		uint32_t size = mp_decode_binl(&buf);
+		mem_set_bin(mem, (char *)buf, size, MEM_Ephem, false);
+		buf += size;
+		break;
 	}
 	case MP_FLOAT: {
-		mem->u.r = mp_decode_float(&buf);
-		mem->flags = sqlIsNaN(mem->u.r) ? MEM_Null : MEM_Real;
+		mem_set_double(mem, mp_decode_float(&buf));
 		break;
 	}
 	case MP_DOUBLE: {
-		mem->u.r = mp_decode_double(&buf);
-		mem->flags = sqlIsNaN(mem->u.r) ? MEM_Null : MEM_Real;
+		mem_set_double(mem, mp_decode_double(&buf));
 		break;
 	}
+	default:
+		unreachable();
 	}
 	*len = (uint32_t)(buf - start_buf);
 	return 0;
@@ -2870,15 +2869,8 @@ sqlVdbeRecordUnpackMsgpack(struct key_def *key_def,	/* Information about the rec
 		pMem->z = 0;
 		uint32_t sz = 0;
 		vdbe_decode_msgpack_into_mem(zParse, pMem, &sz);
-		if (sz == 0) {
-			/* MsgPack array, map or ext. Treat as blob. */
-			pMem->z = (char *)zParse;
-			mp_next(&zParse);
-			pMem->n = zParse - pMem->z;
-			pMem->flags = MEM_Blob | MEM_Ephem;
-		} else {
-			zParse += sz;
-		}
+		assert(sz != 0);
+		zParse += sz;
 		pMem++;
 	}
 }
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 1101f7205..a9b552b9c 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -128,7 +128,7 @@ sqlVdbeMemGrow(Mem * pMem, int n, int bPreserve)
 			pMem->zMalloc = sqlDbMallocRaw(pMem->db, n);
 		}
 		if (pMem->zMalloc == 0) {
-			sqlVdbeMemSetNull(pMem);
+			mem_set_null(pMem);
 			pMem->z = 0;
 			pMem->szMalloc = 0;
 			return -1;
@@ -151,18 +151,6 @@ sqlVdbeMemGrow(Mem * pMem, int n, int bPreserve)
 	return 0;
 }
 
-/*
- * Change the pMem->zMalloc allocation to be at least szNew bytes.
- * If pMem->zMalloc already meets or exceeds the requested size, this
- * routine is a no-op.
- *
- * Any prior string or blob content in the pMem object may be discarded.
- * The pMem->xDel destructor is called, if it exists.  Though MEM_Str
- * and MEM_Blob values may be discarded, MEM_Int, MEM_Real, and MEM_Null
- * values are preserved.
- *
- * Return 0 on success or -1 if unable to complete the resizing.
- */
 int
 sqlVdbeMemClearAndResize(Mem * pMem, int szNew)
 {
@@ -363,23 +351,13 @@ sql_vdbemem_finalize(struct Mem *mem, struct func *func)
 	return ctx.is_aborted ? -1 : 0;
 }
 
-/*
- * If the memory cell contains a value that must be freed by
- * invoking the external callback in Mem.xDel, then this routine
- * will free that value.  It also sets Mem.flags to MEM_Null.
- *
- * This is a helper routine for sqlVdbeMemSetNull() and
- * for sqlVdbeMemRelease().  Use those other routines as the
- * entry point for releasing Mem resources.
- */
-static SQL_NOINLINE void
+void
 vdbeMemClearExternAndSetNull(Mem * p)
 {
 	assert(VdbeMemDynamic(p));
 	if (p->flags & MEM_Agg) {
 		sql_vdbemem_finalize(p, p->u.func);
 		assert((p->flags & MEM_Agg) == 0);
-		testcase(p->flags & MEM_Dyn);
 	}
 	if (p->flags & MEM_Dyn) {
 		assert(p->xDel != SQL_DYNAMIC && p->xDel != 0);
@@ -390,6 +368,7 @@ vdbeMemClearExternAndSetNull(Mem * p)
 		pFrame->v->pDelFrame = pFrame;
 	}
 	p->flags = MEM_Null;
+	p->field_type = field_type_MAX;
 }
 
 /*
@@ -420,7 +399,7 @@ vdbeMemClear(Mem * p)
  * Use this routine prior to clean up prior to abandoning a Mem, or to
  * reset a Mem back to its minimum memory utilization.
  *
- * Use sqlVdbeMemSetNull() to release just the Mem.xDel space
+ * Use mem_set_null() to release just the Mem.xDel space
  * prior to inserting new content into the Mem.
  */
 void
@@ -743,9 +722,8 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
 		assert(MEM_Str == (MEM_Blob >> 3));
 		if ((pMem->flags & MEM_Bool) != 0) {
 			const char *str_bool = SQL_TOKEN_BOOLEAN(pMem->u.b);
-			sqlVdbeMemSetStr(pMem, str_bool, strlen(str_bool), 1,
-					 SQL_TRANSIENT);
-			return 0;
+			return mem_copy_str(pMem, str_bool, strlen(str_bool),
+					    true);
 		}
 		pMem->flags |= (pMem->flags & MEM_Blob) >> 3;
 			sql_value_apply_type(pMem, FIELD_TYPE_STRING);
@@ -756,57 +734,6 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
 	}
 }
 
-/*
- * Initialize bulk memory to be a consistent Mem object.
- *
- * The minimum amount of initialization feasible is performed.
- */
-void
-sqlVdbeMemInit(Mem * pMem, sql * db, u32 flags)
-{
-	assert((flags & ~MEM_TypeMask) == 0);
-	pMem->flags = flags;
-	pMem->db = db;
-	pMem->szMalloc = 0;
-	pMem->field_type = field_type_MAX;
-}
-
-/*
- * Delete any previous value and set the value stored in *pMem to NULL.
- *
- * This routine calls the Mem.xDel destructor to dispose of values that
- * require the destructor.  But it preserves the Mem.zMalloc memory allocation.
- * To free all resources, use sqlVdbeMemRelease(), which both calls this
- * routine to invoke the destructor and deallocates Mem.zMalloc.
- *
- * Use this routine to reset the Mem prior to insert a new value.
- *
- * Use sqlVdbeMemRelease() to complete erase the Mem prior to abandoning it.
- */
-void
-sqlVdbeMemSetNull(Mem * pMem)
-{
-	if (VdbeMemDynamic(pMem)) {
-		vdbeMemClearExternAndSetNull(pMem);
-	} else {
-		pMem->flags = MEM_Null;
-	}
-}
-
-void
-sqlValueSetNull(sql_value * p)
-{
-	sqlVdbeMemSetNull((Mem *) p);
-}
-
-void
-mem_set_ptr(struct Mem *mem, void *ptr)
-{
-	sqlVdbeMemRelease(mem);
-	mem->flags = MEM_Ptr;
-	mem->u.p = ptr;
-}
-
 /*
  * Delete any previous value and set the value to be a BLOB of length
  * n containing all zeros.
@@ -823,63 +750,6 @@ sqlVdbeMemSetZeroBlob(Mem * pMem, int n)
 	pMem->z = 0;
 }
 
-void
-mem_set_bool(struct Mem *mem, bool value)
-{
-	sqlVdbeMemSetNull(mem);
-	mem->u.b = value;
-	mem->flags = MEM_Bool;
-	mem->field_type = FIELD_TYPE_BOOLEAN;
-}
-
-void
-mem_set_i64(struct Mem *mem, int64_t value)
-{
-	if (VdbeMemDynamic(mem))
-		sqlVdbeMemSetNull(mem);
-	mem->u.i = value;
-	int flag = value < 0 ? MEM_Int : MEM_UInt;
-	MemSetTypeFlag(mem, flag);
-	mem->field_type = FIELD_TYPE_INTEGER;
-}
-
-void
-mem_set_u64(struct Mem *mem, uint64_t value)
-{
-	if (VdbeMemDynamic(mem))
-		sqlVdbeMemSetNull(mem);
-	mem->u.u = value;
-	MemSetTypeFlag(mem, MEM_UInt);
-	mem->field_type = FIELD_TYPE_UNSIGNED;
-}
-
-void
-mem_set_int(struct Mem *mem, int64_t value, bool is_neg)
-{
-	if (VdbeMemDynamic(mem))
-		sqlVdbeMemSetNull(mem);
-	if (is_neg) {
-		assert(value < 0);
-		mem->u.i = value;
-		MemSetTypeFlag(mem, MEM_Int);
-	} else {
-		mem->u.u = value;
-		MemSetTypeFlag(mem, MEM_UInt);
-	}
-	mem->field_type = FIELD_TYPE_INTEGER;
-}
-
-void
-mem_set_double(struct Mem *mem, double value)
-{
-	sqlVdbeMemSetNull(mem);
-	if (sqlIsNaN(value))
-		return;
-	mem->u.r = value;
-	MemSetTypeFlag(mem, MEM_Real);
-	mem->field_type = FIELD_TYPE_DOUBLE;
-}
-
 /*
  * Return true if the Mem object contains a TEXT or BLOB that is
  * too large - whose size exceeds SQL_MAX_LENGTH.
@@ -1021,7 +891,7 @@ sqlVdbeMemSetStr(Mem * pMem,	/* Memory cell to set to string value */
 
 	/* If z is a NULL pointer, set pMem to contain an SQL NULL. */
 	if (!z) {
-		sqlVdbeMemSetNull(pMem);
+		mem_set_null(pMem);
 		return 0;
 	}
 
@@ -1584,7 +1454,7 @@ stat4ValueFromExpr(Parse * pParse,	/* Parse context */
 	if (!pExpr) {
 		pVal = valueNew(db, pAlloc);
 		if (pVal) {
-			sqlVdbeMemSetNull((Mem *) pVal);
+			mem_set_null((Mem *) pVal);
 		}
 	} else if (pExpr->op == TK_VARIABLE
 		   || NEVER(pExpr->op == TK_REGISTER
diff --git a/src/box/sql/vdbesort.c b/src/box/sql/vdbesort.c
index a2d681255..23900b8eb 100644
--- a/src/box/sql/vdbesort.c
+++ b/src/box/sql/vdbesort.c
@@ -2163,13 +2163,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, false) != 0)
 		return -1;
-	}
-	pOut->n = nKey;
-	MemSetTypeFlag(pOut, MEM_Blob);
-	memcpy(pOut->z, pKey, nKey);
-
 	return 0;
 }
 
@@ -2217,7 +2212,7 @@ sqlVdbeSorterCompare(const VdbeCursor * pCsr,	/* Sorter cursor */
 	pKey = vdbeSorterRowkey(pSorter, &nKey);
 	sqlVdbeRecordUnpackMsgpack(pCsr->key_def, pKey, r2);
 	for (i = 0; i < nKeyCol; i++) {
-		if (r2->aMem[i].flags & MEM_Null) {
+		if (mem_is_null(&r2->aMem[i])) {
 			*pRes = -1;
 			return 0;
 		}
diff --git a/src/box/sql/vdbetrace.c b/src/box/sql/vdbetrace.c
index 2ee9f668c..175cbc0a6 100644
--- a/src/box/sql/vdbetrace.c
+++ b/src/box/sql/vdbetrace.c
@@ -147,15 +147,15 @@ sqlVdbeExpandSql(Vdbe * p,	/* The prepared statement being evaluated */
 			nextIndex = idx + 1;
 			assert(idx > 0 && idx <= p->nVar);
 			pVar = &p->aVar[idx - 1];
-			if (pVar->flags & MEM_Null) {
+			if (mem_is_null(pVar)) {
 				sqlStrAccumAppend(&out, "NULL", 4);
-			} else if (pVar->flags & MEM_Int) {
+			} else if (mem_is_neg_int(pVar)) {
 				sqlXPrintf(&out, "%lld", pVar->u.i);
-			} else if (pVar->flags & MEM_UInt) {
+			} else if (mem_is_pos_int(pVar)) {
 				sqlXPrintf(&out, "%llu", pVar->u.u);
-			} else if (pVar->flags & MEM_Real) {
+			} else if (mem_is_double(pVar)) {
 				sqlXPrintf(&out, "%!.15g", pVar->u.r);
-			} else if (pVar->flags & MEM_Str) {
+			} else if (mem_is_string(pVar)) {
 				int nOut;	/* Number of bytes of the string text to include in output */
 				nOut = pVar->n;
 				sqlXPrintf(&out, "'%.*q'", nOut, pVar->z);
@@ -164,7 +164,7 @@ sqlVdbeExpandSql(Vdbe * p,	/* The prepared statement being evaluated */
 					       pVar->u.nZero);
 			} else {
 				int nOut;	/* Number of bytes of the blob to include in output */
-				assert(pVar->flags & MEM_Blob);
+				assert(mem_is_binary(pVar));
 				sqlStrAccumAppend(&out, "x'", 2);
 				nOut = pVar->n;
 				for (i = 0; i < nOut; i++) {
-- 
2.25.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v3 0/2] Encapsulate MEM type changing and checking
  2021-02-20 16:59 [Tarantool-patches] [PATCH v3 0/2] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
  2021-02-20 16:59 ` [Tarantool-patches] [PATCH v3 1/2] sql: Initialize MEM in sqlVdbeAllocUnpackedRecord() Mergen Imeev via Tarantool-patches
  2021-02-20 16:59 ` [Tarantool-patches] [PATCH v3 2/2] sql: Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
@ 2021-02-28 17:35 ` Vladislav Shpilevoy via Tarantool-patches
  2021-03-23 12:34   ` Mergen Imeev via Tarantool-patches
  2 siblings, 1 reply; 7+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-02-28 17:35 UTC (permalink / raw)
  To: imeevma, sergos, tsafin; +Cc: tarantool-patches

Hi! Thanks for the patchset!

On 20.02.2021 17:59, Mergen Imeev via Tarantool-patches wrote:
> This patch-set is part of issue #5818. It changes the way to check and change
> MEM from outside. However, there is almost no changes in functions that work
> with internal structure of MEM, most of which located in vdbemem.c. This will be
> done in another patch-set.
> 
> https://github.com/tarantool/tarantool/issues/5818
> https://github.com/tarantool/tarantool/tree/imeevma/gh-5818-encapsulate-mem-type-checking-and-changing
> 
> Changes in v3:
>   - Inlined most of the introduced functions to improve performance.
>   - Some other fixes in code to improve performance.

How is this patch related to performance? Isn't it called 'encapsulate'? Not
'optimize'.

Or did the non-inlined approach make the performance worse than it is on the
master branch? I see that on the master branch things like mem_set_... are
not inlined and all is fine.

I have no issues with optimizing it a bit, as long as it does not involve
doing extra actions and does not complicate the code even more. And is not
motivated by introduction of a perf issue in a different place.

My main concern to the patchset is why do we still have Mem in that scattered
all over the other source files? Can it be extracted into a new file sql/mem.h
and .c? With proper API, separated into public and private.

> Changes in v2:
>   - Squashed almost all patches.
>   - Review fixes.
> 
> Mergen Imeev (2):
>   sql: Initialize MEM in sqlVdbeAllocUnpackedRecord()
>   sql: Encapsulate MEM type changing and checking
> 
>  src/box/sql/func.c      |  14 +-
>  src/box/sql/sqlInt.h    |   1 -
>  src/box/sql/vdbe.c      | 513 ++++++++++++++++++----------------------
>  src/box/sql/vdbeInt.h   | 465 +++++++++++++++++++++++++++++++++---
>  src/box/sql/vdbeapi.c   |  57 ++---
>  src/box/sql/vdbeaux.c   | 360 ++++++++++++++--------------
>  src/box/sql/vdbemem.c   | 146 +-----------
>  src/box/sql/vdbesort.c  |   9 +-
>  src/box/sql/vdbetrace.c |  12 +-
>  9 files changed, 874 insertions(+), 703 deletions(-)
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v3 2/2] sql: Encapsulate MEM type changing and checking
  2021-02-20 16:59 ` [Tarantool-patches] [PATCH v3 2/2] sql: Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
@ 2021-02-28 17:35   ` Vladislav Shpilevoy via Tarantool-patches
  2021-03-23 13:07     ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 7+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-02-28 17:35 UTC (permalink / raw)
  To: imeevma, sergos, tsafin; +Cc: tarantool-patches

Thanks for the patch!

See 33 comments below.

On 20.02.2021 17:59, imeevma@tarantool.org wrote:
> This patch encapsulates type changing and checking for MEM. This is done to make
> it easier for us to introduce new rules for implicit and explicit type casting
> and new types in SQL.
> 
> Part of #5818
> ---
>  src/box/sql/func.c      |  14 +-
>  src/box/sql/sqlInt.h    |   1 -
>  src/box/sql/vdbe.c      | 513 ++++++++++++++++++----------------------
>  src/box/sql/vdbeInt.h   | 465 +++++++++++++++++++++++++++++++++---
>  src/box/sql/vdbeapi.c   |  57 ++---
>  src/box/sql/vdbeaux.c   | 360 ++++++++++++++--------------
>  src/box/sql/vdbemem.c   | 146 +-----------
>  src/box/sql/vdbesort.c  |   9 +-
>  src/box/sql/vdbetrace.c |  12 +-
>  9 files changed, 873 insertions(+), 704 deletions(-)
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index f15d27051..70b8f8eb7 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -283,13 +283,12 @@ port_lua_get_vdbemem(struct port *base, uint32_t *size)
>  			mem_set_u64(&val[i], field.ival);
>  			break;
>  		case MP_STR:
> -			if (sqlVdbeMemSetStr(&val[i], field.sval.data,
> -					     field.sval.len, 1,
> -					     SQL_TRANSIENT) != 0)
> +			if (mem_copy_str(&val[i], field.sval.data,
> +					 field.sval.len, false) != 0)

1. Having flags passed explicitly always makes the code harder to read.
Beause the reader has no idea what 'false' means.

Also you have 4 cases when you calculate strlen() for the argument.

Please, make it 3 separate functions:

	mem_copy_str() // Calls strlen() inside.
	mem_copy_strn() // Does not include 0 byte.
	mem_copy_strn0() // The name can be discussed.

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 3b3b1f01d..8b87165ee 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -116,7 +116,7 @@ int sql_max_blobsize = 0;
>  static void
>  updateMaxBlobsize(Mem *p)
>  {
> -	if ((p->flags & (MEM_Str|MEM_Blob))!=0 && p->n>sql_max_blobsize) {
> +	if (mem_is_varstring(p) && p->n>sql_max_blobsize) {

2. Please, add whitespaces around `>`.

>  		sql_max_blobsize = p->n;
>  	}
>  }
> @@ -185,7 +185,7 @@ vdbeTakeBranch(int iSrcLine, u8 I, u8 M)
>   * already. Return non-zero if a malloc() fails.
>   */
>  #define Stringify(P)						\
> -	if(((P)->flags&(MEM_Str|MEM_Blob))==0 && sqlVdbeMemStringify(P)) \
> +	if(!mem_is_varstring(P) && sqlVdbeMemStringify(P)) \

3. Please, add whitespace after `if`. Also for error check we should
use `sqlVdbeMemStringify(P) != 0`, no implicit bool casts.

> @@ -528,16 +518,13 @@ mem_convert_to_numeric(struct Mem *mem, enum field_type type)
>   * numeric type, if has one.  Set the pMem->u.r and pMem->u.i fields
>   * accordingly.
>   */
> -static u16 SQL_NOINLINE computeNumericType(Mem *pMem)
> +static bool SQL_NOINLINE computeNumericType(Mem *pMem, int64_t *i, bool *is_neg)

4. For functions doing something we use 0/-1 as success/error sign. Since you
decided to refactor this function. Also we don't use Camel naming notation in
the new code. This code can be considered new, since it is changed on 100%.

It also does not reflect what the function is doing I think. It says 'compute type',
but does not return a type anymore. Besides, it is not about 'numeric' now.
Each part of the name is misleading. Maybe call it mem_convert_to_int()? Or just
inline it in its single usage place?

And finally, we write return type on a separate line in the new code.

>  {
> -	assert((pMem->flags & (MEM_Int | MEM_UInt | MEM_Real)) == 0);
> -	assert((pMem->flags & (MEM_Str|MEM_Blob))!=0);
> -	if (sqlAtoF(pMem->z, &pMem->u.r, pMem->n)==0)
> -		return 0;
> -	bool is_neg;
> -	if (sql_atoi64(pMem->z, (int64_t *) &pMem->u.i, &is_neg, pMem->n) == 0)
> -		return is_neg ? MEM_Int : MEM_UInt;
> -	return MEM_Real;
> +	assert(!mem_is_integer(pMem));
> +	assert(mem_is_varstring(pMem));
> +	if (sql_atoi64(pMem->z, i, is_neg, pMem->n) == 0)
> +		return true;
> +	return false;
>  }
>  
>  /*
> @@ -547,14 +534,17 @@ static u16 SQL_NOINLINE computeNumericType(Mem *pMem)
>   * Unlike mem_apply_numeric_type(), this routine does not modify pMem->flags.
>   * But it does set pMem->u.r and pMem->u.i appropriately.
>   */
> -static u16 numericType(Mem *pMem)
> +static bool numericType(Mem *pMem, int64_t *i, bool *is_neg)

5. All the same comments here. Except that you can keep the function
itself probably. It is used more than once.

>  {
> -	if ((pMem->flags & (MEM_Int | MEM_UInt | MEM_Real)) != 0)
> -		return pMem->flags & (MEM_Int | MEM_UInt | MEM_Real);
> -	if (pMem->flags & (MEM_Str|MEM_Blob)) {
> -		return computeNumericType(pMem);
> +	if (mem_is_integer(pMem)) {
> +		*i = pMem->u.i;
> +		*is_neg = mem_is_neg_int(pMem);
> +		return true;
>  	}
> -	return 0;
> +	if (mem_is_varstring(pMem)) {
> +		return computeNumericType(pMem, i, is_neg);
> +	}
> +	return false;
>  }> @@ -731,35 +718,32 @@ char *
>  
>  /* Allocate memory for internal VDBE structure on region. */
>  static int
>  vdbe_mem_alloc_blob_region(struct Mem *vdbe_mem, uint32_t size)
>  {
> -	vdbe_mem->n = size;
> -	vdbe_mem->z = region_alloc(&fiber()->gc, size);
> -	if (vdbe_mem->z == NULL)
> +	char *buf = region_alloc(&fiber()->gc, size);
> +	if (buf == NULL) {
> +		diag_set(OutOfMemory, size, "region_alloc", "buf");
>  		return -1;
> -	vdbe_mem->flags = MEM_Ephem | MEM_Blob;
> +	}
> +	mem_set_bin(vdbe_mem, buf, size, MEM_Ephem, false);

6. All the same as with mem_copy_str - 'false' does not give a
clue what the function does with that parameter.

>  	assert(sqlVdbeCheckMemInvariants(vdbe_mem));
>  	return 0;
>  }
> @@ -879,34 +863,13 @@ vdbe_field_ref_fetch(struct vdbe_field_ref *field_ref, uint32_t fieldno,
>  	if (vdbe_decode_msgpack_into_mem(data, dest_mem, &dummy) != 0)
>  		return -1;
>  
> -	/*
> -	 * MsgPack map, array or extension (unsupported in sql).
> -	 * Wrap it in a blob verbatim.
> -	 */
> -	if (dest_mem->flags == 0) {
> -		dest_mem->z = (char *) data;
> -		dest_mem->n = vdbe_field_ref_fetch_data(field_ref,
> -							fieldno + 1) - data;
> -		dest_mem->flags = MEM_Blob | MEM_Ephem | MEM_Subtype;
> -		dest_mem->subtype = SQL_SUBTYPE_MSGPACK;
> -	}

7. Why did you delete that? Seems like it was never necessary, right?
Maybe delete it in a separate commit? I saw Timur does not like having
many commits, but he is wrong. The fact of having many commits does not
affect patch readability. Only commit atomicity does. It is fine to
have a titanic commit as long as it is atomic. Just as it is fine to
have 40 commits if they are all atomic too.

I didn't review v1 very thourougly, but at a first glance it looked
better.

> -	/*
> -	 * Add 0 termination (at most for strings)
> -	 * Not sure why do we check MEM_Ephem
> -	 */
> -	if ((dest_mem->flags & (MEM_Ephem | MEM_Str)) ==
> -	    (MEM_Ephem | MEM_Str)) {
> -		int len = dest_mem->n;
> -		if (dest_mem->szMalloc < len + 1) {
> -			if (sqlVdbeMemGrow(dest_mem, len + 1, 1) != 0)
> -				return -1;
> +	if (mem_is_string(dest_mem) && (dest_mem->flags & MEM_Ephem) != 0) {

8. The patch looks incomplete while you still have to check flags manually
in the code which is not a part of Mem API. This function (vdbe_field_ref_fetch)
does not look like a part of Mem's API. It rather belongs to Vdbe.

> +		if (dest_mem->n > 0) {
> +			mem_copy_str(dest_mem, dest_mem->z, dest_mem->n,
> +				     (dest_mem->flags & MEM_Term) != 0);

9. The same here. Perhaps for this entire block you could add a
function mem_copy_strmem(dst, src). It would assert inside that the source
mem is a string object, and would hide all this inside.

>  		} else {
> -			dest_mem->z =
> -				memcpy(dest_mem->zMalloc, dest_mem->z, len);
> -			dest_mem->flags &= ~MEM_Ephem;
> +			mem_set_str(dest_mem, "", 0, MEM_Static, true);
>  		}
> -		dest_mem->z[len] = 0;
> -		dest_mem->flags |= MEM_Term;
>  	}
>  	UPDATE_MAX_BLOBSIZE(dest_mem);
>  	dest_mem->field_type = vdbe_field_ref_fetch_type(field_ref, fieldno);
> @@ -1364,19 +1325,17 @@ case OP_String: {          /* out2 */
>   */
>  case OP_Null: {           /* out2 */
>  	int cnt;
> -	u16 nullFlag;
>  	pOut = vdbe_prepare_null_out(p, pOp->p2);
>  	cnt = pOp->p3-pOp->p2;
>  	assert(pOp->p3<=(p->nMem+1 - p->nCursor));
> -	pOut->flags = nullFlag = pOp->p1 ? (MEM_Null|MEM_Cleared) : MEM_Null;
> -	pOut->n = 0;
> +	if (pOp->p1)

10. We use != 0 instead of implicit cast.

> +		pOut->flags |= MEM_Cleared;
>  	while( cnt>0) {
>  		pOut++;
>  		memAboutToChange(p, pOut);

11. Why isn't this included into each mem_set/mem_copy function?

> -		sqlVdbeMemSetNull(pOut);
> -		pOut->flags = nullFlag;
> -		pOut->field_type = field_type_MAX;
> -		pOut->n = 0;
> +		mem_set_null(pOut);
> +		if (pOp->p1)
> +			pOut->flags |= MEM_Cleared;

12. To help performance you could keep `nullFlag` variable,
and set it to either 0 or MEM_Cleared. Then in the loop
you can do `pOut->flags |= nullFlag;` without any branches.

By the way, the Cleared thing looks related to NULL type. It
changes its behaviour. Maybe it is worth encapsulating it too.
It depends on whether you will manage to extract OP_Ge/...
into a function not depending on Vdbe but only on mems.

Which would be great as the comparison opcode now is enormous,
hard to read, and impossible to unit test.

>  		cnt--;
>  	}
>  	break;> @@ -1608,7 +1566,7 @@ case OP_Concat: {           /* same as TK_CONCAT, in1, in2, out3 */
>  	}
>  
>  	/* Moreover, both operands must be of the same type. */
> -	if (str_type_p1 != str_type_p2) {
> +	if (!mems_have_same_type(pIn1, pIn2)) {

13. Better keep it 'mem', not 'mems'. To have all the Mem API functions
have the same prefix.

>  		diag_set(ClientError, ER_INCONSISTENT_TYPES,
>  			 mem_type_to_str(pIn2), mem_type_to_str(pIn1));
>  		goto abort_due_to_error;
> @@ -1619,21 +1577,19 @@ case OP_Concat: {           /* same as TK_CONCAT, in1, in2, out3 */
>  	if (nByte>db->aLimit[SQL_LIMIT_LENGTH]) {
>  		goto too_big;
>  	}
> -	if (sqlVdbeMemGrow(pOut, (int)nByte+2, pOut==pIn2)) {
> -		goto no_mem;
> +	size_t svp = region_used(&fiber()->gc);
> +	char *buf = region_alloc(&fiber()->gc, nByte);

14. Recently we confirmed that TLS access might be actually not as fast
as a normal variable access. Fiber() reads a TLS variable. So if you
care abot top perf, better cache &fiber()->gc in a variable. Up to you
though. I can't force such rule because can't (and don't want) to measure
if it really wins anything.

> +	if (buf == NULL) {
> +		diag_set(OutOfMemory, nByte, "region_alloc", "buf");
> +		goto abort_due_to_error;
>  	}
> -	if (pIn1->flags & MEM_Str)
> -		MemSetTypeFlag(pOut, MEM_Str);
> +	memcpy(buf, pIn2->z, pIn2->n);
> +	memcpy(&buf[pIn2->n], pIn1->z, pIn1->n);
> +	if (mem_is_binary(pIn1))
> +		mem_copy_bin(pOut, buf, nByte, false);
>  	else
> -		MemSetTypeFlag(pOut, MEM_Blob);
> -	if (pOut!=pIn2) {
> -		memcpy(pOut->z, pIn2->z, pIn2->n);
> -	}
> -	memcpy(&pOut->z[pIn2->n], pIn1->z, pIn1->n);
> -	pOut->z[nByte]=0;
> -	pOut->z[nByte+1] = 0;
> -	pOut->flags |= MEM_Term;
> -	pOut->n = (int)nByte;
> +		mem_copy_str(pOut, buf, nByte, false);
> +	region_truncate(&fiber()->gc, svp);

15. AFAIU, before your patch it was allocated in place, with sqlVdbeMemGrow()
used to reserve the space. Why did you change it? Looks slower. Before your
changes there was 2 copies at most: out mem realloc + second mem copy. Now
there are 4 copies: copy original + new into new place, copy the 2 copies
into the out mem.

>  	UPDATE_MAX_BLOBSIZE(pOut);
>  	break;
>  }
> @@ -1681,27 +1637,25 @@ case OP_Subtract:              /* same as TK_MINUS, in1, in2, out3 */
>  case OP_Multiply:              /* same as TK_STAR, in1, in2, out3 */
>  case OP_Divide:                /* same as TK_SLASH, in1, in2, out3 */
>  case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
> -	u32 flags;      /* Combined MEM_* flags from both inputs */
> -	u16 type1;      /* Numeric type of left operand */
> -	u16 type2;      /* Numeric type of right operand */
> +	bool is_in1_int;      /* Numeric type of left operand */
> +	bool is_in2_int;      /* Numeric type of right operand */

16. The comments are wrong now.

>  	i64 iA;         /* Integer value of left operand */
>  	i64 iB;         /* Integer value of right operand */
> +	bool is_lhs_neg;
> +	bool is_rhs_neg;
>  	double rA;      /* Real value of left operand */
>  	double rB;      /* Real value of right operand */
>  
>  	pIn1 = &aMem[pOp->p1];
> -	type1 = numericType(pIn1);
> +	is_in1_int = numericType(pIn1, (int64_t *)&iA, &is_lhs_neg);
>  	pIn2 = &aMem[pOp->p2];
> -	type2 = numericType(pIn2);
> +	is_in2_int = numericType(pIn2, (int64_t *)&iB, &is_rhs_neg);
>  	pOut = vdbe_prepare_null_out(p, pOp->p3);
> -	flags = pIn1->flags | pIn2->flags;
> -	if ((flags & MEM_Null)!=0) goto arithmetic_result_is_null;
> -	if ((type1 & (MEM_Int | MEM_UInt)) != 0 &&
> -	    (type2 & (MEM_Int | MEM_UInt)) != 0) {
> -		iA = pIn1->u.i;
> -		iB = pIn2->u.i;
> -		bool is_lhs_neg = pIn1->flags & MEM_Int;
> -		bool is_rhs_neg = pIn2->flags & MEM_Int;
> +	if (mem_is_null(pIn1) || mem_is_null(pIn2))
> +		goto arithmetic_result_is_null;
> +	if (is_in1_int && is_in2_int) {
> +		bool is_lhs_neg = mem_is_neg_int(pIn1);
> +		bool is_rhs_neg = mem_is_neg_int(pIn2);

17. You already know is_lhs_neg and is_rhs_neg from the
numericType() calls above.

>  		bool is_res_neg;
>  		switch( pOp->opcode) {
>  		case OP_Add: {
> @@ -2265,10 +2212,10 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
>  			 * or not both operands are null.
>  			 */
>  			assert(pOp->opcode==OP_Eq || pOp->opcode==OP_Ne);
> -			assert((flags1 & MEM_Cleared)==0);
> +			assert((pIn1->flags & MEM_Cleared)==0);
>  			assert((pOp->p5 & SQL_JUMPIFNULL)==0);
> -			if ((flags1&flags3&MEM_Null)!=0
> -			    && (flags3&MEM_Cleared)==0
> +			if (mem_is_null(pIn1) && mem_is_null(pIn3)
> +			    && (pIn3->flags & MEM_Cleared)==0

18. This is what I was talking about with Cleared. It seems to be
an internal flag of Mem which is needed only for the comparison.
Which means you probably should hide it. With the entire comparison
opcode code along. We already have sqlMemCompare. Why does not
the opcode use it? Probably worth doing something with this since
you are working on the encapsulation.

To get the comparison code in order and to hide Mem details in it.

>  				) {
>  				res = 0;  /* Operands are equal */
>  			} else {
> @@ -2314,18 +2261,25 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
>  		res = sqlMemCompare(pIn3, pIn1, NULL);
>  	} else {
>  		enum field_type type = pOp->p5 & FIELD_TYPE_MASK;
> +		struct Mem tmp_mem1, tmp_mem2, *mem1, *mem2;
> +		mem1 = pIn1;
> +		mem2 = pIn3;

19. Wtf? Why do you need to copy the mems? The original code worked
fine without them. It is not just API refactoring then. If this is
really necessary, lets do it in a separate patch with clear explanation
why you need it.

>  		if (sql_type_is_numeric(type)) {
> -			if ((flags1 | flags3)&MEM_Str) {
> -				if ((flags1 & MEM_Str) == MEM_Str) {
> -					mem_apply_numeric_type(pIn1);
> -					testcase( flags3!=pIn3->flags); /* Possible if pIn1==pIn3 */
> -					flags3 = pIn3->flags;
> +			if (mem_is_string(mem1) || mem_is_string(mem2)) {
> +				if (mem_is_string(mem1)) {
> +					mem_init(&tmp_mem1);

20. Constructors should have 'create' suffix.

> +					memcpy(&tmp_mem1, mem1, sizeof(*mem1));
> +					mem1 = &tmp_mem1;
> +					mem_apply_numeric_type(mem1);
>  				}
> -				if ((flags3 & MEM_Str) == MEM_Str) {
> -					if (mem_apply_numeric_type(pIn3) != 0) {
> +				if (mem_is_string(mem2)) {
> +					mem_init(&tmp_mem2);
> +					memcpy(&tmp_mem2, mem2, sizeof(*mem2));
> +					mem2 = &tmp_mem2;
> +					if (mem_apply_numeric_type(mem2) != 0) {
>  						diag_set(ClientError,
>  							 ER_SQL_TYPE_MISMATCH,
> -							 sql_value_to_diag_str(pIn3),
> +							 sql_value_to_diag_str(mem2),
>  							 "numeric");
>  						goto abort_due_to_error;
>  					}
> @@ -3860,14 +3797,14 @@ case OP_FCopy: {     /* out2 */
>  		pIn1 = &aMem[pOp->p1];
>  	}
>  
> -	if ((pOp->p3 & OPFLAG_NOOP_IF_NULL) && (pIn1->flags & MEM_Null)) {
> +	if ((pOp->p3 & OPFLAG_NOOP_IF_NULL) && mem_is_null(pIn1)) {
>  		pOut = vdbe_prepare_null_out(p, pOp->p2);
>  	} else {
>  		assert(memIsValid(pIn1));
> -		assert((pIn1->flags & (MEM_Int | MEM_UInt)) != 0);
> +		assert(mem_is_integer(pIn1));
>  
>  		pOut = vdbe_prepare_null_out(p, pOp->p2);
> -		mem_set_int(pOut, pIn1->u.i, pIn1->flags == MEM_Int);
> +		mem_set_int(pOut, pIn1->u.i, mem_is_neg_int(pIn1));

21. Perhaps it is worth adding mem_set_intmem(Mem* src). You won't
need to call mem_is_neg_int() then. -1 branch.

>  		pOut->field_type = pIn1->field_type;
>  	}
>  	break;
> @@ -5252,7 +5187,7 @@ case OP_AggFinal: {
>  	Mem *pMem;
>  	assert(pOp->p1>0 && pOp->p1<=(p->nMem+1 - p->nCursor));
>  	pMem = &aMem[pOp->p1];
> -	assert((pMem->flags & ~(MEM_Null|MEM_Agg))==0);
> +	assert(mem_is_null(pMem) || (pMem->flags & MEM_Agg) != 0);

22. MEM_Agg says "Mem.z points to an agg function context". So it
is related to mem types. And probably there must be a helper
mem_is_aggctx().

>  	if (sql_vdbemem_finalize(pMem, pOp->p4.func) != 0)
>  		goto abort_due_to_error;
>  	UPDATE_MAX_BLOBSIZE(pMem);
> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index 7205f1af3..a80a7b511 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> @@ -484,41 +484,455 @@ void sqlVdbeMemMove(Mem *, Mem *);
>  int sqlVdbeMemNulTerminate(Mem *);
>  int sqlVdbeMemSetStr(Mem *, const char *, int, u8, void (*)(void *));
>  
> +/**
> + * Memory cell mem contains the context of an aggregate function.
> + * This routine calls the finalize method for that function. The
> + * result of the aggregate is stored back into mem.
> + *
> + * Returns -1 if the finalizer reports an error. 0 otherwise.
> + */
> +int
> +sql_vdbemem_finalize(struct Mem *mem, struct func *func);
> +
> +/*

23. Comments out of function bodies should start from /**.

> + * If the memory cell contains a value that must be freed by
> + * invoking the external callback in Mem.xDel, then this routine
> + * will free that value.  It also sets Mem.flags to MEM_Null.
> + */
>  void
> -mem_set_bool(struct Mem *mem, bool value);
> +vdbeMemClearExternAndSetNull(Mem * p);

24. Please, don't use whitespace after `*`. Here and in the
next declaration.

> +
> +/*
> + * Change the pMem->zMalloc allocation to be at least szNew bytes.

25. There is no `szNew` argument.

> + * If pMem->zMalloc already meets or exceeds the requested size, this
> + * routine is a no-op.
> + *
> + * Any prior string or blob content in the pMem object may be discarded.
> + * The pMem->xDel destructor is called, if it exists.  Though MEM_Str
> + * and MEM_Blob values may be discarded, MEM_Int, MEM_Real, and MEM_Null
> + * values are preserved.
> + *
> + * Return 0 on success or -1 if unable to complete the resizing.
> + */
> +int
> +sqlVdbeMemClearAndResize(Mem * pMem, int n);
> +
> +/**
> + * Initialize a new mem. After initializing the mem will hold a NULL value.
> + *
> + * @param mem VDBE memory register to initialize.

26. I will remind just in case - using doxygen is not obligatory. Especially
for such trivial functions.

> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index 7c59ef83f..338853495 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -588,39 +588,17 @@ sql_data_count(sql_stmt * pStmt)
>  static const Mem *
>  columnNullValue(void)
>  {
> -	/* Even though the Mem structure contains an element
> -	 * of type i64, on certain architectures (x86) with certain compiler
> -	 * switches (-Os), gcc may align this Mem object on a 4-byte boundary
> -	 * instead of an 8-byte one. This all works fine, except that when
> -	 * running with SQL_DEBUG defined the sql code sometimes assert()s
> -	 * that a Mem structure is located on an 8-byte boundary. To prevent
> -	 * these assert()s from failing, when building with SQL_DEBUG defined
> -	 * using gcc, we force nullMem to be 8-byte aligned using the magical
> -	 * __attribute__((aligned(8))) macro.
> -	 */
> -	static const Mem nullMem
>  #if defined(SQL_DEBUG) && defined(__GNUC__)
> -	    __attribute__ ((aligned(8)))
> -#endif
> -	    = {
> -		/* .u          = */  {
> -		0},
> -		    /* .flags      = */ (u16) MEM_Null,
> -		    /* .eSubtype   = */ (u8) 0,
> -		    /* .field_type = */ field_type_MAX,
> -		    /* .n          = */ (int)0,
> -		    /* .z          = */ (char *)0,
> -		    /* .zMalloc    = */ (char *)0,
> -		    /* .szMalloc   = */ (int)0,
> -		    /* .uTemp      = */ (u32) 0,
> -		    /* .db         = */ (sql *) 0,
> -		    /* .xDel       = */ (void (*)(void *))0,
> -#ifdef SQL_DEBUG
> -		    /* .pScopyFrom = */ (Mem *) 0,
> -		    /* .pFiller    = */ (void *)0,
> +	static struct Mem nullMem __attribute__ ((aligned(8)));
> +#else
> +	static struct Mem nullMem;
>  #endif
> -	};
> -	return &nullMem;
> +	static struct Mem *null_mem_ptr = NULL;
> +	if (null_mem_ptr == NULL) {
> +		mem_init(&nullMem);
> +		null_mem_ptr = &nullMem;
> +	}
> +	return null_mem_ptr;

27. The old code was a bit ugly, yeah, but it didn't have
branches. It is fine to keep the old code, because it is
internal part of Mem API anyway. At least it should be.

>  }
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index 5c2706e49..fb55f82a6 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -1198,17 +1198,13 @@ sqlVdbePrintOp(FILE * pOut, int pc, Op * pOp)
>   * Initialize an array of N Mem element.
>   */
>  static void
> -initMemArray(Mem * p, int N, sql * db, u32 flags)
> +initMemArray(Mem *p, int N, bool is_undefined)
>  {
> -	while ((N--) > 0) {
> -		p->db = db;
> -		p->flags = flags;
> -		p->szMalloc = 0;
> -		p->field_type = field_type_MAX;
> -#ifdef SQL_DEBUG
> -		p->pScopyFrom = 0;
> -#endif
> -		p++;
> +	for (int i = 0; i < N; ++i) {
> +		struct Mem *mem = &p[i];
> +		mem_init(mem);
> +		if (is_undefined)
> +			mem_set_undefined(mem);
>  	}

28. Maybe just inline this entire function.

>  }
> @@ -1382,13 +1376,25 @@ sqlVdbeList(Vdbe * p)
>  					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 (j == nSub) {
> +					size_t svp = region_used(&fiber()->gc);
> +					struct SubProgram **buf = (SubProgram **)
> +						region_aligned_alloc(&fiber()->gc,
> +								     nByte,
> +								     alignof(struct SubProgram));
> +					if (buf == NULL) {
> +						diag_set(OutOfMemory, nByte,
> +							 "region_aligned_alloc",
> +							 "buf");
> +						p->is_aborted = true;
> +						return -1;
> +					}
> +					if (nSub > 0)
> +						memcpy(buf, pSub->z, pSub->n);
> +					buf[nSub++] = pOp->p4.pProgram;
> +					mem_copy_bin(pSub, (char *)buf, nByte,
> +						    false);
> +					region_truncate(&fiber()->gc, svp);
>  				}

29. What was wrong with keeping a 'grow' function? Such changes
only increase number of copies and pad out the code.

>  			}
>  		}
> @@ -1402,41 +1408,39 @@ sqlVdbeList(Vdbe * p)
>  		mem_set_i64(pMem, pOp->p3);
>  		pMem++;
>  
> -		if (sqlVdbeMemClearAndResize(pMem, 256)) {
> -			assert(p->db->mallocFailed);
> +		size_t size = 256;
> +		size_t svp = region_used(&fiber()->gc);
> +		char *buf = (char *)region_alloc(&fiber()->gc, size);
> +		if (buf == NULL) {
> +			diag_set(OutOfMemory, size, "region_alloc", "buf");
> +			p->is_aborted = true;
>  			return -1;
>  		}
> -		pMem->flags = MEM_Str | MEM_Term;
> -		zP4 = displayP4(pOp, pMem->z, pMem->szMalloc);
> -
> -		if (zP4 != pMem->z) {
> -			pMem->n = 0;
> -			sqlVdbeMemSetStr(pMem, zP4, -1, 1, 0);
> -		} else {
> -			assert(pMem->z != 0);
> -			pMem->n = sqlStrlen30(pMem->z);
> -		}
> +		zP4 = displayP4(pOp, buf, size);
> +		mem_copy_str(pMem, zP4, strlen(zP4), true);
> +		region_truncate(&fiber()->gc, svp);
>  		pMem++;
>  
>  		if (p->explain == 1) {
> -			if (sqlVdbeMemClearAndResize(pMem, 4)) {
> -				assert(p->db->mallocFailed);
> -				return -1;
> -			}
> -			pMem->flags = MEM_Str | MEM_Term;
> -			pMem->n = 2;
> -			sql_snprintf(3, pMem->z, "%.2x", pOp->p5);	/* P5 */
> +			char *str = (char *)tt_sprintf("%02hu", pOp->p5);
> +			mem_copy_str(pMem, str, strlen(str), true);

30. Probably Timur didn't know, but tt_sprintf() also is static
alloc. I see no sense in avoiding it if you know what you are doing.

The same below for 512 byte allocation and above for 256.

If someone has issues with static alloc, I propose them to create a
ticket on making its API more safe. For example, introduce static_free()
and use it after each static_alloc() to prevent unintended overwrites.
Would be a good idea to increase the safety.

> @@ -2418,80 +2422,72 @@ sqlBlobCompare(const Mem * pB1, const Mem * pB2)
>  int
>  sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl)
>  {
> -	int f1, f2;
> -	int combined_flags;
> -
> -	f1 = pMem1->flags;
> -	f2 = pMem2->flags;
> -	combined_flags = f1 | f2;
> -
>  	/* If one value is NULL, it is less than the other. If both values
>  	 * are NULL, return 0.
>  	 */
> -	if (combined_flags & MEM_Null) {
> -		return (f2 & MEM_Null) - (f1 & MEM_Null);
> -	}
> +	if (mem_is_null(pMem1) || mem_is_null(pMem2))
> +		return (int)mem_is_null(pMem2) - (int)mem_is_null(pMem1);
>  
> -	if ((combined_flags & MEM_Bool) != 0) {
> -		if ((f1 & f2 & MEM_Bool) != 0) {
> +	if (mem_is_bool(pMem1) || mem_is_bool(pMem2)) {
> +		if (mem_is_bool(pMem1) && mem_is_bool(pMem2)) {

31. Issue with such changes is that the code is a part of internal
Mem's code. I can tell it from the code itsef (it works with
mems only), and from the name (sqlMemCompare). So it could afford
to touch internal members to achieve the best perf. I think it was
totally fine as long as it happened only inside of Mem API functions,
where you are supposed to know meaning of the fields.

This probably arises from the code being a total mess when Mem
functions are defined all over the common files and don't have
their own place. Where you could define big internal functions in
mem.c, and do there whatever you want.

The perf issues you are having might also be because of that.

For instance, in this function 'combined_flags' allowed to do only
one branch per check. Now you do two. Was 'combined & type_flags'
became 'mem1 & type_flags || mem2 & type_flags'. `||` is a branch.

>  			if (pMem1->u.b == pMem2->u.b)
>  				return 0;
>  			if (pMem1->u.b)
>  				return 1;
>  			return -1;
>  		}
> -		if ((f2 & MEM_Bool) != 0)
> +		if (mem_is_bool(pMem2))
>  			return +1;
>  		return -1;
>  	}
> @@ -2613,7 +2609,12 @@ sqlVdbeCompareMsgpack(const char **key1,
>  {
>  	const char *aKey1 = *key1;
>  	Mem *pKey2 = unpacked->aMem + key2_idx;
> -	Mem mem1;
> +	bool b;
> +	uint64_t u;
> +	int64_t i;
> +	double r;
> +	const char *s;
> +	size_t size;

32. What was wrong with declaring these variables in-place?

>  	int rc = 0;
>  	switch (mp_typeof(*aKey1)) {
>  	default:{
> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> index 1101f7205..a9b552b9c 100644
> --- a/src/box/sql/vdbemem.c
> +++ b/src/box/sql/vdbemem.c
> @@ -823,63 +750,6 @@ sqlVdbeMemSetZeroBlob(Mem * pMem, int n)
>  	pMem->z = 0;
>  }
>  
> -void
> -mem_set_bool(struct Mem *mem, bool value)
> -{
> -	sqlVdbeMemSetNull(mem);
> -	mem->u.b = value;
> -	mem->flags = MEM_Bool;
> -	mem->field_type = FIELD_TYPE_BOOLEAN;
> -}
> -
> -void
> -mem_set_i64(struct Mem *mem, int64_t value)
> -{
> -	if (VdbeMemDynamic(mem))
> -		sqlVdbeMemSetNull(mem);
> -	mem->u.i = value;
> -	int flag = value < 0 ? MEM_Int : MEM_UInt;
> -	MemSetTypeFlag(mem, flag);
> -	mem->field_type = FIELD_TYPE_INTEGER;
> -}
> -
> -void
> -mem_set_u64(struct Mem *mem, uint64_t value)
> -{
> -	if (VdbeMemDynamic(mem))
> -		sqlVdbeMemSetNull(mem);
> -	mem->u.u = value;
> -	MemSetTypeFlag(mem, MEM_UInt);
> -	mem->field_type = FIELD_TYPE_UNSIGNED;
> -}
> -
> -void
> -mem_set_int(struct Mem *mem, int64_t value, bool is_neg)
> -{
> -	if (VdbeMemDynamic(mem))
> -		sqlVdbeMemSetNull(mem);
> -	if (is_neg) {
> -		assert(value < 0);
> -		mem->u.i = value;
> -		MemSetTypeFlag(mem, MEM_Int);
> -	} else {
> -		mem->u.u = value;
> -		MemSetTypeFlag(mem, MEM_UInt);
> -	}
> -	mem->field_type = FIELD_TYPE_INTEGER;
> -}
> -
> -void
> -mem_set_double(struct Mem *mem, double value)
> -{
> -	sqlVdbeMemSetNull(mem);
> -	if (sqlIsNaN(value))
> -		return;
> -	mem->u.r = value;
> -	MemSetTypeFlag(mem, MEM_Real);
> -	mem->field_type = FIELD_TYPE_DOUBLE;
> -}

33. So these functions were defined in C file, and there was no
perf issues with them. Doesn't it mean something became wrong just
now?

> -
>  /*
>   * Return true if the Mem object contains a TEXT or BLOB that is
>   * too large - whose size exceeds SQL_MAX_LENGTH.
Overall I think you need to create a plan how to solve the Mem
mess.

A first step would be to locate all Mem API functions. It is not
clear now which of them are about Mem only, and which just happened
to have 'mem' in their name. And which don't have 'mem' in their name
but are a part of Mem API.

Then I would propose to move all the mem code to its own couple of
files if possible. If not - break the dependencies of its functions
from the other subsystems and modules, and then move.

Then use proper names for the existing functions.

Then introduce new functions.

This way it should be easier to see which functions are private (they
can touch flags, do combined_flags, do various hacks on Mems, but can't
be used out of mem.c and mem.h), and which are public (the other functions).

This would do the real encapsulation.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v3 0/2] Encapsulate MEM type changing and checking
  2021-02-28 17:35 ` [Tarantool-patches] [PATCH v3 0/2] " Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-23 12:34   ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 7+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-03-23 12:34 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review. My answer below. I already sent you a new
version.

On Sun, Feb 28, 2021 at 06:35:39PM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patchset!
> 
> On 20.02.2021 17:59, Mergen Imeev via Tarantool-patches wrote:
> > This patch-set is part of issue #5818. It changes the way to check and change
> > MEM from outside. However, there is almost no changes in functions that work
> > with internal structure of MEM, most of which located in vdbemem.c. This will be
> > done in another patch-set.
> > 
> > https://github.com/tarantool/tarantool/issues/5818
> > https://github.com/tarantool/tarantool/tree/imeevma/gh-5818-encapsulate-mem-type-checking-and-changing
> > 
> > Changes in v3:
> >   - Inlined most of the introduced functions to improve performance.
> >   - Some other fixes in code to improve performance.
> 
> How is this patch related to performance? Isn't it called 'encapsulate'? Not
> 'optimize'.
> 
> Or did the non-inlined approach make the performance worse than it is on the
> master branch? I see that on the master branch things like mem_set_... are
> not inlined and all is fine.
> 
> I have no issues with optimizing it a bit, as long as it does not involve
> doing extra actions and does not complicate the code even more. And is not
> motivated by introduction of a perf issue in a different place.
> 
> My main concern to the patchset is why do we still have Mem in that scattered
> all over the other source files? Can it be extracted into a new file sql/mem.h
> and .c? With proper API, separated into public and private.
> 
Done. I moved all MEM-related functions to sql/mem.c ans sqm/mem.h. Rewrote some
functions and wrote some new functions.

> > Changes in v2:
> >   - Squashed almost all patches.
> >   - Review fixes.
> > 
> > Mergen Imeev (2):
> >   sql: Initialize MEM in sqlVdbeAllocUnpackedRecord()
> >   sql: Encapsulate MEM type changing and checking
> > 
> >  src/box/sql/func.c      |  14 +-
> >  src/box/sql/sqlInt.h    |   1 -
> >  src/box/sql/vdbe.c      | 513 ++++++++++++++++++----------------------
> >  src/box/sql/vdbeInt.h   | 465 +++++++++++++++++++++++++++++++++---
> >  src/box/sql/vdbeapi.c   |  57 ++---
> >  src/box/sql/vdbeaux.c   | 360 ++++++++++++++--------------
> >  src/box/sql/vdbemem.c   | 146 +-----------
> >  src/box/sql/vdbesort.c  |   9 +-
> >  src/box/sql/vdbetrace.c |  12 +-
> >  9 files changed, 874 insertions(+), 703 deletions(-)
> > 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v3 2/2] sql: Encapsulate MEM type changing and checking
  2021-02-28 17:35   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-23 13:07     ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 7+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-03-23 13:07 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Thank you for the review. My answers below.

On Sun, Feb 28, 2021 at 06:35:46PM +0100, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 33 comments below.
> 
> On 20.02.2021 17:59, imeevma@tarantool.org wrote:
> > This patch encapsulates type changing and checking for MEM. This is done to make
> > it easier for us to introduce new rules for implicit and explicit type casting
> > and new types in SQL.
> > 
> > Part of #5818
> > ---
> >  src/box/sql/func.c      |  14 +-
> >  src/box/sql/sqlInt.h    |   1 -
> >  src/box/sql/vdbe.c      | 513 ++++++++++++++++++----------------------
> >  src/box/sql/vdbeInt.h   | 465 +++++++++++++++++++++++++++++++++---
> >  src/box/sql/vdbeapi.c   |  57 ++---
> >  src/box/sql/vdbeaux.c   | 360 ++++++++++++++--------------
> >  src/box/sql/vdbemem.c   | 146 +-----------
> >  src/box/sql/vdbesort.c  |   9 +-
> >  src/box/sql/vdbetrace.c |  12 +-
> >  9 files changed, 873 insertions(+), 704 deletions(-)
> > 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index f15d27051..70b8f8eb7 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -283,13 +283,12 @@ port_lua_get_vdbemem(struct port *base, uint32_t *size)
> >  			mem_set_u64(&val[i], field.ival);
> >  			break;
> >  		case MP_STR:
> > -			if (sqlVdbeMemSetStr(&val[i], field.sval.data,
> > -					     field.sval.len, 1,
> > -					     SQL_TRANSIENT) != 0)
> > +			if (mem_copy_str(&val[i], field.sval.data,
> > +					 field.sval.len, false) != 0)
> 
> 1. Having flags passed explicitly always makes the code harder to read.
> Beause the reader has no idea what 'false' means.
> 
> Also you have 4 cases when you calculate strlen() for the argument.
> 
> Please, make it 3 separate functions:
> 
> 	mem_copy_str() // Calls strlen() inside.
> 	mem_copy_strn() // Does not include 0 byte.
> 	mem_copy_strn0() // The name can be discussed.
> 
Done. Also, I made separate functions for each allocation type.

> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index 3b3b1f01d..8b87165ee 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -116,7 +116,7 @@ int sql_max_blobsize = 0;
> >  static void
> >  updateMaxBlobsize(Mem *p)
> >  {
> > -	if ((p->flags & (MEM_Str|MEM_Blob))!=0 && p->n>sql_max_blobsize) {
> > +	if (mem_is_varstring(p) && p->n>sql_max_blobsize) {
> 
> 2. Please, add whitespaces around `>`.
> 
I think I did so in new version.

> >  		sql_max_blobsize = p->n;
> >  	}
> >  }
> > @@ -185,7 +185,7 @@ vdbeTakeBranch(int iSrcLine, u8 I, u8 M)
> >   * already. Return non-zero if a malloc() fails.
> >   */
> >  #define Stringify(P)						\
> > -	if(((P)->flags&(MEM_Str|MEM_Blob))==0 && sqlVdbeMemStringify(P)) \
> > +	if(!mem_is_varstring(P) && sqlVdbeMemStringify(P)) \
> 
> 3. Please, add whitespace after `if`. Also for error check we should
> use `sqlVdbeMemStringify(P) != 0`, no implicit bool casts.
> 
Same.

> > @@ -528,16 +518,13 @@ mem_convert_to_numeric(struct Mem *mem, enum field_type type)
> >   * numeric type, if has one.  Set the pMem->u.r and pMem->u.i fields
> >   * accordingly.
> >   */
> > -static u16 SQL_NOINLINE computeNumericType(Mem *pMem)
> > +static bool SQL_NOINLINE computeNumericType(Mem *pMem, int64_t *i, bool *is_neg)
> 
> 4. For functions doing something we use 0/-1 as success/error sign. Since you
> decided to refactor this function. Also we don't use Camel naming notation in
> the new code. This code can be considered new, since it is changed on 100%.
> 
> It also does not reflect what the function is doing I think. It says 'compute type',
> but does not return a type anymore. Besides, it is not about 'numeric' now.
> Each part of the name is misleading. Maybe call it mem_convert_to_int()? Or just
> inline it in its single usage place?
> 
> And finally, we write return type on a separate line in the new code.
> 
I inlined these functions.

> >  {
> > -	assert((pMem->flags & (MEM_Int | MEM_UInt | MEM_Real)) == 0);
> > -	assert((pMem->flags & (MEM_Str|MEM_Blob))!=0);
> > -	if (sqlAtoF(pMem->z, &pMem->u.r, pMem->n)==0)
> > -		return 0;
> > -	bool is_neg;
> > -	if (sql_atoi64(pMem->z, (int64_t *) &pMem->u.i, &is_neg, pMem->n) == 0)
> > -		return is_neg ? MEM_Int : MEM_UInt;
> > -	return MEM_Real;
> > +	assert(!mem_is_integer(pMem));
> > +	assert(mem_is_varstring(pMem));
> > +	if (sql_atoi64(pMem->z, i, is_neg, pMem->n) == 0)
> > +		return true;
> > +	return false;
> >  }
> >  
> >  /*
> > @@ -547,14 +534,17 @@ static u16 SQL_NOINLINE computeNumericType(Mem *pMem)
> >   * Unlike mem_apply_numeric_type(), this routine does not modify pMem->flags.
> >   * But it does set pMem->u.r and pMem->u.i appropriately.
> >   */
> > -static u16 numericType(Mem *pMem)
> > +static bool numericType(Mem *pMem, int64_t *i, bool *is_neg)
> 
> 5. All the same comments here. Except that you can keep the function
> itself probably. It is used more than once.
> 
Same.

> >  {
> > -	if ((pMem->flags & (MEM_Int | MEM_UInt | MEM_Real)) != 0)
> > -		return pMem->flags & (MEM_Int | MEM_UInt | MEM_Real);
> > -	if (pMem->flags & (MEM_Str|MEM_Blob)) {
> > -		return computeNumericType(pMem);
> > +	if (mem_is_integer(pMem)) {
> > +		*i = pMem->u.i;
> > +		*is_neg = mem_is_neg_int(pMem);
> > +		return true;
> >  	}
> > -	return 0;
> > +	if (mem_is_varstring(pMem)) {
> > +		return computeNumericType(pMem, i, is_neg);
> > +	}
> > +	return false;
> >  }> @@ -731,35 +718,32 @@ char *
> >  
> >  /* Allocate memory for internal VDBE structure on region. */
> >  static int
> >  vdbe_mem_alloc_blob_region(struct Mem *vdbe_mem, uint32_t size)
> >  {
> > -	vdbe_mem->n = size;
> > -	vdbe_mem->z = region_alloc(&fiber()->gc, size);
> > -	if (vdbe_mem->z == NULL)
> > +	char *buf = region_alloc(&fiber()->gc, size);
> > +	if (buf == NULL) {
> > +		diag_set(OutOfMemory, size, "region_alloc", "buf");
> >  		return -1;
> > -	vdbe_mem->flags = MEM_Ephem | MEM_Blob;
> > +	}
> > +	mem_set_bin(vdbe_mem, buf, size, MEM_Ephem, false);
> 
> 6. All the same as with mem_copy_str - 'false' does not give a
> clue what the function does with that parameter.
> 
Fixed.

> >  	assert(sqlVdbeCheckMemInvariants(vdbe_mem));
> >  	return 0;
> >  }
> > @@ -879,34 +863,13 @@ vdbe_field_ref_fetch(struct vdbe_field_ref *field_ref, uint32_t fieldno,
> >  	if (vdbe_decode_msgpack_into_mem(data, dest_mem, &dummy) != 0)
> >  		return -1;
> >  
> > -	/*
> > -	 * MsgPack map, array or extension (unsupported in sql).
> > -	 * Wrap it in a blob verbatim.
> > -	 */
> > -	if (dest_mem->flags == 0) {
> > -		dest_mem->z = (char *) data;
> > -		dest_mem->n = vdbe_field_ref_fetch_data(field_ref,
> > -							fieldno + 1) - data;
> > -		dest_mem->flags = MEM_Blob | MEM_Ephem | MEM_Subtype;
> > -		dest_mem->subtype = SQL_SUBTYPE_MSGPACK;
> > -	}
> 
> 7. Why did you delete that? Seems like it was never necessary, right?
> Maybe delete it in a separate commit? I saw Timur does not like having
> many commits, but he is wrong. The fact of having many commits does not
> affect patch readability. Only commit atomicity does. It is fine to
> have a titanic commit as long as it is atomic. Just as it is fine to
> have 40 commits if they are all atomic too.
> 
> I didn't review v1 very thourougly, but at a first glance it looked
> better.
> 
Done.

> > -	/*
> > -	 * Add 0 termination (at most for strings)
> > -	 * Not sure why do we check MEM_Ephem
> > -	 */
> > -	if ((dest_mem->flags & (MEM_Ephem | MEM_Str)) ==
> > -	    (MEM_Ephem | MEM_Str)) {
> > -		int len = dest_mem->n;
> > -		if (dest_mem->szMalloc < len + 1) {
> > -			if (sqlVdbeMemGrow(dest_mem, len + 1, 1) != 0)
> > -				return -1;
> > +	if (mem_is_string(dest_mem) && (dest_mem->flags & MEM_Ephem) != 0) {
> 
> 8. The patch looks incomplete while you still have to check flags manually
> in the code which is not a part of Mem API. This function (vdbe_field_ref_fetch)
> does not look like a part of Mem's API. It rather belongs to Vdbe.
> 
Fixed.

> > +		if (dest_mem->n > 0) {
> > +			mem_copy_str(dest_mem, dest_mem->z, dest_mem->n,
> > +				     (dest_mem->flags & MEM_Term) != 0);
> 
> 9. The same here. Perhaps for this entire block you could add a
> function mem_copy_strmem(dst, src). It would assert inside that the source
> mem is a string object, and would hide all this inside.
> 
I created mem_copy(), however, in this case I changed the funtion which decodes
msgpack.

> >  		} else {
> > -			dest_mem->z =
> > -				memcpy(dest_mem->zMalloc, dest_mem->z, len);
> > -			dest_mem->flags &= ~MEM_Ephem;
> > +			mem_set_str(dest_mem, "", 0, MEM_Static, true);
> >  		}
> > -		dest_mem->z[len] = 0;
> > -		dest_mem->flags |= MEM_Term;
> >  	}
> >  	UPDATE_MAX_BLOBSIZE(dest_mem);
> >  	dest_mem->field_type = vdbe_field_ref_fetch_type(field_ref, fieldno);
> > @@ -1364,19 +1325,17 @@ case OP_String: {          /* out2 */
> >   */
> >  case OP_Null: {           /* out2 */
> >  	int cnt;
> > -	u16 nullFlag;
> >  	pOut = vdbe_prepare_null_out(p, pOp->p2);
> >  	cnt = pOp->p3-pOp->p2;
> >  	assert(pOp->p3<=(p->nMem+1 - p->nCursor));
> > -	pOut->flags = nullFlag = pOp->p1 ? (MEM_Null|MEM_Cleared) : MEM_Null;
> > -	pOut->n = 0;
> > +	if (pOp->p1)
> 
> 10. We use != 0 instead of implicit cast.
> 
Fixed.

> > +		pOut->flags |= MEM_Cleared;
> >  	while( cnt>0) {
> >  		pOut++;
> >  		memAboutToChange(p, pOut);
> 
> 11. Why isn't this included into each mem_set/mem_copy function?
> 
This function works only for MEMs from VDBE. We do not have list of all MEMs.

> > -		sqlVdbeMemSetNull(pOut);
> > -		pOut->flags = nullFlag;
> > -		pOut->field_type = field_type_MAX;
> > -		pOut->n = 0;
> > +		mem_set_null(pOut);
> > +		if (pOp->p1)
> > +			pOut->flags |= MEM_Cleared;
> 
> 12. To help performance you could keep `nullFlag` variable,
> and set it to either 0 or MEM_Cleared. Then in the loop
> you can do `pOut->flags |= nullFlag;` without any branches.
> 
> By the way, the Cleared thing looks related to NULL type. It
> changes its behaviour. Maybe it is worth encapsulating it too.
> It depends on whether you will manage to extract OP_Ge/...
> into a function not depending on Vdbe but only on mems.
> 
> Which would be great as the comparison opcode now is enormous,
> hard to read, and impossible to unit test.
> 
I rewrote this in new version.

> >  		cnt--;
> >  	}
> >  	break;> @@ -1608,7 +1566,7 @@ case OP_Concat: {           /* same as TK_CONCAT, in1, in2, out3 */
> >  	}
> >  
> >  	/* Moreover, both operands must be of the same type. */
> > -	if (str_type_p1 != str_type_p2) {
> > +	if (!mems_have_same_type(pIn1, pIn2)) {
> 
> 13. Better keep it 'mem', not 'mems'. To have all the Mem API functions
> have the same prefix.
> 
Fixed.

> >  		diag_set(ClientError, ER_INCONSISTENT_TYPES,
> >  			 mem_type_to_str(pIn2), mem_type_to_str(pIn1));
> >  		goto abort_due_to_error;
> > @@ -1619,21 +1577,19 @@ case OP_Concat: {           /* same as TK_CONCAT, in1, in2, out3 */
> >  	if (nByte>db->aLimit[SQL_LIMIT_LENGTH]) {
> >  		goto too_big;
> >  	}
> > -	if (sqlVdbeMemGrow(pOut, (int)nByte+2, pOut==pIn2)) {
> > -		goto no_mem;
> > +	size_t svp = region_used(&fiber()->gc);
> > +	char *buf = region_alloc(&fiber()->gc, nByte);
> 
> 14. Recently we confirmed that TLS access might be actually not as fast
> as a normal variable access. Fiber() reads a TLS variable. So if you
> care abot top perf, better cache &fiber()->gc in a variable. Up to you
> though. I can't force such rule because can't (and don't want) to measure
> if it really wins anything.
> 
Thanks. For now I decided to not think too much about perf.

> > +	if (buf == NULL) {
> > +		diag_set(OutOfMemory, nByte, "region_alloc", "buf");
> > +		goto abort_due_to_error;
> >  	}
> > -	if (pIn1->flags & MEM_Str)
> > -		MemSetTypeFlag(pOut, MEM_Str);
> > +	memcpy(buf, pIn2->z, pIn2->n);
> > +	memcpy(&buf[pIn2->n], pIn1->z, pIn1->n);
> > +	if (mem_is_binary(pIn1))
> > +		mem_copy_bin(pOut, buf, nByte, false);
> >  	else
> > -		MemSetTypeFlag(pOut, MEM_Blob);
> > -	if (pOut!=pIn2) {
> > -		memcpy(pOut->z, pIn2->z, pIn2->n);
> > -	}
> > -	memcpy(&pOut->z[pIn2->n], pIn1->z, pIn1->n);
> > -	pOut->z[nByte]=0;
> > -	pOut->z[nByte+1] = 0;
> > -	pOut->flags |= MEM_Term;
> > -	pOut->n = (int)nByte;
> > +		mem_copy_str(pOut, buf, nByte, false);
> > +	region_truncate(&fiber()->gc, svp);
> 
> 15. AFAIU, before your patch it was allocated in place, with sqlVdbeMemGrow()
> used to reserve the space. Why did you change it? Looks slower. Before your
> changes there was 2 copies at most: out mem realloc + second mem copy. Now
> there are 4 copies: copy original + new into new place, copy the 2 copies
> into the out mem.
> 
Rewrote this part.

> >  	UPDATE_MAX_BLOBSIZE(pOut);
> >  	break;
> >  }
> > @@ -1681,27 +1637,25 @@ case OP_Subtract:              /* same as TK_MINUS, in1, in2, out3 */
> >  case OP_Multiply:              /* same as TK_STAR, in1, in2, out3 */
> >  case OP_Divide:                /* same as TK_SLASH, in1, in2, out3 */
> >  case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
> > -	u32 flags;      /* Combined MEM_* flags from both inputs */
> > -	u16 type1;      /* Numeric type of left operand */
> > -	u16 type2;      /* Numeric type of right operand */
> > +	bool is_in1_int;      /* Numeric type of left operand */
> > +	bool is_in2_int;      /* Numeric type of right operand */
> 
> 16. The comments are wrong now.
> 
Rewrote this part.

> >  	i64 iA;         /* Integer value of left operand */
> >  	i64 iB;         /* Integer value of right operand */
> > +	bool is_lhs_neg;
> > +	bool is_rhs_neg;
> >  	double rA;      /* Real value of left operand */
> >  	double rB;      /* Real value of right operand */
> >  
> >  	pIn1 = &aMem[pOp->p1];
> > -	type1 = numericType(pIn1);
> > +	is_in1_int = numericType(pIn1, (int64_t *)&iA, &is_lhs_neg);
> >  	pIn2 = &aMem[pOp->p2];
> > -	type2 = numericType(pIn2);
> > +	is_in2_int = numericType(pIn2, (int64_t *)&iB, &is_rhs_neg);
> >  	pOut = vdbe_prepare_null_out(p, pOp->p3);
> > -	flags = pIn1->flags | pIn2->flags;
> > -	if ((flags & MEM_Null)!=0) goto arithmetic_result_is_null;
> > -	if ((type1 & (MEM_Int | MEM_UInt)) != 0 &&
> > -	    (type2 & (MEM_Int | MEM_UInt)) != 0) {
> > -		iA = pIn1->u.i;
> > -		iB = pIn2->u.i;
> > -		bool is_lhs_neg = pIn1->flags & MEM_Int;
> > -		bool is_rhs_neg = pIn2->flags & MEM_Int;
> > +	if (mem_is_null(pIn1) || mem_is_null(pIn2))
> > +		goto arithmetic_result_is_null;
> > +	if (is_in1_int && is_in2_int) {
> > +		bool is_lhs_neg = mem_is_neg_int(pIn1);
> > +		bool is_rhs_neg = mem_is_neg_int(pIn2);
> 
> 17. You already know is_lhs_neg and is_rhs_neg from the
> numericType() calls above.
> 
True, but this will change test. I was asked to not change behaviour during this
refactoring. I added comment in new version.

> >  		bool is_res_neg;
> >  		switch( pOp->opcode) {
> >  		case OP_Add: {
> > @@ -2265,10 +2212,10 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
> >  			 * or not both operands are null.
> >  			 */
> >  			assert(pOp->opcode==OP_Eq || pOp->opcode==OP_Ne);
> > -			assert((flags1 & MEM_Cleared)==0);
> > +			assert((pIn1->flags & MEM_Cleared)==0);
> >  			assert((pOp->p5 & SQL_JUMPIFNULL)==0);
> > -			if ((flags1&flags3&MEM_Null)!=0
> > -			    && (flags3&MEM_Cleared)==0
> > +			if (mem_is_null(pIn1) && mem_is_null(pIn3)
> > +			    && (pIn3->flags & MEM_Cleared)==0
> 
> 18. This is what I was talking about with Cleared. It seems to be
> an internal flag of Mem which is needed only for the comparison.
> Which means you probably should hide it. With the entire comparison
> opcode code along. We already have sqlMemCompare. Why does not
> the opcode use it? Probably worth doing something with this since
> you are working on the encapsulation.
> 
> To get the comparison code in order and to hide Mem details in it.
> 
Rewrote this part. Not sure that this comment was answered.

> >  				) {
> >  				res = 0;  /* Operands are equal */
> >  			} else {
> > @@ -2314,18 +2261,25 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
> >  		res = sqlMemCompare(pIn3, pIn1, NULL);
> >  	} else {
> >  		enum field_type type = pOp->p5 & FIELD_TYPE_MASK;
> > +		struct Mem tmp_mem1, tmp_mem2, *mem1, *mem2;
> > +		mem1 = pIn1;
> > +		mem2 = pIn3;
> 
> 19. Wtf? Why do you need to copy the mems? The original code worked
> fine without them. It is not just API refactoring then. If this is
> really necessary, lets do it in a separate patch with clear explanation
> why you need it.
> 
Rewrote this part.

> >  		if (sql_type_is_numeric(type)) {
> > -			if ((flags1 | flags3)&MEM_Str) {
> > -				if ((flags1 & MEM_Str) == MEM_Str) {
> > -					mem_apply_numeric_type(pIn1);
> > -					testcase( flags3!=pIn3->flags); /* Possible if pIn1==pIn3 */
> > -					flags3 = pIn3->flags;
> > +			if (mem_is_string(mem1) || mem_is_string(mem2)) {
> > +				if (mem_is_string(mem1)) {
> > +					mem_init(&tmp_mem1);
> 
> 20. Constructors should have 'create' suffix.
> 
Fixed.

> > +					memcpy(&tmp_mem1, mem1, sizeof(*mem1));
> > +					mem1 = &tmp_mem1;
> > +					mem_apply_numeric_type(mem1);
> >  				}
> > -				if ((flags3 & MEM_Str) == MEM_Str) {
> > -					if (mem_apply_numeric_type(pIn3) != 0) {
> > +				if (mem_is_string(mem2)) {
> > +					mem_init(&tmp_mem2);
> > +					memcpy(&tmp_mem2, mem2, sizeof(*mem2));
> > +					mem2 = &tmp_mem2;
> > +					if (mem_apply_numeric_type(mem2) != 0) {
> >  						diag_set(ClientError,
> >  							 ER_SQL_TYPE_MISMATCH,
> > -							 sql_value_to_diag_str(pIn3),
> > +							 sql_value_to_diag_str(mem2),
> >  							 "numeric");
> >  						goto abort_due_to_error;
> >  					}
> > @@ -3860,14 +3797,14 @@ case OP_FCopy: {     /* out2 */
> >  		pIn1 = &aMem[pOp->p1];
> >  	}
> >  
> > -	if ((pOp->p3 & OPFLAG_NOOP_IF_NULL) && (pIn1->flags & MEM_Null)) {
> > +	if ((pOp->p3 & OPFLAG_NOOP_IF_NULL) && mem_is_null(pIn1)) {
> >  		pOut = vdbe_prepare_null_out(p, pOp->p2);
> >  	} else {
> >  		assert(memIsValid(pIn1));
> > -		assert((pIn1->flags & (MEM_Int | MEM_UInt)) != 0);
> > +		assert(mem_is_integer(pIn1));
> >  
> >  		pOut = vdbe_prepare_null_out(p, pOp->p2);
> > -		mem_set_int(pOut, pIn1->u.i, pIn1->flags == MEM_Int);
> > +		mem_set_int(pOut, pIn1->u.i, mem_is_neg_int(pIn1));
> 
> 21. Perhaps it is worth adding mem_set_intmem(Mem* src). You won't
> need to call mem_is_neg_int() then. -1 branch.
> 
Rewrote this part.

> >  		pOut->field_type = pIn1->field_type;
> >  	}
> >  	break;
> > @@ -5252,7 +5187,7 @@ case OP_AggFinal: {
> >  	Mem *pMem;
> >  	assert(pOp->p1>0 && pOp->p1<=(p->nMem+1 - p->nCursor));
> >  	pMem = &aMem[pOp->p1];
> > -	assert((pMem->flags & ~(MEM_Null|MEM_Agg))==0);
> > +	assert(mem_is_null(pMem) || (pMem->flags & MEM_Agg) != 0);
> 
> 22. MEM_Agg says "Mem.z points to an agg function context". So it
> is related to mem types. And probably there must be a helper
> mem_is_aggctx().
> 
Fixed by adding new check functions.

> >  	if (sql_vdbemem_finalize(pMem, pOp->p4.func) != 0)
> >  		goto abort_due_to_error;
> >  	UPDATE_MAX_BLOBSIZE(pMem);
> > diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> > index 7205f1af3..a80a7b511 100644
> > --- a/src/box/sql/vdbeInt.h
> > +++ b/src/box/sql/vdbeInt.h
> > @@ -484,41 +484,455 @@ void sqlVdbeMemMove(Mem *, Mem *);
> >  int sqlVdbeMemNulTerminate(Mem *);
> >  int sqlVdbeMemSetStr(Mem *, const char *, int, u8, void (*)(void *));
> >  
> > +/**
> > + * Memory cell mem contains the context of an aggregate function.
> > + * This routine calls the finalize method for that function. The
> > + * result of the aggregate is stored back into mem.
> > + *
> > + * Returns -1 if the finalizer reports an error. 0 otherwise.
> > + */
> > +int
> > +sql_vdbemem_finalize(struct Mem *mem, struct func *func);
> > +
> > +/*
> 
> 23. Comments out of function bodies should start from /**.
> 
Fixed in new functions. However, I believe I didn't change this in functions
that were just moved.

> > + * If the memory cell contains a value that must be freed by
> > + * invoking the external callback in Mem.xDel, then this routine
> > + * will free that value.  It also sets Mem.flags to MEM_Null.
> > + */
> >  void
> > -mem_set_bool(struct Mem *mem, bool value);
> > +vdbeMemClearExternAndSetNull(Mem * p);
> 
> 24. Please, don't use whitespace after `*`. Here and in the
> next declaration.
> 
Same.

> > +
> > +/*
> > + * Change the pMem->zMalloc allocation to be at least szNew bytes.
> 
> 25. There is no `szNew` argument.
> 
Just copied this function for now, didn't change comments.

> > + * If pMem->zMalloc already meets or exceeds the requested size, this
> > + * routine is a no-op.
> > + *
> > + * Any prior string or blob content in the pMem object may be discarded.
> > + * The pMem->xDel destructor is called, if it exists.  Though MEM_Str
> > + * and MEM_Blob values may be discarded, MEM_Int, MEM_Real, and MEM_Null
> > + * values are preserved.
> > + *
> > + * Return 0 on success or -1 if unable to complete the resizing.
> > + */
> > +int
> > +sqlVdbeMemClearAndResize(Mem * pMem, int n);
> > +
> > +/**
> > + * Initialize a new mem. After initializing the mem will hold a NULL value.
> > + *
> > + * @param mem VDBE memory register to initialize.
> 
> 26. I will remind just in case - using doxygen is not obligatory. Especially
> for such trivial functions.
> 
There is a lot less comments in new version, at least for now...

> > diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> > index 7c59ef83f..338853495 100644
> > --- a/src/box/sql/vdbeapi.c
> > +++ b/src/box/sql/vdbeapi.c
> > @@ -588,39 +588,17 @@ sql_data_count(sql_stmt * pStmt)
> >  static const Mem *
> >  columnNullValue(void)
> >  {
> > -	/* Even though the Mem structure contains an element
> > -	 * of type i64, on certain architectures (x86) with certain compiler
> > -	 * switches (-Os), gcc may align this Mem object on a 4-byte boundary
> > -	 * instead of an 8-byte one. This all works fine, except that when
> > -	 * running with SQL_DEBUG defined the sql code sometimes assert()s
> > -	 * that a Mem structure is located on an 8-byte boundary. To prevent
> > -	 * these assert()s from failing, when building with SQL_DEBUG defined
> > -	 * using gcc, we force nullMem to be 8-byte aligned using the magical
> > -	 * __attribute__((aligned(8))) macro.
> > -	 */
> > -	static const Mem nullMem
> >  #if defined(SQL_DEBUG) && defined(__GNUC__)
> > -	    __attribute__ ((aligned(8)))
> > -#endif
> > -	    = {
> > -		/* .u          = */  {
> > -		0},
> > -		    /* .flags      = */ (u16) MEM_Null,
> > -		    /* .eSubtype   = */ (u8) 0,
> > -		    /* .field_type = */ field_type_MAX,
> > -		    /* .n          = */ (int)0,
> > -		    /* .z          = */ (char *)0,
> > -		    /* .zMalloc    = */ (char *)0,
> > -		    /* .szMalloc   = */ (int)0,
> > -		    /* .uTemp      = */ (u32) 0,
> > -		    /* .db         = */ (sql *) 0,
> > -		    /* .xDel       = */ (void (*)(void *))0,
> > -#ifdef SQL_DEBUG
> > -		    /* .pScopyFrom = */ (Mem *) 0,
> > -		    /* .pFiller    = */ (void *)0,
> > +	static struct Mem nullMem __attribute__ ((aligned(8)));
> > +#else
> > +	static struct Mem nullMem;
> >  #endif
> > -	};
> > -	return &nullMem;
> > +	static struct Mem *null_mem_ptr = NULL;
> > +	if (null_mem_ptr == NULL) {
> > +		mem_init(&nullMem);
> > +		null_mem_ptr = &nullMem;
> > +	}
> > +	return null_mem_ptr;
> 
> 27. The old code was a bit ugly, yeah, but it didn't have
> branches. It is fine to keep the old code, because it is
> internal part of Mem API anyway. At least it should be.
> 
Fixed. Also, at some point I removed this function.

> >  }
> > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> > index 5c2706e49..fb55f82a6 100644
> > --- a/src/box/sql/vdbeaux.c
> > +++ b/src/box/sql/vdbeaux.c
> > @@ -1198,17 +1198,13 @@ sqlVdbePrintOp(FILE * pOut, int pc, Op * pOp)
> >   * Initialize an array of N Mem element.
> >   */
> >  static void
> > -initMemArray(Mem * p, int N, sql * db, u32 flags)
> > +initMemArray(Mem *p, int N, bool is_undefined)
> >  {
> > -	while ((N--) > 0) {
> > -		p->db = db;
> > -		p->flags = flags;
> > -		p->szMalloc = 0;
> > -		p->field_type = field_type_MAX;
> > -#ifdef SQL_DEBUG
> > -		p->pScopyFrom = 0;
> > -#endif
> > -		p++;
> > +	for (int i = 0; i < N; ++i) {
> > +		struct Mem *mem = &p[i];
> > +		mem_init(mem);
> > +		if (is_undefined)
> > +			mem_set_undefined(mem);
> >  	}
> 
> 28. Maybe just inline this entire function.
> 
Inlined.

> >  }
> > @@ -1382,13 +1376,25 @@ sqlVdbeList(Vdbe * p)
> >  					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 (j == nSub) {
> > +					size_t svp = region_used(&fiber()->gc);
> > +					struct SubProgram **buf = (SubProgram **)
> > +						region_aligned_alloc(&fiber()->gc,
> > +								     nByte,
> > +								     alignof(struct SubProgram));
> > +					if (buf == NULL) {
> > +						diag_set(OutOfMemory, nByte,
> > +							 "region_aligned_alloc",
> > +							 "buf");
> > +						p->is_aborted = true;
> > +						return -1;
> > +					}
> > +					if (nSub > 0)
> > +						memcpy(buf, pSub->z, pSub->n);
> > +					buf[nSub++] = pOp->p4.pProgram;
> > +					mem_copy_bin(pSub, (char *)buf, nByte,
> > +						    false);
> > +					region_truncate(&fiber()->gc, svp);
> >  				}
> 
> 29. What was wrong with keeping a 'grow' function? Such changes
> only increase number of copies and pad out the code.
> 
Rewrote this part.

> >  			}
> >  		}
> > @@ -1402,41 +1408,39 @@ sqlVdbeList(Vdbe * p)
> >  		mem_set_i64(pMem, pOp->p3);
> >  		pMem++;
> >  
> > -		if (sqlVdbeMemClearAndResize(pMem, 256)) {
> > -			assert(p->db->mallocFailed);
> > +		size_t size = 256;
> > +		size_t svp = region_used(&fiber()->gc);
> > +		char *buf = (char *)region_alloc(&fiber()->gc, size);
> > +		if (buf == NULL) {
> > +			diag_set(OutOfMemory, size, "region_alloc", "buf");
> > +			p->is_aborted = true;
> >  			return -1;
> >  		}
> > -		pMem->flags = MEM_Str | MEM_Term;
> > -		zP4 = displayP4(pOp, pMem->z, pMem->szMalloc);
> > -
> > -		if (zP4 != pMem->z) {
> > -			pMem->n = 0;
> > -			sqlVdbeMemSetStr(pMem, zP4, -1, 1, 0);
> > -		} else {
> > -			assert(pMem->z != 0);
> > -			pMem->n = sqlStrlen30(pMem->z);
> > -		}
> > +		zP4 = displayP4(pOp, buf, size);
> > +		mem_copy_str(pMem, zP4, strlen(zP4), true);
> > +		region_truncate(&fiber()->gc, svp);
> >  		pMem++;
> >  
> >  		if (p->explain == 1) {
> > -			if (sqlVdbeMemClearAndResize(pMem, 4)) {
> > -				assert(p->db->mallocFailed);
> > -				return -1;
> > -			}
> > -			pMem->flags = MEM_Str | MEM_Term;
> > -			pMem->n = 2;
> > -			sql_snprintf(3, pMem->z, "%.2x", pOp->p5);	/* P5 */
> > +			char *str = (char *)tt_sprintf("%02hu", pOp->p5);
> > +			mem_copy_str(pMem, str, strlen(str), true);
> 
> 30. Probably Timur didn't know, but tt_sprintf() also is static
> alloc. I see no sense in avoiding it if you know what you are doing.
> 
> The same below for 512 byte allocation and above for 256.
> 
> If someone has issues with static alloc, I propose them to create a
> ticket on making its API more safe. For example, introduce static_free()
> and use it after each static_alloc() to prevent unintended overwrites.
> Would be a good idea to increase the safety.
> 
> > @@ -2418,80 +2422,72 @@ sqlBlobCompare(const Mem * pB1, const Mem * pB2)
> >  int
> >  sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl)
> >  {
> > -	int f1, f2;
> > -	int combined_flags;
> > -
> > -	f1 = pMem1->flags;
> > -	f2 = pMem2->flags;
> > -	combined_flags = f1 | f2;
> > -
> >  	/* If one value is NULL, it is less than the other. If both values
> >  	 * are NULL, return 0.
> >  	 */
> > -	if (combined_flags & MEM_Null) {
> > -		return (f2 & MEM_Null) - (f1 & MEM_Null);
> > -	}
> > +	if (mem_is_null(pMem1) || mem_is_null(pMem2))
> > +		return (int)mem_is_null(pMem2) - (int)mem_is_null(pMem1);
> >  
> > -	if ((combined_flags & MEM_Bool) != 0) {
> > -		if ((f1 & f2 & MEM_Bool) != 0) {
> > +	if (mem_is_bool(pMem1) || mem_is_bool(pMem2)) {
> > +		if (mem_is_bool(pMem1) && mem_is_bool(pMem2)) {
> 
> 31. Issue with such changes is that the code is a part of internal
> Mem's code. I can tell it from the code itsef (it works with
> mems only), and from the name (sqlMemCompare). So it could afford
> to touch internal members to achieve the best perf. I think it was
> totally fine as long as it happened only inside of Mem API functions,
> where you are supposed to know meaning of the fields.
> 
> This probably arises from the code being a total mess when Mem
> functions are defined all over the common files and don't have
> their own place. Where you could define big internal functions in
> mem.c, and do there whatever you want.
> 
> The perf issues you are having might also be because of that.
> 
> For instance, in this function 'combined_flags' allowed to do only
> one branch per check. Now you do two. Was 'combined & type_flags'
> became 'mem1 & type_flags || mem2 & type_flags'. `||` is a branch.
> 
Rewrote this part.

> >  			if (pMem1->u.b == pMem2->u.b)
> >  				return 0;
> >  			if (pMem1->u.b)
> >  				return 1;
> >  			return -1;
> >  		}
> > -		if ((f2 & MEM_Bool) != 0)
> > +		if (mem_is_bool(pMem2))
> >  			return +1;
> >  		return -1;
> >  	}
> > @@ -2613,7 +2609,12 @@ sqlVdbeCompareMsgpack(const char **key1,
> >  {
> >  	const char *aKey1 = *key1;
> >  	Mem *pKey2 = unpacked->aMem + key2_idx;
> > -	Mem mem1;
> > +	bool b;
> > +	uint64_t u;
> > +	int64_t i;
> > +	double r;
> > +	const char *s;
> > +	size_t size;
> 
> 32. What was wrong with declaring these variables in-place?
> 
Nothing. In new version this part is reverted, I believe.

> >  	int rc = 0;
> >  	switch (mp_typeof(*aKey1)) {
> >  	default:{
> > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> > index 1101f7205..a9b552b9c 100644
> > --- a/src/box/sql/vdbemem.c
> > +++ b/src/box/sql/vdbemem.c
> > @@ -823,63 +750,6 @@ sqlVdbeMemSetZeroBlob(Mem * pMem, int n)
> >  	pMem->z = 0;
> >  }
> >  
> > -void
> > -mem_set_bool(struct Mem *mem, bool value)
> > -{
> > -	sqlVdbeMemSetNull(mem);
> > -	mem->u.b = value;
> > -	mem->flags = MEM_Bool;
> > -	mem->field_type = FIELD_TYPE_BOOLEAN;
> > -}
> > -
> > -void
> > -mem_set_i64(struct Mem *mem, int64_t value)
> > -{
> > -	if (VdbeMemDynamic(mem))
> > -		sqlVdbeMemSetNull(mem);
> > -	mem->u.i = value;
> > -	int flag = value < 0 ? MEM_Int : MEM_UInt;
> > -	MemSetTypeFlag(mem, flag);
> > -	mem->field_type = FIELD_TYPE_INTEGER;
> > -}
> > -
> > -void
> > -mem_set_u64(struct Mem *mem, uint64_t value)
> > -{
> > -	if (VdbeMemDynamic(mem))
> > -		sqlVdbeMemSetNull(mem);
> > -	mem->u.u = value;
> > -	MemSetTypeFlag(mem, MEM_UInt);
> > -	mem->field_type = FIELD_TYPE_UNSIGNED;
> > -}
> > -
> > -void
> > -mem_set_int(struct Mem *mem, int64_t value, bool is_neg)
> > -{
> > -	if (VdbeMemDynamic(mem))
> > -		sqlVdbeMemSetNull(mem);
> > -	if (is_neg) {
> > -		assert(value < 0);
> > -		mem->u.i = value;
> > -		MemSetTypeFlag(mem, MEM_Int);
> > -	} else {
> > -		mem->u.u = value;
> > -		MemSetTypeFlag(mem, MEM_UInt);
> > -	}
> > -	mem->field_type = FIELD_TYPE_INTEGER;
> > -}
> > -
> > -void
> > -mem_set_double(struct Mem *mem, double value)
> > -{
> > -	sqlVdbeMemSetNull(mem);
> > -	if (sqlIsNaN(value))
> > -		return;
> > -	mem->u.r = value;
> > -	MemSetTypeFlag(mem, MEM_Real);
> > -	mem->field_type = FIELD_TYPE_DOUBLE;
> > -}
> 
> 33. So these functions were defined in C file, and there was no
> perf issues with them. Doesn't it mean something became wrong just
> now?
> 
I believe that most part of perf changes happened due to mem_is_*() functions.
Still, in new version I was told to not think too much about perf, at least for
now.

> > -
> >  /*
> >   * Return true if the Mem object contains a TEXT or BLOB that is
> >   * too large - whose size exceeds SQL_MAX_LENGTH.
> Overall I think you need to create a plan how to solve the Mem
> mess.
> 
> A first step would be to locate all Mem API functions. It is not
> clear now which of them are about Mem only, and which just happened
> to have 'mem' in their name. And which don't have 'mem' in their name
> but are a part of Mem API.
> 
> Then I would propose to move all the mem code to its own couple of
> files if possible. If not - break the dependencies of its functions
> from the other subsystems and modules, and then move.
> 
> Then use proper names for the existing functions.
> 
> Then introduce new functions.
> 
> This way it should be easier to see which functions are private (they
> can touch flags, do combined_flags, do various hacks on Mems, but can't
> be used out of mem.c and mem.h), and which are public (the other functions).
> 
> This would do the real encapsulation.
I believe I did this, however, I was not very strict with order. Maybe this
should be fixed in next versions.


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-03-23 13:07 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-20 16:59 [Tarantool-patches] [PATCH v3 0/2] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
2021-02-20 16:59 ` [Tarantool-patches] [PATCH v3 1/2] sql: Initialize MEM in sqlVdbeAllocUnpackedRecord() Mergen Imeev via Tarantool-patches
2021-02-20 16:59 ` [Tarantool-patches] [PATCH v3 2/2] sql: Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
2021-02-28 17:35   ` Vladislav Shpilevoy via Tarantool-patches
2021-03-23 13:07     ` Mergen Imeev via Tarantool-patches
2021-02-28 17:35 ` [Tarantool-patches] [PATCH v3 0/2] " Vladislav Shpilevoy via Tarantool-patches
2021-03-23 12:34   ` Mergen Imeev via Tarantool-patches

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git