Re: [Tarantool-patches] [PATCH v5 09/52] sql: introduce mem_str()

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

Thank you for the review! My answer, diff and new patch below.

On Sun, Apr 11, 2021 at 07:44:53PM +0200, Vladislav Shpilevoy wrote:
> I appreciate the work you did here!
> 
> > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> > index 79c3d60e0..b7e148422 100644
> > --- a/src/box/sql/vdbeaux.c
> > +++ b/src/box/sql/vdbeaux.c
> > @@ -1108,21 +1108,10 @@ displayP4(Op * pOp, char *zTemp, int nTemp)
> >  			break;
> >  		}
> >  	case P4_MEM:{
> > -			Mem *pMem = pOp->p4.pMem;
> > -			if (pMem->flags & MEM_Str) {
> > -				zP4 = pMem->z;
> > -			} else if (pMem->flags & MEM_Int) {
> > -				sqlXPrintf(&x, "%lld", pMem->u.i);
> > -			} else if (pMem->flags & MEM_UInt) {
> > -				sqlXPrintf(&x, "%llu", pMem->u.u);
> > -			} else if (pMem->flags & MEM_Real) {
> > -				sqlXPrintf(&x, "%.16g", pMem->u.r);
> > -			} else if (pMem->flags & MEM_Null) {
> > -				zP4 = "NULL";
> > -			} else {
> > -				assert(pMem->flags & MEM_Blob);
> > -				zP4 = "(binary string)";
> > -			}
> > +			const char *value = mem_str(pOp->p4.pMem);
> > +			uint32_t size = MIN((int)strlen(value), nTemp - 1);
> > +			memcpy(zP4, value, size);
> > +			zP4[size] = '\0';
> 
> 'x' stays in an invalid state now, because its counter nChar is
> not updated. I would propose to use sqlStrAccumAppend/sqlStrAccumAppendAll
> instead of memcpy and manual 0 termination.
> 
> The same below in sqlVdbeExpandSql().
> 
Fixed.

> >  			break;
> >  		}
> >  	case P4_INTARRAY:{
> > diff --git a/src/box/sql/vdbetrace.c b/src/box/sql/vdbetrace.c
> > index e84bb3192..4ca56865d 100644
> > --- a/src/box/sql/vdbetrace.c
> > +++ b/src/box/sql/vdbetrace.c
> > @@ -147,33 +145,11 @@ sqlVdbeExpandSql(Vdbe * p,	/* The prepared statement being evaluated */
> >  			zRawSql += nToken;
> >  			nextIndex = idx + 1;
> >  			assert(idx > 0 && idx <= p->nVar);
> > -			pVar = &p->aVar[idx - 1];
> > -			if (pVar->flags & MEM_Null) {
> > -				sqlStrAccumAppend(&out, "NULL", 4);
> > -			} else if (pVar->flags & MEM_Int) {
> > -				sqlXPrintf(&out, "%lld", pVar->u.i);
> > -			} else if (pVar->flags & MEM_UInt) {
> > -				sqlXPrintf(&out, "%llu", pVar->u.u);
> > -			} else if (pVar->flags & MEM_Real) {
> > -				sqlXPrintf(&out, "%!.15g", pVar->u.r);
> > -			} else if (pVar->flags & MEM_Str) {
> > -				int nOut;	/* Number of bytes of the string text to include in output */
> > -				nOut = pVar->n;
> > -				sqlXPrintf(&out, "'%.*q'", nOut, pVar->z);
> > -			} else if (pVar->flags & MEM_Zero) {
> > -				sqlXPrintf(&out, "zeroblob(%d)",
> > -					       pVar->u.nZero);
> > -			} else {
> > -				int nOut;	/* Number of bytes of the blob to include in output */
> > -				assert(pVar->flags & MEM_Blob);
> > -				sqlStrAccumAppend(&out, "x'", 2);
> > -				nOut = pVar->n;
> > -				for (i = 0; i < nOut; i++) {
> > -					sqlXPrintf(&out, "%02x",
> > -						       pVar->z[i] & 0xff);
> > -				}
> > -				sqlStrAccumAppend(&out, "'", 1);
> > -			}
> > +			const char *value = mem_str(&p->aVar[idx - 1]);
> > +			uint32_t len = strlen(value);
> > +			uint32_t size = MIN(len, sizeof(zBase) - 1);
> > +			memcpy(zBase, value, size);
> > +			zBase[size] = '\0';
> >  		}
> >  	}
> >  	if (out.accError)
> > 



