Re: [Tarantool-patches] [PATCH v3 2/2] sql: Encapsulate MEM type changing and checking

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

See 33 comments below.

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

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

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

Please, make it 3 separate functions:

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

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

2. Please, add whitespaces around `>`.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

16. The comments are wrong now.

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

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

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

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

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

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

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

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

20. Constructors should have 'create' suffix.

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

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

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

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

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

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

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

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

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

25. There is no `szNew` argument.

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

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

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

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

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

28. Maybe just inline this entire function.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Then use proper names for the existing functions.

Then introduce new functions.

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

This would do the real encapsulation.