Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking
@ 2021-02-01  8:14 Mergen Imeev via Tarantool-patches
  2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 01/10] sql: introduce mem_set_*() functions Mergen Imeev via Tarantool-patches
                   ` (10 more replies)
  0 siblings, 11 replies; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-01  8:14 UTC (permalink / raw)
  To: s.ostanevich, tsafin; +Cc: tarantool-patches

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

https://github.com/tarantool/tarantool/issues/4470
https://github.com/tarantool/tarantool/tree/imeevma/gh-4470-encapsule-type-setting-v3


Mergen Imeev (10):
  sql: introduce mem_set_*() functions
  sql: Initialize MEM in sqlVdbeAllocUnpackedRecord()
  sql: introduce mem_is_*() functions
  sql: introduce mem_convert_to_binary()
  sql: refactor vdbesort.c
  sql: refactor sql/func.c
  sql: refactor vdbetrace.c
  sql: refactor vdbeapi.c
  sql: refactor vdbeaux.c
  sql: refactor vdbe.c

 src/box/sql/func.c      |   2 +-
 src/box/sql/vdbe.c      | 453 ++++++++++++++++++----------------------
 src/box/sql/vdbeInt.h   | 204 ++++++++++++++++++
 src/box/sql/vdbeapi.c   |  53 ++---
 src/box/sql/vdbeaux.c   | 268 +++++++++++-------------
 src/box/sql/vdbemem.c   | 116 ++++++++++
 src/box/sql/vdbesort.c  |   9 +-
 src/box/sql/vdbetrace.c |  12 +-
 8 files changed, 674 insertions(+), 443 deletions(-)

-- 
2.25.1


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

* [Tarantool-patches] [PATCH v1 01/10] sql: introduce mem_set_*() functions
  2021-02-01  8:14 [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
@ 2021-02-01  8:14 ` Mergen Imeev via Tarantool-patches
  2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 02/10] sql: Initialize MEM in sqlVdbeAllocUnpackedRecord() Mergen Imeev via Tarantool-patches
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-01  8:14 UTC (permalink / raw)
  To: s.ostanevich, tsafin; +Cc: tarantool-patches

This patch add some new mem_set_*() functions. These functions will be
used to change value of a MEM. Also, function mem_init() is introduced.
This function is used to initialize a newly created MEM.

Part of #4470
---
 src/box/sql/vdbeInt.h | 101 ++++++++++++++++++++++++++++++++++++++++++
 src/box/sql/vdbemem.c | 101 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 202 insertions(+)

diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 7205f1af3..a24fbfc0e 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -484,6 +484,30 @@ void sqlVdbeMemMove(Mem *, Mem *);
 int sqlVdbeMemNulTerminate(Mem *);
 int sqlVdbeMemSetStr(Mem *, const char *, int, u8, void (*)(void *));
 
+/**
+ * Initialize a new mem. After initializing the mem will hold a NULL value.
+ *
+ * @param mem VDBE memory register to initialize.
+ */
+void
+mem_init(struct Mem *mem);
+
+/**
+ * Set VDBE memory register as NULL.
+ *
+ * @param mem VDBE memory register to update.
+ */
+void
+mem_set_null(struct Mem *mem);
+
+/**
+ * Set VDBE memory register as Undefined. MEM is not cleared prior to that.
+ *
+ * @param mem VDBE memory register to update.
+ */
+void
+mem_set_undefined(struct Mem *mem);
+
 void
 mem_set_bool(struct Mem *mem, bool value);
 
@@ -495,6 +519,15 @@ mem_set_bool(struct Mem *mem, bool value);
 void
 mem_set_ptr(struct Mem *mem, void *ptr);
 
+/**
+ * Set VDBE memory register as frame.
+ *
+ * @param mem VDBE memory register to update.
+ * @param frame Frame to set.
+ */
+void
+mem_set_frame(struct Mem *mem, struct VdbeFrame *frame);
+
 /**
  * Set integer value. Depending on its sign MEM_Int (in case
  * of negative value) or MEM_UInt flag is set.
@@ -517,6 +550,74 @@ mem_set_int(struct Mem *mem, int64_t value, bool is_neg);
 void
 mem_set_double(struct Mem *mem, double value);
 
+/**
+ * Set VDBE memory register as string. Length and way of allocation is also
+ * given. If is_null_terminated is true then sizeof(value) should be len + 1,
+ * otherwise sizeof(value) == len. If alloc_type is either MEM_Static or
+ * MEM_Ephem, then no more allocation, deallocation, or copying is required.
+ * In case it is MEM_Dyn, no need to allocate and copy, but MEM should be freed
+ * each time MEM changes or destroyed. If it is 0, the entire string should be
+ * copied into newly allocated memory, which should be freed when the memory is
+ * destroyed. However, in this case, there is no need to free memory every time
+ * the MEM changes. We only need to free memory if this MEM is given a new
+ * string or binary string with alloc_type 0.
+ *
+ * @param mem VDBE memory register to update.
+ * @param value String to set.
+ * @param len Length of the string.
+ * @param alloc_type Type of allocation of binary string.
+ * @param is_null_terminated True if string is NULL-terminated.
+ */
+int
+mem_set_str(struct Mem *mem, char *value, uint32_t len, int alloc_type,
+	    bool is_null_terminated);
+
+/**
+ * Set VDBE memory register as binary. Length and way of allocation is also
+ * given. If is_zero is true then binary received from this MEM may have length
+ * more than given size, however this is done outside of this MEM. If alloc_type
+ * is either MEM_Static or MEM_Ephem, then no more allocation, deallocation, or
+ * copying is required. In case it is MEM_Dyn, no need to allocate and copy, but
+ * MEM should be freed each time MEM changes or destroyed. If it is 0, the
+ * entire binary string should be copied into newly allocated memory, which
+ * should be freed when the memory is destroyed. However, in this case, there is
+ * no need to free memory every time the MEM changes. We only need to free
+ * memory if this MEM is given a new string or binary string with alloc_type 0.
+ *
+ * @param mem VDBE memory register to update.
+ * @param value Binary string to set.
+ * @param len Length of the string.
+ * @param alloc_type Type of allocation of binary string.
+ * @param is_zero True if binary string may be expanded with zeroes at the end.
+ */
+int
+mem_set_bin(struct Mem *mem, char *value, uint32_t size, int alloc_type,
+	    bool is_zero);
+
+/**
+ * Set VDBE memory register as MAP. See @a mem_set_bin with is_zero = 0 for
+ * more.
+ *
+ * @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 of binary string.
+ */
+int
+mem_set_map(struct Mem *mem, char *value, uint32_t size, int alloc_type);
+
+/**
+ * Set VDBE memory register as ARRAY. See @a mem_set_bin with is_zero = 0 for
+ * more.
+ *
+ * @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 of binary string.
+ */
+int
+mem_set_array(struct Mem *mem, char *value, uint32_t size, int alloc_type);
+
 void sqlVdbeMemInit(Mem *, sql *, u32);
 void sqlVdbeMemSetNull(Mem *);
 void sqlVdbeMemSetZeroBlob(Mem *, int);
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 1101f7205..186aebe03 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -799,6 +799,29 @@ sqlValueSetNull(sql_value * p)
 	sqlVdbeMemSetNull((Mem *) p);
 }
 
