[tarantool-patches] Re: [PATCH 5/8] sql: replace field type with affinity for VDBE runtime

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes!

On 28/01/2019 19:39, n.pettik wrote:
> 
>>>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>>>>> index 917e6e30b..c823c5a06 100644
>>>>> --- a/src/box/sql/expr.c
>>>>> +++ b/src/box/sql/expr.c
>>>>> @@ -2858,8 +2858,11 @@ sqlite3CodeSubselect(Parse * pParse,	/* Parsing context */
>>>>>   						jmpIfDynamic = -1;
>>>>>   					}
>>>>>   					r3 = sqlite3ExprCodeTarget(pParse, pE2, r1);
>>>>> +					char type =
>>>>> +						sql_affinity_to_field_type(affinity);
>>>>>   	 				sqlite3VdbeAddOp4(v, OP_MakeRecord, r3,
>>>>> -							  1, r2, &affinity, 1);
>>>>> +							  1, r2, &type,
>>>>> +							  1);
>>>>
>>>> 4. I do not understand. Is p4type of sqlite3VdbeAddOp4 of OP_MakeRecord
>>>> a length of an affinity string, or a type of its allocation? Or both?
>>> Both. p4type > 0 means that p4type bytes are copied to freshly allocated memory
>>> and P4 set to DYNAMIC. sqlite3DbStrNDup() in vdbeChangeP4Full() reserves
>>> one more byte for NULL termination.
>>
>> I see now, that OP_MakeRecord uses only p4.z and does not touch p4type, so it makes
>> no sense to pass here length of the field_type array. Please, use only P4_STATIC and
>> P4_DYNAMIC. At this moment this opcode is mess: somewhere 0 is passed, somewhere
>> P4_DYNAMIC, in other places length of a field_type array.
> 
> Well, actually we can’t pass here neither of these args and that’s why:
> STATIC and DYNAMIC indicate that memory which is passed through
> P4 argument will be copied to new chunk of memory:
> 
>     985	void
>     986	sqlite3VdbeChangeP4(Vdbe * p, int addr, const char *zP4, int n)
>     987	{
>     
> …
> 
>    1008		if (n == P4_INT32) {
>    1009			/* Note: this cast is safe, because the origin data point was an int
>    1010			 * that was cast to a (const char *).
>    1011			 */
>    1012			pOp->p4.i = SQLITE_PTR_TO_INT(zP4);
>    1013			pOp->p4type = P4_INT32;
>    1014		} if (n == P4_BOOL) {
>    1015			pOp->p4.b = *(bool*)zP4;
>    1016			pOp->p4type = P4_BOOL;
>    1017		} else {
>    1018			assert(n < 0);
>    1019			pOp->p4.p = (void *)zP4;
>    1020			pOp->p4type = (signed char)n;
>    1021		}
>    1022	}
> 
> Both P4_STATIC and P4_DYNAMIC are less than 0, so pOp->p4.p points
> to object on allocated on stack (in this particular case).
> 
> Passing n > 0 we are implying that n bytes of memory starting from zP4
> must be copied to new chunk, which comes with P4_DYNAMIC
> 
> If you still have doubts, I assure you that I’ve already tried to replace it and
> always get assertion faults like:
> Assertion failed: (f_type < field_type_MAX), function mem_apply_type, file tarantool/src/box/sql/vdbe.c, line 334.
> 
> In other places (where we use P4_DYNAMIC), field_type array is allocated
> on heap, so it can be freed after VDBE execution.

Ok, now I got it. Thanks for the explanation.

> 
> As an option we can allocate on heap this string before calling
> sqlite3VdbeChangeP4, like this:
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index ca39faf51..27bbc1b1c 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -2860,11 +2860,15 @@ sqlite3CodeSubselect(Parse * pParse,    /* Parsing context */
>                                          r3 = sqlite3ExprCodeTarget(pParse, pE2, r1);
>                                          enum field_type type =
>                                                  sql_affinity_to_field_type(affinity);
> -                                       enum field_type types[2] =
> -                                               { type, field_type_MAX };
> +                                       size_t sz = 2 * sizeof(enum field_type);
> +                                       enum field_type *types =
> +                                               sqlite3DbMallocRaw(pParse->db,
> +                                                                  sz);
> +                                       types[0] = type;
> +                                       types[1] = field_type_MAX;
>                                          sqlite3VdbeAddOp4(v, OP_MakeRecord, r3,
>                                                            1, r2, (char *)types,
> -                                                         sizeof(types));
> +                                                         P4_DYNAMIC);

