Re: [Tarantool-patches] [PATCH v1 20/21] sql: remove MEM_Term flag

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

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

On Fri, Oct 15, 2021 at 12:47:33AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/box/sql/printf.c b/src/box/sql/printf.c
> > index b4ab0d0f9..f261a731c 100644
> > --- a/src/box/sql/printf.c
> > +++ b/src/box/sql/printf.c
> > @@ -160,8 +160,22 @@ getTextArg(PrintfArguments * p)
> >  {
> >  	if (p->nArg <= p->nUsed)
> >  		return 0;
> > -	struct Mem *mem = p->apArg[p->nUsed++];
> > -	return (char *)mem_as_str0(mem);
> > +	struct Mem mem;
> > +	mem_create(&mem);
> > +	mem_copy(&mem, p->apArg[p->nUsed++]);
> 
> 1. mem_copy_as_ephemeral() would be enough. But would be even
> better to have mem_snprintf() which would work like snprintf -
> save string representation of mem into a provided buffer and
> return how many bytes written / need to be written. This though
> could be postponed indefinitely, I don't mind.
> 
I wrote mem_snprintf (), but decided to introduce it a little later, as there
are a couple of questions that need to be answered.

> > +	if (mem_to_str(&mem) != 0) {
> > +		mem_destroy(&mem);
> > +		return NULL;
> > +	}
> > +	char *str = region_alloc(&fiber()->gc, mem.n + 1);
> 
> 2. printf is called for each row. Which means it will leak the
> region. You can either do the snprintf-thing described above to
> write directly into the needed buffer, or at least cleanup the
> region using a save point in the end of sqlVXPrintf().
> 
I edited this code so that now the line will be freed correctly.

> > diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
> > index 6849f13ec..ad064d500 100644
> > --- a/src/box/sql/whereexpr.c
> > +++ b/src/box/sql/whereexpr.c
> > @@ -307,12 +307,18 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
> >  
> >  	op = pRight->op;
> >  	if (op == TK_VARIABLE) {
> > -		Vdbe *pReprepare = pParse->pReprepare;
> > -		int iCol = pRight->iColumn;
> > -		pVal = sqlVdbeGetBoundValue(pReprepare, iCol);
> > -		if (pVal != NULL && mem_is_str(pVal)) {
> > -			if (mem_as_str0(pVal) == NULL)
> > +		var = vdbe_get_bound_value(pParse->pReprepare,
> > +					   pRight->iColumn - 1);
> > +		if (var != NULL && mem_is_str(var)) {
> > +			uint32_t size = var->n + 1;
> > +			char *str = region_alloc(&fiber()->gc, size);
> 
> 3. You leak the fiber's region. AFAIR, it is not cleaned up in the
> end of parsing. Try to use pParse->region.
> 
Thanks, fixed. Also, I added region_truncate() at the end of the function.

> > +			if (str == NULL) {
> > +				diag_set(OutOfMemory, size, "region", "str");
> >  				return -1;
> > +			}
> > +			memcpy(str, var->z, var->n);
> > +			str[var->n] = '\0';
> > +			z = str;
> >  		}


Diff:

diff --git a/src/box/sql/printf.c b/src/box/sql/printf.c
index 066aaed9e..2c9fcd467 100644
--- a/src/box/sql/printf.c
+++ b/src/box/sql/printf.c
@@ -167,11 +167,9 @@ getTextArg(PrintfArguments * p)
 		mem_destroy(&mem);
 		return NULL;
 	}
-	char *str = region_alloc(&fiber()->gc, mem.n + 1);
-	if (str == NULL) {
-		diag_set(OutOfMemory, mem.n + 1, "region", "str");
+	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);
@@ -691,6 +689,7 @@ sqlVXPrintf(StrAccum * pAccum,	/* Accumulate results here */
 			if (bArgList) {
 				bufpt = getTextArg(pArgList);
 				c = bufpt ? bufpt[0] : 0;
+				zExtra = bufpt;
 			} else {
 				c = va_arg(ap, int);
 			}
@@ -711,7 +710,7 @@ sqlVXPrintf(StrAccum * pAccum,	/* Accumulate results here */
 		case etDYNSTRING:
 			if (bArgList) {
 				bufpt = getTextArg(pArgList);
-				xtype = etSTRING;
+				xtype = etDYNSTRING;
 			} else {
 				bufpt = va_arg(ap, char *);
 			}
@@ -741,6 +740,7 @@ sqlVXPrintf(StrAccum * pAccum,	/* Accumulate results here */
 
 				if (bArgList) {
 					escarg = getTextArg(pArgList);
+					zExtra = escarg;
 				} else {
 					escarg = va_arg(ap, char *);
 				}
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index ad064d500..08a1b4e3a 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -268,7 +268,6 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 	int cnt;
 	/* Database connection. */
 	sql *db = pParse->db;
-	const struct Mem *var = NULL;
 	/* Opcode of pRight. */
 	int op;
 	/* Result code to return. */
@@ -306,12 +305,15 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 		return 0;
 
 	op = pRight->op;
+	struct region *region = &pParse->region;
+	size_t svp = region_used(region);
 	if (op == TK_VARIABLE) {
-		var = vdbe_get_bound_value(pParse->pReprepare,
-					   pRight->iColumn - 1);
+		Vdbe *pReprepare = pParse->pReprepare;
+		int iCol = pRight->iColumn;
+		const struct Mem *var = vdbe_get_bound_value(pReprepare, iCol);
 		if (var != NULL && mem_is_str(var)) {
 			uint32_t size = var->n + 1;
-			char *str = region_alloc(&fiber()->gc, size);
+			char *str = region_alloc(region, size);
 			if (str == NULL) {
 				diag_set(OutOfMemory, size, "region", "str");
 				return -1;
@@ -362,6 +364,7 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 		}
 	}
 
+	region_truncate(region, svp);
 	rc = (z != 0);
 	return rc;
 }


New patch:

commit fa3a78f95d3391c1629e31e2618d956c9ece4d14
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Oct 7 17:40:14 2021 +0300

    sql: remove MEM_Term flag
    
    This patch removes the MEM_Term flag, because after changes in the SQL
    built-in functions, this flag is no longer used.
    
    Needed for #4145

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 4150a24f0..1f5df3332 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -346,21 +346,18 @@ void
 mem_set_str0_ephemeral(struct Mem *mem, char *value)
 {
 	set_str_const(mem, value, strlen(value), MEM_Ephem);
-	mem->flags |= MEM_Term;
 }
 
 void
 mem_set_str0_static(struct Mem *mem, char *value)
 {
 	set_str_const(mem, value, strlen(value), MEM_Static);
-	mem->flags |= MEM_Term;
 }
 
 void
 mem_set_str0_allocated(struct Mem *mem, char *value)
 {
 	set_str_dynamic(mem, value, strlen(value), 0);
-	mem->flags |= MEM_Term;
 }
 
 int
@@ -392,7 +389,6 @@ mem_copy_str0(struct Mem *mem, const char *value)
 	if (mem_copy_str(mem, value, len + 1) != 0)
 		return -1;
 	mem->n = len;
-	mem->flags |= MEM_Term;
 	return 0;
 }
 
@@ -657,24 +653,12 @@ int_to_str0(struct Mem *mem)
 	return mem_copy_str0(mem, str);
 }
 
-static inline int
-str_to_str0(struct Mem *mem)
-{
-	assert(mem->type == MEM_TYPE_STR);
-	if (sqlVdbeMemGrow(mem, mem->n + 1, 1) != 0)
-		return -1;
-	mem->z[mem->n] = '\0';
-	mem->flags |= MEM_Term;
-	mem->flags &= ~MEM_Scalar;
-	return 0;
-}
-
 static inline int
 str_to_bin(struct Mem *mem)
 {
 	assert(mem->type == MEM_TYPE_STR);
 	mem->type = MEM_TYPE_BIN;
-	mem->flags &= ~(MEM_Term | MEM_Scalar);
+	mem->flags &= ~MEM_Scalar;
 	return 0;
 }
 
@@ -725,18 +709,6 @@ bin_to_str(struct Mem *mem)
 	return 0;
 }
 
-static inline int
-bin_to_str0(struct Mem *mem)
-{
-	assert(mem->type == MEM_TYPE_BIN);
-	if (sqlVdbeMemGrow(mem, mem->n + 1, 1) != 0)
-		return -1;
-	mem->z[mem->n] = '\0';
-	mem->type = MEM_TYPE_STR;
-	mem->flags = MEM_Term;
-	return 0;
-}
-
 static inline int
 bin_to_uuid(struct Mem *mem)
 {
@@ -1002,7 +974,7 @@ double_to_str0(struct Mem *mem)
 	sql_snprintf(BUF_SIZE, mem->z, "%!.15g", mem->u.r);
 	mem->n = strlen(mem->z);
 	mem->type = MEM_TYPE_STR;
-	mem->flags = MEM_Term;
+	mem->flags = 0;
 	return 0;
 }
 
@@ -1284,39 +1256,6 @@ mem_to_number(struct Mem *mem)
 	return -1;
 }
 
-int
-mem_to_str0(struct Mem *mem)
-{
-	assert(mem->type < MEM_TYPE_INVALID);
-	switch (mem->type) {
-	case MEM_TYPE_STR:
-		if ((mem->flags & MEM_Term) != 0) {
-			mem->flags &= ~MEM_Scalar;
-			return 0;
-		}
-		return str_to_str0(mem);
-	case MEM_TYPE_INT:
-	case MEM_TYPE_UINT:
-		return int_to_str0(mem);
-	case MEM_TYPE_DOUBLE:
-		return double_to_str0(mem);
-	case MEM_TYPE_BOOL:
-		return bool_to_str0(mem);
-	case MEM_TYPE_BIN:
-		return bin_to_str0(mem);
-	case MEM_TYPE_MAP:
-		return map_to_str0(mem);
-	case MEM_TYPE_ARRAY:
-		return array_to_str0(mem);
-	case MEM_TYPE_UUID:
-		return uuid_to_str0(mem);
-	case MEM_TYPE_DEC:
-		return dec_to_str0(mem);
-	default:
-		return -1;
-	}
-}
-
 int
 mem_to_str(struct Mem *mem)
 {
@@ -1763,15 +1702,6 @@ mem_get_bool(const struct Mem *mem, bool *b)
 	return -1;
 }
 
-int
-mem_get_str0(const struct Mem *mem, const char **s)
-{
-	if (mem->type != MEM_TYPE_STR || (mem->flags & MEM_Term) == 0)
-		return -1;
-	*s = mem->z;
-	return 0;
-}
-
 int
 mem_get_bin(const struct Mem *mem, const char **s)
 {
@@ -1814,7 +1744,7 @@ mem_copy(struct Mem *to, const struct Mem *from)
 	to->szMalloc = sqlDbMallocSize(to->db, to->zMalloc);
 	memcpy(to->zMalloc, to->z, to->n);
 	to->z = to->zMalloc;
-	to->flags &= MEM_Term;
+	to->flags = 0;
 	return 0;
 }
 
@@ -1831,7 +1761,7 @@ mem_copy_as_ephemeral(struct Mem *to, const struct Mem *from)
 		return;
 	if ((to->flags & (MEM_Static | MEM_Ephem)) != 0)
 		return;
-	to->flags &= MEM_Term;
+	to->flags = 0;
 	to->flags |= MEM_Ephem;
 	return;
 }
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index d2e9cc135..1ef8a945b 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -98,13 +98,6 @@ struct Mem {
 /** MEM is of SCALAR meta-type. */
 #define MEM_Scalar    0x0002
 #define MEM_Cleared   0x0200	/* NULL set by OP_Null, not from data */
-
-/* Whenever Mem contains a valid string or blob representation, one of
- * the following flags must be set to determine the memory management
- * policy for Mem.z.  The MEM_Term flag tells us whether or not the
- * string is \000 or \u0000 terminated
- */
-#define MEM_Term      0x0400	/* String rep is nul terminated */
 #define MEM_Static    0x1000	/* Mem.z points to a static string */
 #define MEM_Ephem     0x2000	/* Mem.z points to an ephemeral string */
 
@@ -610,14 +603,6 @@ mem_to_number(struct Mem *mem);
 int
 mem_to_str(struct Mem *mem);
 
-/**
- * Convert the given MEM to STRING. This function and the function above define
- * the rules that are used to convert values of all other types to STRING. In
- * this function, the string received after convertion is NULL-terminated.
- */
-int
-mem_to_str0(struct Mem *mem);
-
 /** Convert the given MEM to given type according to explicit cast rules. */
 int
 mem_cast_explicit(struct Mem *mem, enum field_type type);
@@ -722,27 +707,6 @@ mem_get_bool_unsafe(const struct Mem *mem)
 	return b;
 }
 
