From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 0E47D2CE12 for ; Thu, 11 Oct 2018 10:56:21 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id iq8PIc7VHXFH for ; Thu, 11 Oct 2018 10:56:20 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 6FB542CDF8 for ; Thu, 11 Oct 2018 10:56:20 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/1] sql: hold in stat tables space/index id instead of name References: <1f1960f842e9443511b4bf2712a0b79bd7fb0764.1535711802.git.imeevma@gmail.com> <0bbf0c18-9d9b-4849-5a01-24729ab08468@tarantool.org> <70A5C3C7-8069-4FF0-A653-46B3B82C9716@tarantool.org> <1538498570.8155792@f528.i.mail.ru> <16CEE3B0-B6FC-4BE0-B381-704AF1794585@tarantool.org> From: Imeev Mergen Message-ID: Date: Thu, 11 Oct 2018 17:56:17 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------7865B2A56B442BC514648CA3" Content-Language: en-US Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" , tarantool-patches@freelists.org, Vladislav Shpilevoy This is a multi-part message in MIME format. --------------7865B2A56B442BC514648CA3 Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit 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. --------------7865B2A56B442BC514648CA3 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit

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.

--------------7865B2A56B442BC514648CA3--