[tarantool-patches] Re: [PATCH v2 1/1] sql: hold in stat tables space/index id instead of name

[Imeev Mergen <imeevma@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 2283 bytes --]

Hello! Thank you for review! My answer below.


On 10/11/2018 02:41 PM, n.pettik wrote:
>
>>> Please, next time attach intermediate diff (i.e. diff between two versions) or
>>> inline each hunk as answer to comment. It is quite complicated to review
>>> full patch (especially when it comes to patches containing hundreds of lines)
>>> each time.
>>>
>>> https://travis-ci.org/tarantool/tarantool/jobs/436232731
>>>
>>> box/sql.test.lua fails on Travis. Please, fix it.
>>>
>> Now it is good on travis:
>> https://travis-ci.org/tarantool/tarantool/builds/439704198*
>>
>> Diff between two last versions:*
>>
>> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
>> index 95c516a..a886d8a 100644
>> --- a/src/box/sql/analyze.c
>> +++ b/src/box/sql/analyze.c
>> @@ -797,13 +797,13 @@ vdbe_emit_analyze_space(struct Parse *parse, 
>> struct space *space)
>>      assert(space->index_count != 0);
>>      struct Vdbe *v = sqlite3GetVdbe(parse);
>>      assert(v != NULL);
>> -    const char *tab_name = space_name(space);
>> +    MAYBE_UNUSED const char *tab_name = space_name(space);
>>      sqlite3VdbeAddOp4(v, OP_IteratorOpen, tab_cursor, 0, 0, (void *) 
>> space,
>>                P4_SPACEPTR);
>>      sqlite3VdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
>>      for (uint32_t j = 0; j < space->index_count; ++j) {
>>          struct index *idx = space->index[j];
>> -        const char *idx_name;
>> +        MAYBE_UNUSED const char *idx_name;
>>          /*
>>           * Primary indexes feature automatically generated
>>           * names. Thus, for the sake of clarity, use
>
> Actually, this diff doesn’t look like fix of that failed test.
> I guess it is simply flaky, so this time you get lucky and it is passed.
> Did you checked that test-trace from Travis fails on fresh 2.0 as well?
> Did you manage to understand the reason of failure? Otherwise there is
> no guarantee that you patch is innocent in this situation.
> Without any investigation I can give my approval on this patch.
>
I was able to reproduce this failure on current 2.0:
https://github.com/tarantool/tarantool/issues/3737

I think my patch do not affect this test in the way it showed in
error.


[-- Attachment #2: Type: text/html, Size: 4504 bytes --]