Re: [Tarantool-patches] [PATCH v5 27/52] sql: introduce mem_set_str_*() functions

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

Thanks for working on this!

>>> index ae55e4c29..171cb8946 100644
>>> --- a/src/box/sql/vdbeaux.c
>>> +++ b/src/box/sql/vdbeaux.c
>>> @@ -1333,41 +1331,34 @@ sqlVdbeList(Vdbe * p)
>>>  		mem_set_int(pMem, pOp->p3, pOp->p3 < 0);
>>>  		pMem++;
>>>  
>>> -		if (sqlVdbeMemClearAndResize(pMem, 256)) {
>>> -			assert(p->db->mallocFailed);
>>> +		char *buf = sqlDbMallocRaw(sql_get(), 256);
>>
>> 2. I think you need some kind of mem_set_strlen(), or mem_grow()/mem_reserve(),
>> or something else to reserve the memory. To extend zMalloc. Otherwise you
>> can't reuse the memory which might already be in the mem object.
> Wouldn't mem_copy_*() be enough? In general, allocated by MEM memory should not
> be accessed for changing (except for MEM_Agg).

Both your current solution and mem_copy suffer from unnecessary copying.
In the old code the string was created in-place. This is what I am talking
about.

But nevermind, it is not a perf-critical place as I see. Can be tweaked
later.

> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index 6c08e772d..484c66b29 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -125,6 +125,12 @@ setResultStrOrError(sql_context * pCtx,	/* Function context */
>  		    void (*xDel) (void *)	/* Destructor function */
>      )
>  {
> +	if (xDel == SQL_STATIC)
> +		return mem_set_strl_static(pCtx->pOut, (char *)z, n);
> +	if (xDel == SQL_DYNAMIC)
> +		return mem_set_strl_allocated(pCtx->pOut, (char *)z, n);
> +	if (xDel != SQL_TRANSIENT)
> +		return mem_set_strl_dynamic(pCtx->pOut, (char *)z, n);

I just can't help myself. It makes my eyes bleed - for dynamic we use
allocated, and for non-transient we use dynamic! I see why, but I would
want us to hide it as deep inside of mem.c as possible, so as we at
least could change the names in a not very intrusive manner in the
future. Or delete it in future entirely.

Please, move this hunk and the one below to a separate functions
like mem_set_strl(Mem, str, len_hint, destructor). Then it would be in
one terrible place instead of 2 terrible places.

The same for the bins.

>  	if (sqlVdbeMemSetStr(pCtx->pOut, z, n, 1, xDel) != 0)
>  		pCtx->is_aborted = true;
>  }
> @@ -762,7 +768,13 @@ bindText(sql_stmt * pStmt,	/* The statement to bind against */
>  	if (zData == NULL)
>  		return 0;
>  	pVar = &p->aVar[i - 1];
> -	if (sqlVdbeMemSetStr(pVar, zData, nData, 1, xDel) != 0)
> +	if (xDel == SQL_STATIC)
> +		mem_set_strl_static(pVar, (char *)zData, nData);
> +	else if (xDel == SQL_DYNAMIC)
> +		mem_set_strl_allocated(pVar, (char *)zData, nData);
> +	else if (xDel != SQL_TRANSIENT)
> +		mem_set_strl_dynamic(pVar, (char *)zData, nData);
> +	else if (sqlVdbeMemSetStr(pVar, zData, nData, 1, xDel) != 0)
>  		return -1;
>  	return sql_bind_type(p, i, "text");
>  }