Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce mem_snprintf()

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

Thank you for the review! My answers, diff and new patch below. Also, I found
out that mem_str() for MEM with NULL returns "NULL(NULL)". Fixed, see diff.

On Thu, Nov 18, 2021 at 12:03:18AM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 5 comments below.
> 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index 5abaf490d..0f8f0b5cc 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -869,9 +869,14 @@ func_printf(struct sql_context *ctx, int argc, const struct Mem *argv)
> >  	if (argc < 1 || mem_is_null(&argv[0]))
> >  		return;
> >  	if (argc == 1 || !mem_is_str(&argv[0])) {
> > -		struct Mem *mem = ctx->pOut;
> > -		if (mem_copy(mem, &argv[0]) != 0 || mem_to_str(mem) != 0)
> > +		uint32_t size = mem_snprintf(NULL, 0, &argv[0]);
> 
> 1. You are not checking for an error here. mem_snprintf() returns int,
> not uint. I am fine with making it never failing, but then we need
> to at least add some assertions inside of mem_snprintf() that its
> internal snprintf() calls never fail. We can keep returning 'int' though
> because it makes the function compatible with SNPRINT macro.
> 
Thanks. I added two asserts in mem_snprintf(), one of which checks that returned
by snprintf() value is non-negative.

> > +		char *str = sqlDbMallocRawNN(sql_get(), size + 1);
> > +		if (str == NULL) {
> >  			ctx->is_aborted = true;
> > +			return;
> > +		}
> > +		mem_snprintf(str, size + 1, &argv[0]);
> > +		mem_set_str_allocated(ctx->pOut, str, size);
> >  		return;
> >  	}
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index 9ddeea5bb..6e1729f32 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -116,30 +116,51 @@ mem_is_field_compatible(const struct Mem *mem, enum field_type type)
> >  	return field_mp_plain_type_is_compatible(type, mp_type, true);
> >  }
> >  
> > -const char *
> > -mem_str(const struct Mem *mem)
> > +int
> > +mem_snprintf(char *buf, uint32_t size, const struct Mem *mem)
> >  {
> > -	char buf[STR_VALUE_MAX_LEN];
> > -	const char *type = mem_type_to_str(mem);
> >  	switch (mem->type) {
> >  	case MEM_TYPE_NULL:
> > -		return "NULL";
> > +		return snprintf(buf, size, "NULL");
> >  	case MEM_TYPE_STR:
> > +	case MEM_TYPE_BIN:
> > +		return snprintf(buf, size, "%.*s", mem->n, mem->z);
> 
> 2. Why is bin displayed as hex in mem_str() but as a plain string here?
> 
I decided not to break compatibility with the old result of the built-in
printf() function. As for mem_str() - we have to get the readable value there,
so we can't just print the original value. This does not work for STRING values,
as we might end up with an unreadable value there.

> > +	case MEM_TYPE_INT:
> > +		return snprintf(buf, size, "%lld", mem->u.i);
> > +	case MEM_TYPE_UINT:
> > +		return snprintf(buf, size, "%llu",
> > +				(unsigned long long)mem->u.u);
> > +	case MEM_TYPE_DOUBLE: {
> > +		char str[BUF_SIZE];
> > +		sql_snprintf(BUF_SIZE, str, "%!.15g", mem->u.r);
> > +		return snprintf(buf, size, "%s", str);
> > +	}
> > +	case MEM_TYPE_DEC:
> > +		return snprintf(buf, size, "%s", decimal_str(&mem->u.d));
> > +	case MEM_TYPE_MAP:
> > +	case MEM_TYPE_ARRAY:
> > +		return mp_snprint(buf, size, mem->z);
> > +	case MEM_TYPE_UUID:
> > +		return snprintf(buf, size, "%s", tt_uuid_str(&mem->u.uuid));
> > +	case MEM_TYPE_BOOL:
> > +		return snprintf(buf, size, "%s", mem->u.b ? "TRUE" : "FALSE");
> 
> 3. You can drop '%s' here:
> 
> 	return snprintf(buf, size, mem->u.b ? "TRUE" : "FALSE");
> 
True, fixed.

> > +	default:
> > +		return snprintf(buf, size, "unknown");
> 
> 4. Maybe add an assertion that it is not possible? We don't have non-typed
> mems AFAIK.
> 
Added.

> > +	}
> > +}
> > diff --git a/src/box/sql/printf.c b/src/box/sql/printf.c
> > index 8da7c9878..08e18c15d 100644
> > --- a/src/box/sql/printf.c
> > +++ b/src/box/sql/printf.c
> > @@ -160,19 +160,12 @@ getTextArg(PrintfArguments * p)
> >  {
> >  	if (p->nArg <= p->nUsed)
> >  		return 0;
> > -	struct Mem mem;
> > -	mem_create(&mem);
> > -	mem_copy_as_ephemeral(&mem, &p->apArg[p->nUsed++]);
> > -	if (mem_to_str(&mem) != 0) {
> > -		mem_destroy(&mem);
> > -		return NULL;
> > -	}
> > -	char *str = sqlDbMallocRawNN(sql_get(), mem.n + 1);
> > +	const struct Mem *mem = &p->apArg[p->nUsed++];
> > +	uint32_t size = mem_snprintf(NULL, 0, mem);
> > +	char *str = sqlDbMallocRawNN(sql_get(), size + 1);
> >  	if (str == NULL)
> >  		return NULL;
> > -	memcpy(str, mem.z, mem.n);
> > -	str[mem.n] = '\0';
> > -	mem_destroy(&mem);
> > +	mem_snprintf(str, size + 1, mem);
> 
> 5. Hm. I suspect this pattern of snprintf + malloc + snprintf
> is going to be frequent. Maybe wrap it into a function like
> mem_strdup()? It would do these 3 actions inside. WDYT?
I agree. I added mem_strdup(). I decided not to make mem_snprintf() static,
since it was designed as one of the functions for working with MEM outside
mem.c.

Diff:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 0f8f0b5cc..26ccebf83 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -869,14 +869,11 @@ func_printf(struct sql_context *ctx, int argc, const struct Mem *argv)
 	if (argc < 1 || mem_is_null(&argv[0]))
 		return;
 	if (argc == 1 || !mem_is_str(&argv[0])) {
-		uint32_t size = mem_snprintf(NULL, 0, &argv[0]);
-		char *str = sqlDbMallocRawNN(sql_get(), size + 1);
-		if (str == NULL) {
+		char *str = mem_strdup(&argv[0]);
+		if (str == NULL)
 			ctx->is_aborted = true;
-			return;
-		}
-		mem_snprintf(str, size + 1, &argv[0]);
-		mem_set_str_allocated(ctx->pOut, str, size);
+		else
+			mem_set_str0_allocated(ctx->pOut, str);
 		return;
 	}
 	struct PrintfArguments pargs;
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index e415b8b66..234a00d57 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -119,39 +119,64 @@ mem_is_field_compatible(const struct Mem *mem, enum field_type type)
 int
 mem_snprintf(char *buf, uint32_t size, const struct Mem *mem)
 {
+	int res = -1;
 	switch (mem->type) {
 	case MEM_TYPE_NULL:
-		return snprintf(buf, size, "NULL");
+		res = snprintf(buf, size, "NULL");
+		break;
 	case MEM_TYPE_STR:
 	case MEM_TYPE_BIN:
-		return snprintf(buf, size, "%.*s", mem->n, mem->z);
+		res = snprintf(buf, size, "%.*s", mem->n, mem->z);
+		break;
 	case MEM_TYPE_INT:
-		return snprintf(buf, size, "%lld", mem->u.i);
+		res = snprintf(buf, size, "%lld", mem->u.i);
+		break;
 	case MEM_TYPE_UINT:
-		return snprintf(buf, size, "%llu",
-				(unsigned long long)mem->u.u);
+		res = snprintf(buf, size, "%llu", (unsigned long long)mem->u.u);
+		break;
 	case MEM_TYPE_DOUBLE: {
 		char str[BUF_SIZE];
 		sql_snprintf(BUF_SIZE, str, "%!.15g", mem->u.r);
-		return snprintf(buf, size, "%s", str);
+		res = snprintf(buf, size, "%s", str);
+		break;
 	}
 	case MEM_TYPE_DEC:
-		return snprintf(buf, size, "%s", decimal_str(&mem->u.d));
+		res = snprintf(buf, size, "%s", decimal_str(&mem->u.d));
+		break;
 	case MEM_TYPE_MAP:
 	case MEM_TYPE_ARRAY:
-		return mp_snprint(buf, size, mem->z);
+		res = mp_snprint(buf, size, mem->z);
+		break;
 	case MEM_TYPE_UUID:
-		return snprintf(buf, size, "%s", tt_uuid_str(&mem->u.uuid));
+		res = snprintf(buf, size, "%s", tt_uuid_str(&mem->u.uuid));
+		break;
 	case MEM_TYPE_BOOL:
-		return snprintf(buf, size, "%s", mem->u.b ? "TRUE" : "FALSE");
+		res = snprintf(buf, size, mem->u.b ? "TRUE" : "FALSE");
+		break;
 	default:
-		return snprintf(buf, size, "unknown");
+		unreachable();
 	}
+	assert(res >= 0);
+	return res;
+}
+
+char *
+mem_strdup(const struct Mem *mem)
+{
+	int size = mem_snprintf(NULL, 0, mem);
+	assert(size >= 0);
+	char *str = sqlDbMallocRawNN(sql_get(), size + 1);
+	if (str == NULL)
+		return NULL;
+	mem_snprintf(str, size + 1, mem);
+	return str;
 }
 
 const char *
 mem_str(const struct Mem *mem)
 {
+	if (mem->type == MEM_TYPE_NULL)
+		return "NULL";
 	const char *type = mem_type_to_str(mem);
 	if (mem->type == MEM_TYPE_STR) {
 		if (mem->n <= STR_VALUE_MAX_LEN)
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 75b9dc858..1a3478f9a 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -254,6 +254,13 @@ mem_is_field_compatible(const struct Mem *mem, enum field_type type);
 int
 mem_snprintf(char *buf, uint32_t size, const struct Mem *mem);
 
+/**
+ * Returns a NULL-terminated string representation of a MEM. Memory for the
+ * result was allocated using sqlDbMallocRawNN() and should be freed.
+ */
+char *
+mem_strdup(const struct Mem *mem);
+
 /**
  * Return a string that contains description of type and value of MEM. String is
  * either allocated using static_alloc() of just a static variable. This
diff --git a/src/box/sql/printf.c b/src/box/sql/printf.c
index 08e18c15d..9caed7aa0 100644
--- a/src/box/sql/printf.c
+++ b/src/box/sql/printf.c
@@ -160,13 +160,7 @@ getTextArg(PrintfArguments * p)
 {
 	if (p->nArg <= p->nUsed)
 		return 0;
-	const struct Mem *mem = &p->apArg[p->nUsed++];
-	uint32_t size = mem_snprintf(NULL, 0, mem);
-	char *str = sqlDbMallocRawNN(sql_get(), size + 1);
-	if (str == NULL)
-		return NULL;
-	mem_snprintf(str, size + 1, mem);
-	return str;
+	return mem_strdup(&p->apArg[p->nUsed++]);
 }
 
 /*


New patch:

commit 870076945970c73ed651c37088406e7c939c6294
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Fri Oct 22 11:44:06 2021 +0300

    sql: introduce mem_snprintf()
    
    This patch introduces the mem_snprintf() function, which writes the
    string representation of a MEM to buf.

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 5abaf490d..26ccebf83 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -869,9 +869,11 @@ func_printf(struct sql_context *ctx, int argc, const struct Mem *argv)
 	if (argc < 1 || mem_is_null(&argv[0]))
 		return;
 	if (argc == 1 || !mem_is_str(&argv[0])) {
-		struct Mem *mem = ctx->pOut;
-		if (mem_copy(mem, &argv[0]) != 0 || mem_to_str(mem) != 0)
+		char *str = mem_strdup(&argv[0]);
+		if (str == NULL)
 			ctx->is_aborted = true;
+		else
+			mem_set_str0_allocated(ctx->pOut, str);
 		return;
 	}
 	struct PrintfArguments pargs;
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index f67742618..234a00d57 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -116,30 +116,76 @@ mem_is_field_compatible(const struct Mem *mem, enum field_type type)
 	return field_mp_plain_type_is_compatible(type, mp_type, true);
 }
 
-const char *
-mem_str(const struct Mem *mem)
+int
+mem_snprintf(char *buf, uint32_t size, const struct Mem *mem)
 {
-	char buf[STR_VALUE_MAX_LEN];
-	const char *type = mem_type_to_str(mem);
+	int res = -1;
 	switch (mem->type) {
 	case MEM_TYPE_NULL:
-		return "NULL";
+		res = snprintf(buf, size, "NULL");
+		break;
 	case MEM_TYPE_STR:
+	case MEM_TYPE_BIN:
+		res = snprintf(buf, size, "%.*s", mem->n, mem->z);
+		break;
+	case MEM_TYPE_INT:
+		res = snprintf(buf, size, "%lld", mem->u.i);
+		break;
+	case MEM_TYPE_UINT:
+		res = snprintf(buf, size, "%llu", (unsigned long long)mem->u.u);
+		break;
+	case MEM_TYPE_DOUBLE: {
+		char str[BUF_SIZE];
+		sql_snprintf(BUF_SIZE, str, "%!.15g", mem->u.r);
+		res = snprintf(buf, size, "%s", str);
+		break;
+	}
+	case MEM_TYPE_DEC:
+		res = snprintf(buf, size, "%s", decimal_str(&mem->u.d));
+		break;
+	case MEM_TYPE_MAP:
+	case MEM_TYPE_ARRAY:
+		res = mp_snprint(buf, size, mem->z);
+		break;
+	case MEM_TYPE_UUID:
+		res = snprintf(buf, size, "%s", tt_uuid_str(&mem->u.uuid));
+		break;
+	case MEM_TYPE_BOOL:
+		res = snprintf(buf, size, mem->u.b ? "TRUE" : "FALSE");
+		break;
+	default:
+		unreachable();
+	}
+	assert(res >= 0);
+	return res;
+}
+
+char *
+mem_strdup(const struct Mem *mem)
+{
+	int size = mem_snprintf(NULL, 0, mem);
+	assert(size >= 0);
+	char *str = sqlDbMallocRawNN(sql_get(), size + 1);
+	if (str == NULL)
+		return NULL;
+	mem_snprintf(str, size + 1, mem);
+	return str;
+}
+
+const char *
+mem_str(const struct Mem *mem)
+{
+	if (mem->type == MEM_TYPE_NULL)
+		return "NULL";
+	const char *type = mem_type_to_str(mem);
+	if (mem->type == MEM_TYPE_STR) {
 		if (mem->n <= STR_VALUE_MAX_LEN)
 			return tt_sprintf("%s('%.*s')", type, mem->n, mem->z);
 		return tt_sprintf("%s('%.*s...)", type, STR_VALUE_MAX_LEN,
 				  mem->z);
-	case MEM_TYPE_INT:
-		return tt_sprintf("%s(%lld)", type, mem->u.i);
-	case MEM_TYPE_UINT:
-		return tt_sprintf("%s(%llu)", type, mem->u.u);
-	case MEM_TYPE_DOUBLE:
-		sql_snprintf(STR_VALUE_MAX_LEN, buf, "%!.15g", mem->u.r);
-		return tt_sprintf("%s(%s)", type, buf);
-	case MEM_TYPE_DEC:
-		decimal_to_string(&mem->u.d, buf);
-		return tt_sprintf("%s(%s)", type, buf);
-	case MEM_TYPE_BIN: {
+	}
+	char buf[STR_VALUE_MAX_LEN];
+	if (mem->type == MEM_TYPE_BIN) {
 		int len = MIN(mem->n, STR_VALUE_MAX_LEN / 2);
 		for (int i = 0; i < len; ++i) {
 			int n = (mem->z[i] & 0xF0) >> 4;
@@ -151,24 +197,10 @@ mem_str(const struct Mem *mem)
 			return tt_sprintf("%s(x'%.*s...)", type, len * 2, buf);
 		return tt_sprintf("%s(x'%.*s')", type, len * 2, buf);
 	}
-	case MEM_TYPE_MAP:
-	case MEM_TYPE_ARRAY: {
-		const char *str = mp_str(mem->z);
-		uint32_t len = strlen(str);
-		uint32_t minlen = MIN(STR_VALUE_MAX_LEN, len);
-		memcpy(buf, str, minlen);
-		if (len <= STR_VALUE_MAX_LEN)
-			return tt_sprintf("%s(%.*s)", type, minlen, buf);
-		return tt_sprintf("%s(%.*s...)", type, minlen, buf);
-	}
-	case MEM_TYPE_UUID:
-		tt_uuid_to_string(&mem->u.uuid, buf);
+	int size = mem_snprintf(buf, STR_VALUE_MAX_LEN, mem);
+	if (size <= STR_VALUE_MAX_LEN)
 		return tt_sprintf("%s(%s)", type, buf);
-	case MEM_TYPE_BOOL:
-		return tt_sprintf("%s(%s)", type, mem->u.b ? "TRUE" : "FALSE");
-	default:
-		return "unknown";
-	}
+	return tt_sprintf("%s(%.*s...)", type, STR_VALUE_MAX_LEN, buf);
 }
 
 static const char *
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 9d5245708..1a3478f9a 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -246,6 +246,21 @@ mem_is_any_null(const struct Mem *mem1, const struct Mem *mem2)
 bool
 mem_is_field_compatible(const struct Mem *mem, enum field_type type);
 
+/**
+ * Write a NULL-terminated string representation of a MEM to buf. Returns the
+ * number of bytes required to write the value, excluding '\0'. If the return
+ * value is equal to or greater than size, then the value has been truncated.
+ */
+int
+mem_snprintf(char *buf, uint32_t size, const struct Mem *mem);
+
+/**
+ * Returns a NULL-terminated string representation of a MEM. Memory for the
+ * result was allocated using sqlDbMallocRawNN() and should be freed.
+ */
+char *
+mem_strdup(const struct Mem *mem);
+
 /**
  * Return a string that contains description of type and value of MEM. String is
  * either allocated using static_alloc() of just a static variable. This
diff --git a/src/box/sql/printf.c b/src/box/sql/printf.c
index 8da7c9878..9caed7aa0 100644
--- a/src/box/sql/printf.c
+++ b/src/box/sql/printf.c
@@ -160,20 +160,7 @@ getTextArg(PrintfArguments * p)
 {
 	if (p->nArg <= p->nUsed)
 		return 0;
-	struct Mem mem;
-	mem_create(&mem);
-	mem_copy_as_ephemeral(&mem, &p->apArg[p->nUsed++]);
-	if (mem_to_str(&mem) != 0) {
-		mem_destroy(&mem);
-		return NULL;
-	}
-	char *str = sqlDbMallocRawNN(sql_get(), mem.n + 1);
-	if (str == NULL)
-		return NULL;
-	memcpy(str, mem.z, mem.n);
-	str[mem.n] = '\0';
-	mem_destroy(&mem);
-	return str;
+	return mem_strdup(&p->apArg[p->nUsed++]);
 }
 
 /*
diff --git a/test/sql-tap/sql-errors.test.lua b/test/sql-tap/sql-errors.test.lua
index a9aa5acf7..6cecd7c6d 100755
--- a/test/sql-tap/sql-errors.test.lua
+++ b/test/sql-tap/sql-errors.test.lua
@@ -812,7 +812,8 @@ test:do_catchsql_test(
 	"SELECT CAST(a AS UNSIGNED) from test;", {
 		1, 'Type mismatch: can not convert array(["aaaaaaaaaaaaaaaaaa'..
 		'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'..
-		'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...) to unsigned'
+		'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...) to '..
+		'unsigned'
 	})
 
 test:do_catchsql_test(
@@ -820,7 +821,7 @@ test:do_catchsql_test(
 	"SELECT CAST(m AS UNSIGNED) from test;", {
 		1, 'Type mismatch: can not convert map({"a": 1, "b": "aaaaaaa'..
 		'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'..
-		'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...) to unsigned'
+		'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...) to unsigned'
 	})
 
 test:execsql('DROP TABLE test;')
diff --git a/test/sql/types.result b/test/sql/types.result
index 947795b03..5a822fddf 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -1623,7 +1623,7 @@ box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
 - null
 - 'Type mismatch: can not convert array([1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13,
   14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33,
-  34, ...) to scalar'
+  34,...) to scalar'
 ...
 s:drop()
 ---