+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;
+}
+
+void
+mem_set_null(struct Mem *mem)
+{
+	sqlVdbeMemSetNull(mem);
+	mem->field_type = field_type_MAX;
+}
+
+void
+mem_set_undefined(struct Mem *mem)
+{
+	mem->flags = MEM_Undefined;
+	mem->field_type = field_type_MAX;
+}
+
 void
 mem_set_ptr(struct Mem *mem, void *ptr)
 {
@@ -807,6 +830,15 @@ mem_set_ptr(struct Mem *mem, void *ptr)
 	mem->u.p = ptr;
 }
 
+void
+mem_set_frame(struct Mem *mem, struct VdbeFrame *frame)
+{
+	sqlVdbeMemSetNull(mem);
+	mem->u.pFrame = frame;
+	mem->flags = MEM_Frame;
+	mem->field_type = field_type_MAX;
+}
+
 /*
  * Delete any previous value and set the value to be a BLOB of length
  * n containing all zeros.
@@ -880,6 +912,75 @@ mem_set_double(struct Mem *mem, double value)
 	mem->field_type = FIELD_TYPE_DOUBLE;
 }
 
+int
+mem_set_str(struct Mem *mem, char *value, uint32_t len, int alloc_type,
+	    bool is_null_terminated)
+{
+	sqlVdbeMemSetNull(mem);
+	if (alloc_type != 0) {
+		mem->z = value;
+		if (alloc_type == MEM_Dyn)
+			mem->xDel = SQL_DYNAMIC;
+	} else {
+		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 | alloc_type;
+	if (is_null_terminated)
+		mem->flags |= MEM_Term;
+	mem->field_type = FIELD_TYPE_STRING;
+	return 0;
+}
+
+int
+mem_set_bin(struct Mem *mem, char *value, uint32_t size, int alloc_type,
+	    bool is_zero)
+{
+	sqlVdbeMemSetNull(mem);
+	if (alloc_type != 0) {
+		mem->z = value;
+		if (alloc_type == MEM_Dyn)
+			mem->xDel = SQL_DYNAMIC;
+	} else {
+		if (sqlVdbeMemClearAndResize(mem, size) != 0)
+			return -1;
+		memcpy(mem->z, value, size);
+	}
+	mem->n = size;
+	mem->flags = MEM_Blob | alloc_type;
+	if (is_zero)
+		mem->flags |= MEM_Zero;
+	mem->field_type = FIELD_TYPE_VARBINARY;
+	return 0;
+}
+
+int
+mem_set_map(struct Mem *mem, char *value, uint32_t size, int alloc_type)
+{
+	assert(mp_typeof(*value) == MP_MAP);
+	if (mem_set_bin(mem, value, size, alloc_type, false) != 0)
+		return -1;
+	mem->subtype = SQL_SUBTYPE_MSGPACK;
+	mem->flags |= MEM_Subtype;
+	mem->field_type = FIELD_TYPE_MAP;
+	return 0;
+}
+
+int
+mem_set_array(struct Mem *mem, char *value, uint32_t size, int alloc_type)
+{
+	assert(mp_typeof(*value) == MP_ARRAY);
+	if (mem_set_bin(mem, value, size, alloc_type, false) != 0)
+		return -1;
+	mem->subtype = SQL_SUBTYPE_MSGPACK;
+	mem->flags |= MEM_Subtype;
+	mem->field_type = FIELD_TYPE_ARRAY;
+	return 0;
+}
+
 /*
  * Return true if the Mem object contains a TEXT or BLOB that is
  * too large - whose size exceeds SQL_MAX_LENGTH.
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v1 02/10] sql: Initialize MEM in sqlVdbeAllocUnpackedRecord()
  2021-02-01  8:14 [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
  2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 01/10] sql: introduce mem_set_*() functions Mergen Imeev via Tarantool-patches
@ 2021-02-01  8:14 ` Mergen Imeev via Tarantool-patches
  2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 03/10] sql: introduce mem_is_*() functions Mergen Imeev via Tarantool-patches
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-01  8:14 UTC (permalink / raw)
  To: s.ostanevich, 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 #4470
---
 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..7b8a1e1d8 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)
+		mem_init(&p->aMem[i]);
 	p->key_def = key_def;
 	p->nField = key_def->part_count + 1;
 	return p;
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v1 03/10] sql: introduce mem_is_*() functions
  2021-02-01  8:14 [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
  2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 01/10] sql: introduce mem_set_*() functions Mergen Imeev via Tarantool-patches
  2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 02/10] sql: Initialize MEM in sqlVdbeAllocUnpackedRecord() Mergen Imeev via Tarantool-patches
@ 2021-02-01  8:14 ` Mergen Imeev via Tarantool-patches
  2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 04/10] sql: introduce mem_convert_to_binary() Mergen Imeev via Tarantool-patches
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-01  8:14 UTC (permalink / raw)
  To: s.ostanevich, tsafin; +Cc: tarantool-patches

This patch introduces mem_is_*() functions. These functions are used to
check type of MEM content.

Part of #4470
---
 src/box/sql/vdbeInt.h | 95 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index a24fbfc0e..0bb045170 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -618,6 +618,101 @@ mem_set_map(struct Mem *mem, char *value, uint32_t size, int alloc_type);
 int
 mem_set_array(struct Mem *mem, char *value, uint32_t size, int alloc_type);
 
+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_is_neg_int(mem) || mem_is_pos_int(mem);
+}
+
+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_is_integer(mem) || mem_is_double(mem);
+}
+
+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_is_string(mem) || mem_is_binary(mem);
+}
+
+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);
+}
+
 void sqlVdbeMemInit(Mem *, sql *, u32);
 void sqlVdbeMemSetNull(Mem *);
 void sqlVdbeMemSetZeroBlob(Mem *, int);
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v1 04/10] sql: introduce mem_convert_to_binary()
  2021-02-01  8:14 [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
                   ` (2 preceding siblings ...)
  2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 03/10] sql: introduce mem_is_*() functions Mergen Imeev via Tarantool-patches
@ 2021-02-01  8:14 ` Mergen Imeev via Tarantool-patches
  2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 05/10] sql: refactor vdbesort.c Mergen Imeev via Tarantool-patches
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-01  8:14 UTC (permalink / raw)
  To: s.ostanevich, tsafin; +Cc: tarantool-patches

This patch introduces function mem_convert_to_binary() which allows us
to unify a method to convert a MEM to a MEM of type VARBINARY.

Part of #4470
---
 src/box/sql/vdbeInt.h |  8 ++++++++
 src/box/sql/vdbemem.c | 15 +++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 0bb045170..213531d05 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -713,6 +713,14 @@ mems_have_same_type(const struct Mem *mem1, const struct Mem *mem2)
 	       (mem2->flags & MEM_PURE_TYPE_MASK);
 }
 
+/**
+ * Cast MEM to varbinary according to explicit cast rules.
+ *
+ * @param mem VDBE memory register to convert.
+ */
+int
+mem_convert_to_binary(struct Mem *mem);
+
 void sqlVdbeMemInit(Mem *, sql *, u32);
 void sqlVdbeMemSetNull(Mem *);
 void sqlVdbeMemSetZeroBlob(Mem *, int);
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 186aebe03..849d490b8 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -981,6 +981,21 @@ mem_set_array(struct Mem *mem, char *value, uint32_t size, int alloc_type)
 	return 0;
 }
 
