[tarantool-patches] Re: [PATCH] Set format for spaces with sysview engine

[Vladislav Shpilevoy]

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.