-/**
- * Return value for MEM of STRING type if MEM contains a NULL-terminated string.
- * Otherwise convert value of the MEM to NULL-terminated string if possible and
- * return converted value. Original MEM is not changed.
- */
-int
-mem_get_str0(const struct Mem *mem, const char **s);
-
-/**
- * Return value for MEM of STRING type if MEM contains NULL-terminated string.
- * Otherwise convert MEM to MEM of string type that contains NULL-terminated
- * string and return its value. Return NULL if conversion is impossible.
- */
-static inline const char *
-mem_as_str0(struct Mem *mem)
-{
-	if (mem_to_str0(mem) != 0)
-		return NULL;
-	return mem->z;
-}
-
 /**
  * Return value for MEM of VARBINARY type. For MEM of all other types convert
  * value of the MEM to VARBINARY if possible and return converted value.
diff --git a/src/box/sql/printf.c b/src/box/sql/printf.c
index 5b61646e3..8da7c9878 100644
--- a/src/box/sql/printf.c
+++ b/src/box/sql/printf.c
@@ -160,8 +160,20 @@ getTextArg(PrintfArguments * p)
 {
 	if (p->nArg <= p->nUsed)
 		return 0;
-	struct Mem *mem = &p->apArg[p->nUsed++];
-	return (char *)mem_as_str0(mem);
+	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;
 }
 
 /*
@@ -677,6 +689,7 @@ sqlVXPrintf(StrAccum * pAccum,	/* Accumulate results here */
 			if (bArgList) {
 				bufpt = getTextArg(pArgList);
 				c = bufpt ? bufpt[0] : 0;
+				zExtra = bufpt;
 			} else {
 				c = va_arg(ap, int);
 			}
