[Tarantool-patches] [PATCH v1 20/21] sql: remove MEM_Term flag
Mergen Imeev
imeevma at tarantool.org
Mon Oct 25 12:57:27 MSK 2021
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 at 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;
}
More information about the Tarantool-patches
mailing list