[Tarantool-patches] [PATCH v1 20/21] sql: remove MEM_Term flag
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Oct 15 01:47:33 MSK 2021
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.
> + 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().
> 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.
> + if (str == NULL) {
> + diag_set(OutOfMemory, size, "region", "str");
> return -1;
> + }
> + memcpy(str, var->z, var->n);
> + str[var->n] = '\0';
> + z = str;
> }
More information about the Tarantool-patches
mailing list