+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;
+}
+
 /*
  * Return true if the Mem object contains a TEXT or BLOB that is
  * too large - whose size exceeds SQL_MAX_LENGTH.
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v1 05/10] sql: refactor vdbesort.c
  2021-02-01  8:14 [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
                   ` (3 preceding siblings ...)
  2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 04/10] sql: introduce mem_convert_to_binary() Mergen Imeev via Tarantool-patches
@ 2021-02-01  8:14 ` Mergen Imeev via Tarantool-patches
  2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 06/10] sql: refactor sql/func.c Mergen Imeev via Tarantool-patches
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-01  8:14 UTC (permalink / raw)
  To: s.ostanevich, tsafin; +Cc: tarantool-patches

---
 src/box/sql/vdbesort.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/src/box/sql/vdbesort.c b/src/box/sql/vdbesort.c
index a2d681255..46759e591 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_set_bin(pOut, pKey, nKey, 0, 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;
 		}
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v1 06/10] sql: refactor sql/func.c
  2021-02-01  8:14 [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
                   ` (4 preceding siblings ...)
  2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 05/10] sql: refactor vdbesort.c Mergen Imeev via Tarantool-patches
@ 2021-02-01  8:15 ` Mergen Imeev via Tarantool-patches
  2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 07/10] sql: refactor vdbetrace.c Mergen Imeev via Tarantool-patches
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-01  8:15 UTC (permalink / raw)
  To: s.ostanevich, tsafin; +Cc: tarantool-patches

---
 src/box/sql/func.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index f15d27051..3fd1cabec 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;
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v1 07/10] sql: refactor vdbetrace.c
  2021-02-01  8:14 [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
                   ` (5 preceding siblings ...)
  2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 06/10] sql: refactor sql/func.c Mergen Imeev via Tarantool-patches
@ 2021-02-01  8:15 ` Mergen Imeev via Tarantool-patches
  2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 08/10] sql: refactor vdbeapi.c Mergen Imeev via Tarantool-patches
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-01  8:15 UTC (permalink / raw)
  To: s.ostanevich, tsafin; +Cc: tarantool-patches

---
 src/box/sql/vdbetrace.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

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] 17+ messages in thread

* [Tarantool-patches] [PATCH v1 08/10] sql: refactor vdbeapi.c
  2021-02-01  8:14 [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
                   ` (6 preceding siblings ...)
  2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 07/10] sql: refactor vdbetrace.c Mergen Imeev via Tarantool-patches
@ 2021-02-01  8:15 ` Mergen Imeev via Tarantool-patches
  2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 09/10] sql: refactor vdbeaux.c Mergen Imeev via Tarantool-patches
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-01  8:15 UTC (permalink / raw)
  To: s.ostanevich, tsafin; +Cc: tarantool-patches

---
 src/box/sql/vdbeapi.c | 53 ++++++++++++-------------------------------
 1 file changed, 15 insertions(+), 38 deletions(-)

diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 7c59ef83f..54ec113b3 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 == 0);
 			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) {
@@ -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;
 }
 
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v1 09/10] sql: refactor vdbeaux.c
  2021-02-01  8:14 [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
                   ` (7 preceding siblings ...)
  2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 08/10] sql: refactor vdbeapi.c Mergen Imeev via Tarantool-patches
@ 2021-02-01  8:15 ` Mergen Imeev via Tarantool-patches
  2021-02-09  9:51   ` [Tarantool-patches] FW: " Timur Safin via Tarantool-patches
  2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 10/10] sql: refactor vdbe.c Mergen Imeev via Tarantool-patches
  2021-02-09  9:36 ` [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Timur Safin via Tarantool-patches
  10 siblings, 1 reply; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-01  8:15 UTC (permalink / raw)
  To: s.ostanevich, tsafin; +Cc: tarantool-patches

---
 src/box/sql/vdbeaux.c | 266 +++++++++++++++++++-----------------------
 1 file changed, 122 insertions(+), 144 deletions(-)

diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 7b8a1e1d8..4ffb34a4e 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_set_bin(pSub, (char *)buf, nByte, 0,
+						    false);
+					region_truncate(&fiber()->gc, svp);
 				}
 			}
 		}
@@ -1402,41 +1408,26 @@ sqlVdbeList(Vdbe * p)
 		mem_set_i64(pMem, pOp->p3);
 		pMem++;
 
-		if (sqlVdbeMemClearAndResize(pMem, 256)) {
-			assert(p->db->mallocFailed);
-			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);
-		}
+		size_t size = 256;
+		char *tmp_buf = (char *) static_alloc(size);
+		assert(tmp_buf != NULL);
+		zP4 = displayP4(pOp, tmp_buf, size);
+		mem_set_str(pMem, zP4, strlen(zP4), 0, true);
 		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_set_str(pMem, str, strlen(str), 0, true);
 			pMem++;
 
 #ifdef SQL_ENABLE_EXPLAIN_COMMENTS
-			if (sqlVdbeMemClearAndResize(pMem, 500)) {
-				assert(p->db->mallocFailed);
-				return -1;
-			}
-			pMem->flags = MEM_Str | MEM_Term;
-			pMem->n = displayComment(pOp, zP4, pMem->z, 500);
+			size = 512;
+			tmp_buf = (char *) static_alloc(size);
+			assert(tmp_buf != NULL);
+			displayComment(pOp, zP4, tmp_buf, size);
+			mem_set_str(pMem, tmp_buf, strlen(tmp_buf), 0, true);
 #else
-			pMem->flags = MEM_Null;	/* Comment */
+			mem_set_null(pMem);
 #endif
 		}
 
