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 8E4892C296 for ; Thu, 18 Apr 2019 06:43:38 -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 tInErmTSzDZR for ; Thu, 18 Apr 2019 06:43:38 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 C95A01F93B for ; Thu, 18 Apr 2019 06:43:37 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] Set format for spaces with sysview engine References: <265af30c-727f-e781-e2c3-4bc13c98eeae@tarantool.org> <20190417081320.xwcf3ogkvrspgdu3@tarantool.org> <68ec9d10-4fa6-4997-54bd-4a80b1ca79ce@tarantool.org> <20190418081630.yfe5hi7w2m3bu5lh@tarantool.org> From: Vladislav Shpilevoy Message-ID: <77d5c2c1-8a26-990c-0fc0-dc40d56f64ae@tarantool.org> Date: Thu, 18 Apr 2019 13:43:35 +0300 MIME-Version: 1.0 In-Reply-To: <20190418081630.yfe5hi7w2m3bu5lh@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: tarantool-patches@freelists.org, Kirill Yukhin Hi! Thanks for the fixes! See 4 comments below. 1. Now I see that sysview tuple format leaks. Please, apply this diff: ====================================================== diff --git a/src/box/sysview.c b/src/box/sysview.c index 0b07c9f4a..96c5e78ca 100644 --- a/src/box/sysview.c +++ b/src/box/sysview.c @@ -545,6 +545,8 @@ sysview_engine_create_space(struct engine *engine, struct space_def *def, free(space); return NULL; } + /* Format is now referenced by the space. */ + tuple_format_unref(format); return space; } ====================================================== On 18/04/2019 11:16, Kirill Yukhin wrote: > Hello, > > Thanks for review. My answers are inlinded. Iterative patch in the bottom. > Branch force pushed and rebased on recent master. I've also renamed function > as per kostja's request in neighbour mail. 2. I do not agree with the new name, because we do not convert one index_def to one key_def. We just reorganize a list of the formers into an array of the latters not touching the objects. But as you wish. > > On 17 апр 13:11, Vladislav Shpilevoy wrote: >> Hi! Thanks for the fixes! >> >>>> 1. Please, prepend a subsystem name to the commit title. >>>> Here I would use 'sysview' (by analogue with 'memtx', 'vinyl'). >>>> Do not forget to omit capital letter in 'Set' when the prefix >>>> is added. >>> >>> Didn't know we have spacial prefix for sysview. Done. >> >> As I know, we do not have a strict list of subsystems at all. >> Or it is hidden somewhere in SOP, which would not make any sense, >> because new ones appear too often. > > I think I saw such a list in web docs... 3. If you mean this: https://www.tarantool.io/ru/doc/2.1/dev_guide/developer_guidelines/#how-to-write-a-commit-message then obviously it is not full. We also use 'test', 'fiber', 'tarantoolctl', 'box', 'lib', 'space', 'swim' etc. It means, that the list in the docs is just a few examples, not a strict list. >>>>> diff --git a/test/box/access_sysview.result b/test/box/access_sysview.result >>>>> index ae04266..6b51566 100644 >>>>> --- a/test/box/access_sysview.result >>>>> +++ b/test/box/access_sysview.result >>>>> @@ -642,7 +643,72 @@ box.space._vspace.index[1]:select('xxx') >>>>> ... >>>>> box.space._vspace.index[1]:select(1) >>>>> + - [356, 1, '_fk_constraint', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, >>>>> + {'name': 'child_id', 'type': 'unsigned'}, {'name': 'parent_id', 'type': 'unsigned'}, >>>>> + {'name': 'is_deferred', 'type': 'boolean'}, {'name': 'match', 'type': 'string'}, >>>>> + {'name': 'on_delete', 'type': 'string'}, {'name': 'on_update', 'type': 'string'}, >>>>> + {'name': 'child_cols', 'type': 'array'}, {'name': 'parent_cols', 'type': 'array'}]] >>>> >>>> 14. Too huge output, which will change on each update in system >>>> spaces set and format. We already have similar tests printing >>>> everything out, and usually it is not a pleasant business to fix >>>> them after each touch of upgrade.lua. I would rather replaced it >>>> like this: >>>> >>>> box.space._vspace.index[1]:count(1) > 0 >>> >>> Done. >> >> 3. No, it is not. This hunk is still literally absolutely the same. >> Please, do it. > > I did it in reg test. 4. Sorry, but I gave you both comments in the first review and you said 'Done' not actually having one of them done. > Done here as well. Thanks, now it is really fixed.