Diff:


diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index b7e148422..907c9f5c6 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1109,9 +1109,9 @@ displayP4(Op * pOp, char *zTemp, int nTemp)
 		}
 	case P4_MEM:{
 			const char *value = mem_str(pOp->p4.pMem);
-			uint32_t size = MIN((int)strlen(value), nTemp - 1);
-			memcpy(zP4, value, size);
-			zP4[size] = '\0';
+			int len = strlen(value);
+			uint32_t size = MIN(len, nTemp - 1);
+			sqlStrAccumAppend(&x, value, size);
 			break;
 		}
 	case P4_INTARRAY:{
diff --git a/src/box/sql/vdbetrace.c b/src/box/sql/vdbetrace.c
index 4ca56865d..677de65e2 100644
--- a/src/box/sql/vdbetrace.c
+++ b/src/box/sql/vdbetrace.c
@@ -148,8 +148,7 @@ sqlVdbeExpandSql(Vdbe * p,	/* The prepared statement being evaluated */
 			const char *value = mem_str(&p->aVar[idx - 1]);
 			uint32_t len = strlen(value);
 			uint32_t size = MIN(len, sizeof(zBase) - 1);
-			memcpy(zBase, value, size);
-			zBase[size] = '\0';
+			sqlStrAccumAppend(&out, value, size);
 		}
 	}
 	if (out.accError)



New patch

commit 3ef9df0c7c1a9947a63fd024b966690cc065d2b0
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Tue Mar 2 13:52:11 2021 +0300

    sql: introduce mem_str()
    
    This patch introduces mem_str() which allows to receive value of MEM as
    a string. Due to the limitations of static_alloc(), this function cannot
    be used to safely retrieve a value of MEM converted to string. This
    function is suitable for debugging, displaying the value in an error
    message, etc.
    
    Part of #5818

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 46814f341..99ce938d5 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -543,7 +543,7 @@ roundFunc(sql_context * context, int argc, sql_value ** argv)
 	enum mp_type mp_type = sql_value_type(argv[0]);
 	if (mp_type_is_bloblike(mp_type)) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 sql_value_to_diag_str(argv[0]), "numeric");
+			 mem_str(argv[0]), "numeric");
 		context->is_aborted = true;
 		return;
 	}
@@ -685,7 +685,7 @@ randomBlob(sql_context * context, int argc, sql_value ** argv)
 	UNUSED_PARAMETER(argc);
 	if (mp_type_is_bloblike(sql_value_type(argv[0]))) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 sql_value_to_diag_str(argv[0]), "numeric");
+			 mem_str(argv[0]), "numeric");
 		context->is_aborted = true;
 		return;
 	}
@@ -1577,7 +1577,7 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv)
 	enum mp_type mp_type = sql_value_type(argv[0]);
 	if (mp_type_is_bloblike(mp_type)) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 sql_value_to_diag_str(argv[0]), "text");
+			 mem_str(argv[0]), "text");
 		context->is_aborted = true;
 		return;
 	}
@@ -1650,7 +1650,7 @@ sum_step(struct sql_context *context, int argc, sql_value **argv)
 	if (type != MP_DOUBLE && type != MP_INT && type != MP_UINT) {
 		if (mem_apply_numeric_type(argv[0]) != 0) {
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-				 sql_value_to_diag_str(argv[0]), "number");
+				 mem_str(argv[0]), "number");
 			context->is_aborted = true;
 			return;
 		}
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 25f5f2f2d..a2a0fc33e 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -40,6 +40,39 @@
 #include "lua/utils.h"
 #include "lua/msgpack.h"
 