@@ -697,7 +710,7 @@ sqlVXPrintf(StrAccum * pAccum,	/* Accumulate results here */
 		case etDYNSTRING:
 			if (bArgList) {
 				bufpt = getTextArg(pArgList);
-				xtype = etSTRING;
+				xtype = etDYNSTRING;
 			} else {
 				bufpt = va_arg(ap, char *);
 			}
@@ -727,6 +740,7 @@ sqlVXPrintf(StrAccum * pAccum,	/* Accumulate results here */
 
 				if (bArgList) {
 					escarg = getTextArg(pArgList);
+					zExtra = escarg;
 				} else {
 					escarg = va_arg(ap, char *);
 				}
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index e482926b2..22e3370a2 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -256,6 +256,9 @@ vdbe_metadata_set_col_autoincrement(struct Vdbe *p, int idx);
 int
 vdbe_metadata_set_col_span(struct Vdbe *p, int idx, const char *span);
 
+const struct Mem *
+vdbe_get_bound_value(struct Vdbe *vdbe, int id);
+
 void sqlVdbeCountChanges(Vdbe *);
 sql *sqlVdbeDb(Vdbe *);
 void sqlVdbeSetSql(Vdbe *, const char *z, int n);
@@ -264,7 +267,6 @@ void sqlVdbeSwap(Vdbe *, Vdbe *);
 struct VdbeOp *
 sqlVdbeTakeOpArray(struct Vdbe *p, int *pnOp);
 
-sql_value *sqlVdbeGetBoundValue(Vdbe *, int);
 char *sqlVdbeExpandSql(Vdbe *, const char *);
 
 /**
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index f5a84956c..5e7a1caff 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2280,31 +2280,12 @@ sqlVdbeDb(Vdbe * v)
 	return v->db;
 }
 
-/*
- * Return a pointer to an sql_value structure containing the value bound
- * parameter iVar of VM v. Except, if the value is an SQL NULL, return
- * 0 instead. Unless it is NULL, apply type to the value before returning it.
- *
- * The returned value must be freed by the caller using sqlValueFree().
- */
-sql_value *
-sqlVdbeGetBoundValue(struct Vdbe *v, int iVar)
-{
-	assert(iVar > 0);
-	if (v) {
-		Mem *pMem = &v->aVar[iVar - 1];
-		if (!mem_is_null(pMem)) {
-			sql_value *pRet = sqlValueNew(v->db);
-			if (pRet == NULL)
-				return NULL;
-			if (mem_copy(pRet, pMem) != 0) {
-				sqlValueFree(pRet);
-				return NULL;
-			}
-			return pRet;
-		}
-	}
-	return 0;
+const struct Mem *
+vdbe_get_bound_value(struct Vdbe *vdbe, int id)
+{
+	if (vdbe == NULL || id < 0 || id >= vdbe->nVar)
+		return NULL;
+	return &vdbe->aVar[id];
 }
 
 void
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index 6849f13ec..08a1b4e3a 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -268,7 +268,6 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 	int cnt;
 	/* Database connection. */
 	sql *db = pParse->db;
-	sql_value *pVal = 0;
 	/* Opcode of pRight. */
 	int op;
 	/* Result code to return. */
@@ -306,13 +305,22 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 		return 0;
 
 	op = pRight->op;
+	struct region *region = &pParse->region;
+	size_t svp = region_used(region);
 	if (op == TK_VARIABLE) {
 		Vdbe *pReprepare = pParse->pReprepare;
 		int iCol = pRight->iColumn;
-		pVal = sqlVdbeGetBoundValue(pReprepare, iCol);
-		if (pVal != NULL && mem_is_str(pVal)) {
-			if (mem_as_str0(pVal) == NULL)
+		const struct Mem *var = vdbe_get_bound_value(pReprepare, iCol);
+		if (var != NULL && mem_is_str(var)) {
+			uint32_t size = var->n + 1;
+			char *str = region_alloc(region, size);
+			if (str == NULL) {
+				diag_set(OutOfMemory, size, "region", "str");
 				return -1;
+			}
+			memcpy(str, var->z, var->n);
+			str[var->n] = '\0';
+			z = str;
 		}
 		assert(pRight->op == TK_VARIABLE || pRight->op == TK_REGISTER);
 	} else if (op == TK_STRING) {
@@ -356,8 +364,8 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 		}
 	}
 
+	region_truncate(region, svp);
 	rc = (z != 0);
-	sqlValueFree(pVal);
 	return rc;
 }