No, never mind. The current way is ok.

>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>>> index 0a80ca622..ca39faf51 100644
>>> --- a/src/box/sql/expr.c
>>> +++ b/src/box/sql/expr.c
>>> @@ -3172,7 +3177,10 @@ sqlite3ExprCodeIN(Parse * pParse,        /* Parsing and code generating context */
>>>           * of the RHS using the LHS as a probe.  If found, the result is
>>>           * true.
>>>           */
>>> -       sqlite3VdbeAddOp4(v, OP_Affinity, rLhs, nVector, 0, zAff, nVector);
>>> +       enum field_type *types = sql_affinity_str_to_field_type_str(zAff);
>>> +       types[nVector] = field_type_MAX;
>>> +       sqlite3VdbeAddOp4(v, OP_ApplyType, rLhs, nVector, 0, (char *)types,
>>> +                         P4_DYNAMIC);
>>
>> 2. Why do you need types[nVector] = field_type_MAX? As I see, before your patch
>> there was no additional zero termination.
> 
> To cut off array of types. Before my patch, first nVector bytes
> were copied inside vdbeChangeP4Full() and automatically
> terminated with NULL (sqlite3DbStrNDup).

Still do not understand. As I see, zAff was taken from exprINAffinity().
This function allocates a string of affinities of length
sqlite3ExprVectorSize(pExpr->pLeft). But nVector was initialized with
exactly the same value. So strlen(zAff) should be equal to nVector, and
your function sql_affinity_str_to_field_type_str() should return already
correct array. I tried to test it and faced into a very strange situation.

Firstly, it appeared that already at the beginning of the
function, after this code:

     zAff = exprINAffinity(pParse, pExpr);
     nVector = sqlite3ExprVectorSize(pExpr->pLeft);

They are not already equal. This assertion fails:

     strlen(zAff) == nVector.

I tried to print their values in debug, and found, that
zAff is stored in a buffer of length 2, but both bytes are
zeros. Why? It is a first strange situation.

Secondly, your function sql_affinity_str_to_field_type_str()
in such a case returns a buffer of one byte - [field_type_MAX],
because it uses strlen(affinity_str) + 1, which == 1 here.

Further, you do types[nVector] = field_type_MAX, so you are
out of array bounds, because nVector == 1, not 0. The only
reason why it does not fail is that malloc returns more
memory than necessary.

Please, check out if I am right, and fix the bug if so.

The assertion fails in sql-tap/randexpr1.test.lua.

Also, see 2 comments below.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 58e35e0d9..d263c1bb5 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -2854,8 +2854,17 @@ sqlite3CodeSubselect(Parse * pParse,	/* Parsing context */
>  						jmpIfDynamic = -1;
>  					}
>  					r3 = sqlite3ExprCodeTarget(pParse, pE2, r1);
> +					enum field_type type =
> +						sql_affinity_to_field_type(affinity);
> +					size_t sz = 2 * sizeof(enum field_type);
> +					enum field_type *types =
> +						sqlite3DbMallocRaw(pParse->db,
> +								   sz);
> +					types[0] = type;
> +					types[1] = field_type_MAX;

1. Please, either check for malloc result, or just use the
same method as in other places - declare it on the stack and pass
sizeof(this_array) instead of P4_DYNAMIC.

>  	 				sqlite3VdbeAddOp4(v, OP_MakeRecord, r3,
> -							  1, r2, &affinity, 1);
> +							  1, r2, (char *)types,
> +							  P4_DYNAMIC);
>  					sqlite3ExprCacheAffinityChange(pParse,
>  								       r3, 1);
>  					sqlite3VdbeAddOp2(v, OP_IdxInsert, r2,
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 24d992284..80d2fd0aa 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2881,7 +2873,7 @@ case OP_MakeRecord: {
>  	 * of the record to data0.
>  	 */
>  	nField = pOp->p1;
> -	zAffinity = pOp->p4.z;
> +	enum field_type *types = (enum field_type *)pOp->p4.z;

2. Maybe, it is worth adding enum field_type *types into VdbeOp.p4union
and do not cast. Like we did with many other pointers.

>  	bIsEphemeral = pOp->p5;
>  	assert(nField>0 && pOp->p2>0 && pOp->p2+nField<=(p->nMem+1 - p->nCursor)+1);
>  	pData0 = &aMem[nField];