+enum {
+	BUF_SIZE = 32,
+};
+
+const char *
+mem_str(const struct Mem *mem)
+{
+	char buf[BUF_SIZE];
+	switch (mem->flags & MEM_PURE_TYPE_MASK) {
+	case MEM_Null:
+		return "NULL";
+	case MEM_Str:
+		return tt_sprintf("%.*s", mem->n, mem->z);
+	case MEM_Int:
+		return tt_sprintf("%lld", mem->u.i);
+	case MEM_UInt:
+		return tt_sprintf("%llu", mem->u.u);
+	case MEM_Real:
+		sql_snprintf(BUF_SIZE, &buf[0], "%!.15g", mem->u.r);
+		return tt_sprintf("%s", buf);
+	case MEM_Blob:
+		if ((mem->flags & MEM_Subtype) == 0)
+			return "varbinary";
+		assert(mem->subtype == SQL_SUBTYPE_MSGPACK);
+		return mp_str(mem->z);
+	case MEM_Bool:
+		return mem->u.b ? "TRUE" : "FALSE";
+	default:
+		break;
+	}
+	return "unknown";
+}
+
 static inline bool
 mem_has_msgpack_subtype(struct Mem *mem)
 {
@@ -1592,18 +1625,6 @@ sqlValueText(sql_value * pVal)
 	return valueToText(pVal);
 }
 
-const char *
-sql_value_to_diag_str(sql_value *value)
-{
-	enum mp_type mp_type = sql_value_type(value);
-	if (mp_type_is_bloblike(mp_type)) {
-		if (mem_has_msgpack_subtype(value))
-			return sqlValueText(value);
-		return "varbinary";
-	}
-	return sqlValueText(value);
-}
-
 enum sql_subtype
 sql_value_subtype(sql_value * pVal)
 {
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index acc8ce054..7b9456426 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -87,6 +87,13 @@ struct Mem {
  */
 #define MEMCELLSIZE offsetof(Mem,zMalloc)
 
+/**
+ * Return a string that represent content of MEM. String is either allocated
+ * using static_alloc() of just a static variable.
+ */
+const char *
+mem_str(const struct Mem *mem);
+
 /* One or more of the following flags are set to indicate the validOK
  * representations of the value stored in the Mem struct.
  *
@@ -358,15 +365,6 @@ sql_value_text(struct Mem *);
 
 const void *sqlValueText(struct Mem *);
 
-/**
- * Return pointer to a string with the data type in the case of
- * binary data stored in @a value. Otherwise, return the result
- * of sql_value_text(). It is used due to the fact that not all
- * binary strings can be displayed correctly (e.g. contain
- * unprintable symbols).
- */
-const char *
-sql_value_to_diag_str(struct Mem *value);
 #define VdbeFrameMem(p) ((Mem *)&((u8 *)p)[ROUND8(sizeof(VdbeFrame))])
 
 enum sql_subtype
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index e269857ea..ec3d23cb2 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1265,12 +1265,12 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
 	} else {
 		if (sqlVdbeRealValue(pIn1, &rA) != 0) {
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-				 sql_value_to_diag_str(pIn1), "numeric");
+				 mem_str(pIn1), "numeric");
 			goto abort_due_to_error;
 		}
 		if (sqlVdbeRealValue(pIn2, &rB) != 0) {
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-				 sql_value_to_diag_str(pIn2), "numeric");
+				 mem_str(pIn2), "numeric");
 			goto abort_due_to_error;
 		}
 		assert(((type1 | type2) & MEM_Real) != 0);
@@ -1546,12 +1546,12 @@ case OP_ShiftRight: {           /* same as TK_RSHIFT, in1, in2, out3 */
 	bool unused;
 	if (sqlVdbeIntValue(pIn2, (int64_t *) &iA, &unused) != 0) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 sql_value_to_diag_str(pIn2), "integer");
+			 mem_str(pIn2), "integer");
 		goto abort_due_to_error;
 	}
 	if (sqlVdbeIntValue(pIn1, (int64_t *) &iB, &unused) != 0) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 sql_value_to_diag_str(pIn1), "integer");
