[tarantool-patches] Re: [PATCH 5/8] sql: replace field type with affinity for VDBE runtime
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Jan 30 16:04:33 MSK 2019
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];
More information about the Tarantool-patches
mailing list