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

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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;
>  		}