+			 mem_str(pIn1), "integer");
 		goto abort_due_to_error;
 	}
 	op = pOp->opcode;
@@ -1616,7 +1616,7 @@ case OP_MustBeInt: {            /* jump, in1 */
 		if ((pIn1->flags & (MEM_Int | MEM_UInt)) == 0) {
 			if (pOp->p2==0) {
 				diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-					 sql_value_to_diag_str(pIn1), "integer");
+					 mem_str(pIn1), "integer");
 				goto abort_due_to_error;
 			} else {
 				goto jump_to_p2;
@@ -1673,7 +1673,7 @@ case OP_Cast: {                  /* in1 */
 	UPDATE_MAX_BLOBSIZE(pIn1);
 	if (rc == 0)
 		break;
-	diag_set(ClientError, ER_SQL_TYPE_MISMATCH, sql_value_to_diag_str(pIn1),
+	diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(pIn1),
 		 field_type_strs[pOp->p2]);
 	goto abort_due_to_error;
 }
@@ -1846,7 +1846,7 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 					if (mem_apply_numeric_type(pIn3) != 0) {
 						diag_set(ClientError,
 							 ER_SQL_TYPE_MISMATCH,
-							 sql_value_to_diag_str(pIn3),
+							 mem_str(pIn3),
 							 "numeric");
 						goto abort_due_to_error;
 					}
@@ -2112,7 +2112,7 @@ case OP_Or: {             /* same as TK_OR, in1, in2, out3 */
 		v1 = pIn1->u.b;
 	} else {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 sql_value_to_diag_str(pIn1), "boolean");
+			 mem_str(pIn1), "boolean");
 		goto abort_due_to_error;
 	}
 	pIn2 = &aMem[pOp->p2];
@@ -2122,7 +2122,7 @@ case OP_Or: {             /* same as TK_OR, in1, in2, out3 */
 		v2 = pIn2->u.b;
 	} else {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 sql_value_to_diag_str(pIn2), "boolean");
+			 mem_str(pIn2), "boolean");
 		goto abort_due_to_error;
 	}
 	if (pOp->opcode==OP_And) {
@@ -2152,7 +2152,7 @@ case OP_Not: {                /* same as TK_NOT, in1, out2 */
 	if ((pIn1->flags & MEM_Null)==0) {
 		if ((pIn1->flags & MEM_Bool) == 0) {
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-				 sql_value_to_diag_str(pIn1), "boolean");
+				 mem_str(pIn1), "boolean");
 			goto abort_due_to_error;
 		}
 		mem_set_bool(pOut, ! pIn1->u.b);
@@ -2177,7 +2177,7 @@ case OP_BitNot: {             /* same as TK_BITNOT, in1, out2 */
 		bool is_neg;
 		if (sqlVdbeIntValue(pIn1, &i, &is_neg) != 0) {
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-				 sql_value_to_diag_str(pIn1), "integer");
+				 mem_str(pIn1), "integer");
 			goto abort_due_to_error;
 		}
 		mem_set_i64(pOut, ~i);
@@ -2223,7 +2223,7 @@ case OP_IfNot: {            /* jump, in1 */
 		c = pOp->opcode == OP_IfNot ? ! pIn1->u.b : pIn1->u.b;
 	} else {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 sql_value_to_diag_str(pIn1), "boolean");
+			 mem_str(pIn1), "boolean");
 		goto abort_due_to_error;
 	}
 	VdbeBranchTaken(c!=0, 2);
@@ -2403,7 +2403,7 @@ case OP_ApplyType: {
 		continue;
 type_mismatch:
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 sql_value_to_diag_str(pIn1), field_type_strs[type]);
+			 mem_str(pIn1), field_type_strs[type]);
 		goto abort_due_to_error;
 	}
 	break;
@@ -3032,7 +3032,7 @@ case OP_SeekGT: {       /* jump, in3 */
 			is_neg = i < 0;
 		} else {
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-				 sql_value_to_diag_str(pIn3), "integer");
+				 mem_str(pIn3), "integer");
 			goto abort_due_to_error;
 		}
 		iKey = i;
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 79c3d60e0..907c9f5c6 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1108,21 +1108,10 @@ displayP4(Op * pOp, char *zTemp, int nTemp)
 			break;
 		}
 	case P4_MEM:{
-			Mem *pMem = pOp->p4.pMem;
-			if (pMem->flags & MEM_Str) {
-				zP4 = pMem->z;
-			} else if (pMem->flags & MEM_Int) {
-				sqlXPrintf(&x, "%lld", pMem->u.i);
-			} else if (pMem->flags & MEM_UInt) {
-				sqlXPrintf(&x, "%llu", pMem->u.u);
-			} else if (pMem->flags & MEM_Real) {
-				sqlXPrintf(&x, "%.16g", pMem->u.r);
-			} else if (pMem->flags & MEM_Null) {
-				zP4 = "NULL";
-			} else {
-				assert(pMem->flags & MEM_Blob);
-				zP4 = "(binary string)";
-			}
+			const char *value = mem_str(pOp->p4.pMem);
+			int len = strlen(value);
+			uint32_t size = MIN(len, nTemp - 1);
+			sqlStrAccumAppend(&x, value, size);
 			break;
 		}
 	case P4_INTARRAY:{
diff --git a/src/box/sql/vdbetrace.c b/src/box/sql/vdbetrace.c
index e84bb3192..677de65e2 100644
--- a/src/box/sql/vdbetrace.c
+++ b/src/box/sql/vdbetrace.c
@@ -97,8 +97,6 @@ sqlVdbeExpandSql(Vdbe * p,	/* The prepared statement being evaluated */
 	int nextIndex = 1;	/* Index of next ? host parameter */
 	int n;			/* Length of a token prefix */
 	int nToken;		/* Length of the parameter token */
-	int i;			/* Loop counter */
-	Mem *pVar;		/* Value of a host parameter */
 	StrAccum out;		/* Accumulate the output here */
 	char zBase[100];	/* Initial working space */
 
@@ -147,33 +145,10 @@ sqlVdbeExpandSql(Vdbe * p,	/* The prepared statement being evaluated */
 			zRawSql += nToken;
 			nextIndex = idx + 1;
 			assert(idx > 0 && idx <= p->nVar);
-			pVar = &p->aVar[idx - 1];
-			if (pVar->flags & MEM_Null) {
-				sqlStrAccumAppend(&out, "NULL", 4);
-			} else if (pVar->flags & MEM_Int) {
-				sqlXPrintf(&out, "%lld", pVar->u.i);
-			} else if (pVar->flags & MEM_UInt) {
-				sqlXPrintf(&out, "%llu", pVar->u.u);
-			} else if (pVar->flags & MEM_Real) {
-				sqlXPrintf(&out, "%!.15g", pVar->u.r);
-			} else if (pVar->flags & MEM_Str) {
-				int nOut;	/* Number of bytes of the string text to include in output */
-				nOut = pVar->n;
-				sqlXPrintf(&out, "'%.*q'", nOut, pVar->z);
-			} else if (pVar->flags & MEM_Zero) {
-				sqlXPrintf(&out, "zeroblob(%d)",
-					       pVar->u.nZero);
-			} else {
-				int nOut;	/* Number of bytes of the blob to include in output */
-				assert(pVar->flags & MEM_Blob);
-				sqlStrAccumAppend(&out, "x'", 2);
-				nOut = pVar->n;
-				for (i = 0; i < nOut; i++) {
-					sqlXPrintf(&out, "%02x",
-						       pVar->z[i] & 0xff);
-				}
-				sqlStrAccumAppend(&out, "'", 1);
-			}
+			const char *value = mem_str(&p->aVar[idx - 1]);
+			uint32_t len = strlen(value);
+			uint32_t size = MIN(len, sizeof(zBase) - 1);
+			sqlStrAccumAppend(&out, value, size);
 		}
 	}
 	if (out.accError)