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

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thank you for the review. My answers below.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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