[Tarantool-patches] [PATCH v5 09/52] sql: introduce mem_str()
Mergen Imeev
imeevma at tarantool.org
Thu Apr 15 01:42:04 MSK 2021
Thank you for the review and diff! My answer below. Also, I forgot to mention
in the last letter that I pushed rebased and fixed patches onto old branch,
imeevma/gh-5818-encapsulate-mem-type-checking-and-changing
New patch below.
On Thu, Apr 15, 2021 at 12:23:36AM +0200, Vladislav Shpilevoy wrote:
> Good job on the fixes!
>
> > 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);
>
> Why do you need that limit? The accum is created as
> sqlStrAccumInit(&x, 0, zTemp, nTemp, 0). The last argument 0 means
> it won't realloc anything, so it is safe to append. The same in the
> next diff hunk.
>
> Consider this diff:
>
> ====================
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index a2a0fc33e..95a035fd4 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -52,7 +52,9 @@ mem_str(const struct Mem *mem)
> case MEM_Null:
> return "NULL";
> case MEM_Str:
> - return tt_sprintf("%.*s", mem->n, mem->z);
> + if ((mem->flags & MEM_Term) != 0)
> + return mem->z;
> + return tt_cstr(mem->z, mem->n);
> case MEM_Int:
> return tt_sprintf("%lld", mem->u.i);
> case MEM_UInt:
> @@ -68,9 +70,8 @@ mem_str(const struct Mem *mem)
> case MEM_Bool:
> return mem->u.b ? "TRUE" : "FALSE";
> default:
> - break;
> + return "unknown";
> }
> - return "unknown";
> }
>
> static inline bool
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index 907c9f5c6..9ef0445bd 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -1109,9 +1109,7 @@ displayP4(Op * pOp, char *zTemp, int nTemp)
> }
> case P4_MEM:{
> const char *value = mem_str(pOp->p4.pMem);
> - int len = strlen(value);
> - uint32_t size = MIN(len, nTemp - 1);
> - sqlStrAccumAppend(&x, value, size);
> + sqlStrAccumAppend(&x, value, strlen(value));
> break;
> }
> case P4_INTARRAY:{
> diff --git a/src/box/sql/vdbetrace.c b/src/box/sql/vdbetrace.c
> index 677de65e2..a26880041 100644
> --- a/src/box/sql/vdbetrace.c
> +++ b/src/box/sql/vdbetrace.c
> @@ -146,9 +146,7 @@ sqlVdbeExpandSql(Vdbe * p, /* The prepared statement being evaluated */
> nextIndex = idx + 1;
> assert(idx > 0 && idx <= p->nVar);
> 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);
> + sqlStrAccumAppend(&out, value, strlen(value));
> }
> }
> if (out.accError)
Thank you again! I applied this patch and checked that tests are fine.
New patch:
commit cd39e0296477effaa054a139604ee52a382dd7be
Author: Mergen Imeev <imeevma at 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..95a035fd4 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -40,6 +40,40 @@
#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:
+ if ((mem->flags & MEM_Term) != 0)
+ return mem->z;
+ return tt_cstr(mem->z, mem->n);
+ 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:
+ return "unknown";
+ }
+}
+
static inline bool
mem_has_msgpack_subtype(struct Mem *mem)
{
@@ -1592,18 +1626,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 d03725106..cfd694c2a 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..9ef0445bd 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1108,21 +1108,8 @@ 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);
+ sqlStrAccumAppend(&x, value, strlen(value));
break;
}
case P4_INTARRAY:{
diff --git a/src/box/sql/vdbetrace.c b/src/box/sql/vdbetrace.c
index e84bb3192..a26880041 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,8 @@ 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]);
+ sqlStrAccumAppend(&out, value, strlen(value));
}
}
if (out.accError)
More information about the Tarantool-patches
mailing list