@@ -1658,9 +1649,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 +1778,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
 
@@ -2418,80 +2409,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 +2487,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 +2578,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);
@@ -2614,6 +2597,7 @@ sqlVdbeCompareMsgpack(const char **key1,
 	const char *aKey1 = *key1;
 	Mem *pKey2 = unpacked->aMem + key2_idx;
 	Mem mem1;
+	mem_init(&mem1);
 	int rc = 0;
 	switch (mp_typeof(*aKey1)) {
 	default:{
@@ -2622,35 +2606,35 @@ 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 (mem_is_bool(pKey2)) {
 				if (mem1.u.b != pKey2->u.b)
 					rc = mem1.u.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) {
+			if (mem_is_neg_int(pKey2)) {
 				rc = +1;
-			} else if ((pKey2->flags & MEM_UInt) != 0) {
+			} else if (mem_is_pos_int(pKey2)) {
 				if (mem1.u.u < pKey2->u.u)
 					rc = -1;
 				else if (mem1.u.u > pKey2->u.u)
 					rc = +1;
-			} else if ((pKey2->flags & MEM_Real) != 0) {
+			} else if (mem_is_double(pKey2)) {
 				rc = double_compare_uint64(pKey2->u.r,
 							   mem1.u.u, -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;
@@ -2659,20 +2643,20 @@ sqlVdbeCompareMsgpack(const char **key1,
 		}
 	case MP_INT:{
 			mem1.u.i = mp_decode_int(&aKey1);
-			if ((pKey2->flags & MEM_UInt) != 0) {
+			if (mem_is_pos_int(pKey2)) {
 				rc = -1;
-			} else if ((pKey2->flags & MEM_Int) != 0) {
+			} else if (mem_is_neg_int(pKey2)) {
 				if (mem1.u.i < pKey2->u.i) {
 					rc = -1;
 				} else if (mem1.u.i > pKey2->u.i) {
 					rc = +1;
 				}
-			} else if (pKey2->flags & MEM_Real) {
+			} else if (mem_is_double(pKey2)) {
 				rc = double_compare_nint64(pKey2->u.r, mem1.u.i,
 							   -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;
@@ -2686,21 +2670,21 @@ sqlVdbeCompareMsgpack(const char **key1,
 	case MP_DOUBLE:{
 			mem1.u.r = mp_decode_double(&aKey1);
  do_float:
-			if ((pKey2->flags & MEM_Int) != 0) {
+			if (mem_is_neg_int(pKey2)) {
 				rc = double_compare_nint64(mem1.u.r, pKey2->u.i,
 							   1);
-			} else if (pKey2->flags & MEM_UInt) {
+			} else if (mem_is_pos_int(pKey2)) {
 				rc = double_compare_uint64(mem1.u.r,
 							   pKey2->u.u, 1);
-			} else if (pKey2->flags & MEM_Real) {
+			} else if (mem_is_double(pKey2)) {
 				if (mem1.u.r < pKey2->u.r) {
 					rc = -1;
 				} else if (mem1.u.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,31 +2692,35 @@ 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;
+				uint32_t size = mp_decode_strl(&aKey1);
+				char *val = (char *)aKey1;
+				aKey1 += size;
 				struct coll *coll =
 					key_def->parts[key2_idx].coll;
 				if (coll != NULL) {
-					mem1.flags = MEM_Str;
+					mem_set_str(&mem1, val, size, MEM_Ephem,
+						    false);
 					rc = vdbeCompareMemString(&mem1, pKey2,
 								  coll);
 				} else {
+					mem_set_bin(&mem1, val, size, MEM_Ephem,
+						    false);
 					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;
+			uint32_t size = mp_decode_binl(&aKey1);
+			mem_set_bin(&mem1, (char *)aKey1, size, MEM_Ephem,
+				    false);
+			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)) {
@@ -2804,48 +2792,39 @@ vdbe_decode_msgpack_into_mem(const char *buf, struct Mem *mem, uint32_t *len)
 	}
 	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;
 	}
 	}
@@ -2872,10 +2851,9 @@ sqlVdbeRecordUnpackMsgpack(struct key_def *key_def,	/* Information about the rec
 		vdbe_decode_msgpack_into_mem(zParse, pMem, &sz);
 		if (sz == 0) {
 			/* MsgPack array, map or ext. Treat as blob. */
-			pMem->z = (char *)zParse;
+			char *buf = (char *)zParse;
 			mp_next(&zParse);
-			pMem->n = zParse - pMem->z;
-			pMem->flags = MEM_Blob | MEM_Ephem;
+			mem_set_bin(pMem, buf, zParse - buf, MEM_Ephem, false);
 		} else {
 			zParse += sz;
 		}
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v1 10/10] sql: refactor vdbe.c
  2021-02-01  8:14 [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
                   ` (8 preceding siblings ...)
  2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 09/10] sql: refactor vdbeaux.c Mergen Imeev via Tarantool-patches
@ 2021-02-01  8:15 ` Mergen Imeev via Tarantool-patches
       [not found]   ` <047f01d6fec7$b5a90bb0$20fb2310$@tarantool.org>
  2021-02-09  9:36 ` [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Timur Safin via Tarantool-patches
  10 siblings, 1 reply; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-01  8:15 UTC (permalink / raw)
  To: s.ostanevich, tsafin; +Cc: tarantool-patches

---
 src/box/sql/vdbe.c | 453 +++++++++++++++++++++------------------------
 1 file changed, 206 insertions(+), 247 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 1707c216e..7d4a0b297 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,21 @@ 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 struct Mem *
+SQL_NOINLINE computeNumericType(struct Mem *pMem, struct Mem *tmp_mem)
 {
-	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;
+	assert(!mem_is_number(pMem));
+	assert(mem_is_varstring(pMem));
+	double r;
+	if (sqlAtoF(pMem->z, &r, pMem->n)==0)
+		return pMem;
 	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;
+	int64_t i;
+	if (sql_atoi64(pMem->z, &i, &is_neg, pMem->n) == 0)
+		mem_set_int(tmp_mem, i, is_neg);
+	else
+		mem_set_double(tmp_mem, r);
+	return tmp_mem;
 }
 
 /*
@@ -547,14 +542,13 @@ 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 struct Mem *
+numericType(struct Mem *pMem, struct Mem *tmp_mem)
 {
-	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_varstring(pMem)) {
+		return computeNumericType(pMem, tmp_mem);
 	}
-	return 0;
+	return pMem;
 }
 
 #ifdef SQL_DEBUG
@@ -568,7 +562,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 +598,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 +640,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];
@@ -731,35 +723,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;
 }
@@ -884,29 +873,20 @@ vdbe_field_ref_fetch(struct vdbe_field_ref *field_ref, uint32_t fieldno,
 	 * 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;
+		uint32_t size =  vdbe_field_ref_fetch_data(field_ref,
+							   fieldno + 1) - data;
+		if (mp_typeof(*data) == MP_MAP)
+			mem_set_map(dest_mem, (char *)data, size, MEM_Ephem);
+		else
+			mem_set_array(dest_mem, (char *)data, size, MEM_Ephem);
 	}
-	/*
-	 * 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_set_str(dest_mem, dest_mem->z, dest_mem->n, 0,
+				    (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 +1109,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 +1148,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 +1322,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 +1342,18 @@ 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;
+	mem_set_null(pOut);
+	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 +1367,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 +1525,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 +1565,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 +1574,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 +1584,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 +1595,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_set_bin(pOut, buf, nByte, 0, 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_set_str(pOut, buf, nByte, 0, false);
+	region_truncate(&fiber()->gc, svp);
 	UPDATE_MAX_BLOBSIZE(pOut);
 	break;
 }
@@ -1681,27 +1655,26 @@ 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 */
 	i64 iA;         /* Integer value of left operand */
 	i64 iB;         /* Integer value of right operand */
 	double rA;      /* Real value of left operand */
 	double rB;      /* Real value of right operand */
 
+	struct Mem tmp_mem1, tmp_mem2, *mem1, *mem2;
+	mem_init(&tmp_mem1);
+	mem_init(&tmp_mem2);
 	pIn1 = &aMem[pOp->p1];
-	type1 = numericType(pIn1);
+	mem1 = numericType(&aMem[pOp->p1], &tmp_mem1);
 	pIn2 = &aMem[pOp->p2];
-	type2 = numericType(pIn2);
+	mem2 = numericType(&aMem[pOp->p2], &tmp_mem2);
 	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(mem1) || mem_is_null(mem2))
+		goto arithmetic_result_is_null;
+	if (mem_is_integer(mem1) && mem_is_integer(mem2)) {
+		iA = mem1->u.i;
+		iB = mem2->u.i;
+		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 +1725,7 @@ 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);
+		assert(mem_is_double(mem1) || mem_is_double(mem2));
 		switch( pOp->opcode) {
 		case OP_Add:         rB += rA;       break;
 		case OP_Subtract:    rB -= rA;       break;
@@ -1908,7 +1881,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 +1940,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 +1990,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 +2049,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 +2063,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 +2089,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;
@@ -2257,7 +2230,7 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 	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
@@ -2267,7 +2240,7 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 			assert(pOp->opcode==OP_Eq || pOp->opcode==OP_Ne);
 			assert((flags1 & MEM_Cleared)==0);
 			assert((pOp->p5 & SQL_JUMPIFNULL)==0);
-			if ((flags1&flags3&MEM_Null)!=0
+			if (mem_is_null(pIn1) && mem_is_null(pIn3)
 			    && (flags3&MEM_Cleared)==0
 				) {
 				res = 0;  /* Operands are equal */
@@ -2291,20 +2264,20 @@ 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 ?
+			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,
@@ -2315,13 +2288,13 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 	} else {
 		enum field_type type = pOp->p5 & FIELD_TYPE_MASK;
 		if (sql_type_is_numeric(type)) {
-			if ((flags1 | flags3)&MEM_Str) {
-				if ((flags1 & MEM_Str) == MEM_Str) {
+			if (mem_is_string(pIn1) || mem_is_string(pIn3)) {
+				if (mem_is_string(pIn1)) {
 					mem_apply_numeric_type(pIn1);
 					testcase( flags3!=pIn3->flags); /* Possible if pIn1==pIn3 */
 					flags3 = pIn3->flags;
 				}
-				if ((flags3 & MEM_Str) == MEM_Str) {
+				if (mem_is_string(pIn3)) {
 					if (mem_apply_numeric_type(pIn3) != 0) {
 						diag_set(ClientError,
 							 ER_SQL_TYPE_MISMATCH,
@@ -2335,8 +2308,9 @@ 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 (mem_is_integer(pIn1) && mem_is_integer(pIn3)) {
+				if (mem_is_neg_int(pIn1) &&
+				    mem_is_neg_int(pIn3)) {
 					if (pIn3->u.i > pIn1->u.i)
 						res = +1;
 					else if (pIn3->u.i < pIn1->u.i)
@@ -2345,7 +2319,8 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 						res = 0;
 					goto compare_op;
 				}
-				if ((pIn1->flags & pIn3->flags & MEM_UInt) != 0) {
+				if (mem_is_pos_int(pIn1) &&
+				    mem_is_pos_int(pIn3)) {
 					if (pIn3->u.u > pIn1->u.u)
 						res = +1;
 					else if (pIn3->u.u < pIn1->u.u)
@@ -2354,8 +2329,8 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 						res = 0;
 					goto compare_op;
 				}
-				if ((pIn1->flags & MEM_UInt) != 0 &&
-				    (pIn3->flags & MEM_Int) != 0) {
+				if (mem_is_pos_int(pIn1) &&
+				    mem_is_neg_int(pIn3)) {
 					res = -1;
 					goto compare_op;
 				}
@@ -2363,21 +2338,13 @@ 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);
+			if (mem_is_number(pIn1)) {
 				sqlVdbeMemStringify(pIn1);
-				testcase( (flags1&MEM_Dyn) != (pIn1->flags&MEM_Dyn));
 				flags1 = (pIn1->flags & ~MEM_TypeMask) | (flags1 & MEM_TypeMask);
 				assert(pIn1!=pIn3);
 			}
-			if ((flags3 & MEM_Str) == 0 &&
-			    (flags3 & (MEM_Int | MEM_UInt | MEM_Real)) != 0) {
-				testcase( pIn3->flags & MEM_Int);
-				testcase( pIn3->flags & MEM_Real);
+			if (mem_is_number(pIn3)) {
 				sqlVdbeMemStringify(pIn3);
-				testcase( (flags3&MEM_Dyn) != (pIn3->flags&MEM_Dyn));
 				flags3 = (pIn3->flags & ~MEM_TypeMask) | (flags3 & MEM_TypeMask);
 			}
 		}
@@ -2585,9 +2552,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 +2562,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 +2595,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 +2618,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 +2663,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 +2686,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 +2700,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 +2755,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 +2784,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);
@@ -2969,21 +2936,16 @@ case OP_MakeRecord: {
 	 * routine.
 	 */
 	if (bIsEphemeral) {
-		if (sqlVdbeMemClearAndResize(pOut, tuple_size) != 0)
+		if (mem_set_bin(pOut, tuple, tuple_size, 0, 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;
+		if (mem_set_bin(pOut, tuple, tuple_size, MEM_Ephem, false) != 0)
+			goto abort_due_to_error;
 	}
 	assert(sqlVdbeCheckMemInvariants(pOut));
 	assert(pOp->p3>0 && pOp->p3<=(p->nMem+1 - p->nCursor));
@@ -3261,8 +3223,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;
 }
 
@@ -3491,18 +3452,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)
@@ -3520,8 +3481,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
 				 */
@@ -3737,7 +3698,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);
@@ -3751,7 +3712,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;
 			}
@@ -3861,14 +3822,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;
@@ -4000,7 +3961,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;
@@ -4358,7 +4319,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;
@@ -4392,7 +4353,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;
@@ -4437,7 +4398,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)
@@ -4483,10 +4444,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);
 
@@ -4915,7 +4876,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
@@ -4931,9 +4892,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;
@@ -4949,7 +4908,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 {
@@ -5055,8 +5014,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;
 		/*
@@ -5090,8 +5049,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;
@@ -5114,7 +5073,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;
@@ -5130,7 +5089,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;
@@ -5216,7 +5175,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;
@@ -5227,7 +5186,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;
@@ -5253,7 +5212,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);
@@ -5355,11 +5314,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;
 }
 
@@ -5380,7 +5339,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);
@@ -5391,7 +5350,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);
-- 
2.25.1


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

* Re: [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking
  2021-02-01  8:14 [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
                   ` (9 preceding siblings ...)
  2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 10/10] sql: refactor vdbe.c Mergen Imeev via Tarantool-patches
@ 2021-02-09  9:36 ` Timur Safin via Tarantool-patches
  2021-02-13 15:13   ` Mergen Imeev via Tarantool-patches
  10 siblings, 1 reply; 17+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-02-09  9:36 UTC (permalink / raw)
  To: imeevma, s.ostanevich; +Cc: tarantool-patches

Thanks for this refactoring attempt, but could you (please, please) 
make it (somehow magically) easier to review? I mean the part where
we extract something from here, wrap I as a function, and replace
with that function call ideally would be to put to the same patch -
for better observability. So we make it more visible that functionality
remains intact?
I know that is tricky in git, but possible after some massaging. (May 
Be simply squash it to the single patch as a first approach?)

I've no much complains yet about particular cases - but there is 
some objection about newly introduced static_alloc, please see in 
other message.

Regards,
Timur

: From: imeevma@tarantool.org <imeevma@tarantool.org>
: Subject: [PATCH v1 00/10] Encapsulate MEM type changing and checking
: 
: This patch-set encapsulates type changing and checking for MEM. This is done
: to
: make easier for us to introduce new rules for implicit and explicit type
: casting
: and new types in SQL.
: 
: https://github.com/tarantool/tarantool/issues/4470
: https://github.com/tarantool/tarantool/tree/imeevma/gh-4470-encapsule-type-
: setting-v3
: 
: 
: Mergen Imeev (10):
:   sql: introduce mem_set_*() functions
:   sql: Initialize MEM in sqlVdbeAllocUnpackedRecord()
:   sql: introduce mem_is_*() functions
:   sql: introduce mem_convert_to_binary()
:   sql: refactor vdbesort.c
:   sql: refactor sql/func.c
:   sql: refactor vdbetrace.c
:   sql: refactor vdbeapi.c
:   sql: refactor vdbeaux.c
:   sql: refactor vdbe.c
: 
:  src/box/sql/func.c      |   2 +-
:  src/box/sql/vdbe.c      | 453 ++++++++++++++++++----------------------
:  src/box/sql/vdbeInt.h   | 204 ++++++++++++++++++
:  src/box/sql/vdbeapi.c   |  53 ++---
:  src/box/sql/vdbeaux.c   | 268 +++++++++++-------------
:  src/box/sql/vdbemem.c   | 116 ++++++++++
:  src/box/sql/vdbesort.c  |   9 +-
:  src/box/sql/vdbetrace.c |  12 +-
:  8 files changed, 674 insertions(+), 443 deletions(-)
: 
: --
: 2.25.1



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

* [Tarantool-patches] FW: [PATCH v1 09/10] sql: refactor vdbeaux.c
  2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 09/10] sql: refactor vdbeaux.c Mergen Imeev via Tarantool-patches
@ 2021-02-09  9:51   ` Timur Safin via Tarantool-patches
  2021-02-13 15:33     ` Mergen Imeev via Tarantool-patches
  2021-02-28 17:35     ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 2 replies; 17+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-02-09  9:51 UTC (permalink / raw)
  To: imeevma, Sergey Ostanevich; +Cc: tarantool-patches



-----Original Message-----
From: Timur Safin <tsafin@tarantool.org> 
Sent: Tuesday, February 9, 2021 12:50 PM
To: 'imeevma@tarantool.org' <imeevma@tarantool.org>
Subject: RE: [PATCH v1 09/10] sql: refactor vdbeaux.c

There are similar notes about neg/pos vs signed/unsigned in 
function names as I've mentioned earlier.

But also some extra comments, please see below...

: From: imeevma@tarantool.org <imeevma@tarantool.org>
: Subject: [PATCH v1 09/10] sql: refactor vdbeaux.c
: 
: ---
:  src/box/sql/vdbeaux.c | 266 +++++++++++++++++++-----------------------
:  1 file changed, 122 insertions(+), 144 deletions(-)
: 
: diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
: index 7b8a1e1d8..4ffb34a4e 100644
: --- a/src/box/sql/vdbeaux.c
: +++ b/src/box/sql/vdbeaux.c
: @@ -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_set_bin(pSub, (char *)buf, nByte, 0,
: +						    false);
: +					region_truncate(&fiber()->gc, svp);
:  				}
:  			}
:  		}

This was quite unexpected. I'd rather expect that refactoring would 
reduce number of code used inside of functions, not make them
verbose. Could you please explain me why this extra code would 
become necessary? (And why not wrap them elsewhere?)

: @@ -1402,41 +1408,26 @@ sqlVdbeList(Vdbe * p)
:  		mem_set_i64(pMem, pOp->p3);
:  		pMem++;
: 
: -		if (sqlVdbeMemClearAndResize(pMem, 256)) {
: -			assert(p->db->mallocFailed);
: -			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);
: -		}
: +		size_t size = 256;
: +		char *tmp_buf = (char *) static_alloc(size);
: +		assert(tmp_buf != NULL);
: +		zP4 = displayP4(pOp, tmp_buf, size);
: +		mem_set_str(pMem, zP4, strlen(zP4), 0, true);
:  		pMem++;

Please, please, not introduce any newer usage of static_alloc,
it's very fragile and might work only inside of controlled context of 
single module (i.e. swim). If used by multiple modules -
results are unpredictable. Any other allocator is fine if used 
by multiple fibers, but not static_alloc.

Regards,
Timur


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

* Re: [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking
  2021-02-09  9:36 ` [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Timur Safin via Tarantool-patches
@ 2021-02-13 15:13   ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-13 15:13 UTC (permalink / raw)
  To: Timur Safin; +Cc: s.ostanevich, tarantool-patches

On Tue, Feb 09, 2021 at 12:36:03PM +0300, Timur Safin wrote:
Hi! Thank you for the review. I squashed almost all patches in one.
Also, I replaced static_alloc() by region_alloc().

I will send it as a new patch-set.


> Thanks for this refactoring attempt, but could you (please, please) 
> make it (somehow magically) easier to review? I mean the part where
> we extract something from here, wrap I as a function, and replace
> with that function call ideally would be to put to the same patch -
> for better observability. So we make it more visible that functionality
> remains intact?
> I know that is tricky in git, but possible after some massaging. (May 
> Be simply squash it to the single patch as a first approach?)
> 
> I've no much complains yet about particular cases - but there is 
> some objection about newly introduced static_alloc, please see in 
> other message.
> 
> Regards,
> Timur
> 
> : From: imeevma@tarantool.org <imeevma@tarantool.org>
> : Subject: [PATCH v1 00/10] Encapsulate MEM type changing and checking
> : 
> : This patch-set encapsulates type changing and checking for MEM. This is done
> : to
> : make easier for us to introduce new rules for implicit and explicit type
> : casting
> : and new types in SQL.
> : 
> : https://github.com/tarantool/tarantool/issues/4470
> : https://github.com/tarantool/tarantool/tree/imeevma/gh-4470-encapsule-type-
> : setting-v3
> : 
> : 
> : Mergen Imeev (10):
> :   sql: introduce mem_set_*() functions
> :   sql: Initialize MEM in sqlVdbeAllocUnpackedRecord()
> :   sql: introduce mem_is_*() functions
> :   sql: introduce mem_convert_to_binary()
> :   sql: refactor vdbesort.c
> :   sql: refactor sql/func.c
> :   sql: refactor vdbetrace.c
> :   sql: refactor vdbeapi.c
> :   sql: refactor vdbeaux.c
> :   sql: refactor vdbe.c
> : 
> :  src/box/sql/func.c      |   2 +-
> :  src/box/sql/vdbe.c      | 453 ++++++++++++++++++----------------------
> :  src/box/sql/vdbeInt.h   | 204 ++++++++++++++++++
> :  src/box/sql/vdbeapi.c   |  53 ++---
> :  src/box/sql/vdbeaux.c   | 268 +++++++++++-------------
> :  src/box/sql/vdbemem.c   | 116 ++++++++++
> :  src/box/sql/vdbesort.c  |   9 +-
> :  src/box/sql/vdbetrace.c |  12 +-
> :  8 files changed, 674 insertions(+), 443 deletions(-)
> : 
> : --
> : 2.25.1
> 
> 

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

* Re: [Tarantool-patches] [PATCH v1 10/10] sql: refactor vdbe.c
       [not found]   ` <047f01d6fec7$b5a90bb0$20fb2310$@tarantool.org>
@ 2021-02-13 15:26     ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-13 15:26 UTC (permalink / raw)
  To: Timur Safin; +Cc: s.ostanevich, tarantool-patches

On Tue, Feb 09, 2021 at 12:41:15PM +0300, Timur Safin wrote:
> 
> 
> : From: imeevma@tarantool.org <imeevma@tarantool.org>
> : Subject: [PATCH v1 10/10] sql: refactor vdbe.c
> : 
> : ---
> :  src/box/sql/vdbe.c | 453 +++++++++++++++++++++------------------------
> :  1 file changed, 206 insertions(+), 247 deletions(-)
> : 
> : diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> : index 1707c216e..7d4a0b297 100644
> : --- a/src/box/sql/vdbe.c
> : +++ b/src/box/sql/vdbe.c
> : @@ -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))
> 
> Is it actually only about positive integers, and not (un)signed 
> integers in general? Why it's called mem_is_pos_int? Not mem_is_uint?
> 
For positive integer it is true that mem_is_uint() is actually better, however
I decided to left it as mem_is_pos_int() so it would be named in the same style
as mem_is_neg_int(). I mean, both named to this way to show that they checks
mem_type defined by implementation. The mem_is_neg_int() function is actually
checks that MEM is negative integer, not just signed integer. The MEM_Int type
is similar to MP_INT which allows only negative integers.

I wonder if it is possible to make VDBE only work with functions that check only
the field type? I mean mem_is_integer(), mem_is_unsigned(), mem_is_scalar() and
so on. In this case, we don't need implementation-defined MEM type checking
functions. I cannot say yet if this is possible.

> : @@ -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)) {
> 
> The same question - why not mem_is_signed_int? Or simply mem_is_int?
> 
Answered above.

> : @@ -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))
> 
> Oh, I see there is already mem_is_integer, thus subtypes better to be 
> Explicitly named mem_is_unsigned_int and mem_is_signed_int (not pos/neg)
> 
Answered above.

> 
> :  		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;
> 
> Timur
> 

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

* Re: [Tarantool-patches] FW: [PATCH v1 09/10] sql: refactor vdbeaux.c
  2021-02-09  9:51   ` [Tarantool-patches] FW: " Timur Safin via Tarantool-patches
@ 2021-02-13 15:33     ` Mergen Imeev via Tarantool-patches
  2021-02-28 17:35     ` Vladislav Shpilevoy via Tarantool-patches
  1 sibling, 0 replies; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-02-13 15:33 UTC (permalink / raw)
  To: Timur Safin; +Cc: tarantool-patches

On Tue, Feb 09, 2021 at 12:51:14PM +0300, Timur Safin wrote:
> 
> 
> -----Original Message-----
> From: Timur Safin <tsafin@tarantool.org> 
> Sent: Tuesday, February 9, 2021 12:50 PM
> To: 'imeevma@tarantool.org' <imeevma@tarantool.org>
> Subject: RE: [PATCH v1 09/10] sql: refactor vdbeaux.c
> 
> There are similar notes about neg/pos vs signed/unsigned in 
> function names as I've mentioned earlier.
Answered in another letter.

> 
> But also some extra comments, please see below...
> 
> : From: imeevma@tarantool.org <imeevma@tarantool.org>
> : Subject: [PATCH v1 09/10] sql: refactor vdbeaux.c
> : 
> : ---
> :  src/box/sql/vdbeaux.c | 266 +++++++++++++++++++-----------------------
> :  1 file changed, 122 insertions(+), 144 deletions(-)
> : 
> : diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> : index 7b8a1e1d8..4ffb34a4e 100644
> : --- a/src/box/sql/vdbeaux.c
> : +++ b/src/box/sql/vdbeaux.c
> : @@ -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_set_bin(pSub, (char *)buf, nByte, 0,
> : +						    false);
> : +					region_truncate(&fiber()->gc, svp);
> :  				}
> :  			}
> :  		}
> 
> This was quite unexpected. I'd rather expect that refactoring would 
> reduce number of code used inside of functions, not make them
> verbose. Could you please explain me why this extra code would 
> become necessary? (And why not wrap them elsewhere?)
> 
In this case function changed MEM without using mem_set_*() functions. I believe
that is not what we want. Now it each time copy value from MEM, append new value
to copied value and set it to MEM. It may be not so efficient, however I believe
it shouldn't be a problem here since EXPLAIN is not something that should be
as fast as possible.

> : @@ -1402,41 +1408,26 @@ sqlVdbeList(Vdbe * p)
> :  		mem_set_i64(pMem, pOp->p3);
> :  		pMem++;
> : 
> : -		if (sqlVdbeMemClearAndResize(pMem, 256)) {
> : -			assert(p->db->mallocFailed);
> : -			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);
> : -		}
> : +		size_t size = 256;
> : +		char *tmp_buf = (char *) static_alloc(size);
> : +		assert(tmp_buf != NULL);
> : +		zP4 = displayP4(pOp, tmp_buf, size);
> : +		mem_set_str(pMem, zP4, strlen(zP4), 0, true);
> :  		pMem++;
> 
> Please, please, not introduce any newer usage of static_alloc,
> it's very fragile and might work only inside of controlled context of 
> single module (i.e. swim). If used by multiple modules -
> results are unpredictable. Any other allocator is fine if used 
> by multiple fibers, but not static_alloc.
Replaced by region_alloc().

> 
> Regards,
> Timur
> 

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

* Re: [Tarantool-patches] FW: [PATCH v1 09/10] sql: refactor vdbeaux.c
  2021-02-09  9:51   ` [Tarantool-patches] FW: " Timur Safin via Tarantool-patches
  2021-02-13 15:33     ` Mergen Imeev via Tarantool-patches
@ 2021-02-28 17:35     ` Vladislav Shpilevoy via Tarantool-patches
  1 sibling, 0 replies; 17+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-02-28 17:35 UTC (permalink / raw)
  To: Timur Safin, imeevma, Sergey Ostanevich; +Cc: tarantool-patches

> : @@ -1402,41 +1408,26 @@ sqlVdbeList(Vdbe * p)
> :  		mem_set_i64(pMem, pOp->p3);
> :  		pMem++;
> : 
> : -		if (sqlVdbeMemClearAndResize(pMem, 256)) {
> : -			assert(p->db->mallocFailed);
> : -			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);
> : -		}
> : +		size_t size = 256;
> : +		char *tmp_buf = (char *) static_alloc(size);
> : +		assert(tmp_buf != NULL);
> : +		zP4 = displayP4(pOp, tmp_buf, size);
> : +		mem_set_str(pMem, zP4, strlen(zP4), 0, true);
> :  		pMem++;
> 
> Please, please, not introduce any newer usage of static_alloc,
> it's very fragile and might work only inside of controlled context of 
> single module (i.e. swim). If used by multiple modules -
> results are unpredictable. Any other allocator is fine if used 
> by multiple fibers, but not static_alloc.

What exactly is the problem with static alloc here? Can you explain?
From the text above I can only see one argument - 'fragile'. Which is
not a real argument.

Static alloc is faster than anything we have except allocation on
the stack. So I would prefer to use it when possible.

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

end of thread, other threads:[~2021-02-28 17:36 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-01  8:14 [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches
2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 01/10] sql: introduce mem_set_*() functions Mergen Imeev via Tarantool-patches
2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 02/10] sql: Initialize MEM in sqlVdbeAllocUnpackedRecord() Mergen Imeev via Tarantool-patches
2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 03/10] sql: introduce mem_is_*() functions Mergen Imeev via Tarantool-patches
2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 04/10] sql: introduce mem_convert_to_binary() Mergen Imeev via Tarantool-patches
2021-02-01  8:14 ` [Tarantool-patches] [PATCH v1 05/10] sql: refactor vdbesort.c Mergen Imeev via Tarantool-patches
2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 06/10] sql: refactor sql/func.c Mergen Imeev via Tarantool-patches
2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 07/10] sql: refactor vdbetrace.c Mergen Imeev via Tarantool-patches
2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 08/10] sql: refactor vdbeapi.c Mergen Imeev via Tarantool-patches
2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 09/10] sql: refactor vdbeaux.c Mergen Imeev via Tarantool-patches
2021-02-09  9:51   ` [Tarantool-patches] FW: " Timur Safin via Tarantool-patches
2021-02-13 15:33     ` Mergen Imeev via Tarantool-patches
2021-02-28 17:35     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-01  8:15 ` [Tarantool-patches] [PATCH v1 10/10] sql: refactor vdbe.c Mergen Imeev via Tarantool-patches
     [not found]   ` <047f01d6fec7$b5a90bb0$20fb2310$@tarantool.org>
2021-02-13 15:26     ` Mergen Imeev via Tarantool-patches
2021-02-09  9:36 ` [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Timur Safin via Tarantool-patches
2021-02-13 15:13   ` Mergen Imeev via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox