From: Kirill Yukhin <kyukhin@tarantool.org>
To: tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH] Set format for spaces with sysview engine
Date: Thu, 18 Apr 2019 11:16:30 +0300 [thread overview]
Message-ID: <20190418081630.yfe5hi7w2m3bu5lh@tarantool.org> (raw)
In-Reply-To: <68ec9d10-4fa6-4997-54bd-4a80b1ca79ce@tarantool.org>
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.
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...
> See 3 comments below.
>
> >>> diff --git a/src/box/index_def.h b/src/box/index_def.h
> >>> index 7717ecd..628342d 100644
> >>> --- a/src/box/index_def.h
> >>> +++ b/src/box/index_def.h
> >>> @@ -336,6 +336,17 @@ index_def_new(uint32_t space_id, uint32_t iid, const char *name,
> >>> const struct index_opts *opts,
> >>> struct key_def *key_def, struct key_def *pk_def);
> >>>
> >>> +/**
> >>> + * Create an array of key_defs from array of index definitions.
> >> 9. It is not an array of index definitions. It is a list. The
> >> same below about @param index_defs.
> >
> > Done.
>
> 1. No, it isn't. You still say "from array of index definitions",
> but it is not array. Please, fix.
Done.
> >> 10. Please, do not write comments just for the sake of them.
> >> Here 'size' is an [out] variable. From your comment it looks
> >> like it is a ready-at-hand size of index_defs list.
> >
> > Fixed.
>
> 2. [out] should be specified after @param, not after parameter
> name (your current version is @param size[out]). Source:
> http://www.doxygen.nl/manual/commands.html#cmdparam
Moved.
> >>> 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)
> >>> ---
> >>> -- error: 'Supplied key type of part 0 does not match index part type: expected string'
> >>> +- - [257, 1, '_vinyl_deferred_delete', 'blackhole', 0, {'group_id': 1}, [{'name': 'space_id',
> >>> + 'type': 'unsigned'}, {'name': 'lsn', 'type': 'unsigned'}, {'name': 'tuple',
> >>> + 'type': 'array'}]]
> >>> + - [272, 1, '_schema', 'memtx', 0, {}, [{'type': 'string', 'name': 'key'}, {'type': 'any',
> >>> + 'name': 'value', 'is_nullable': true}]]
> >>> + - [276, 1, '_collation', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {
> >>> + 'name': 'name', 'type': 'string'}, {'name': 'owner', 'type': 'unsigned'},
> >>> + {'name': 'type', 'type': 'string'}, {'name': 'locale', 'type': 'string'}, {
> >>> + 'name': 'opts', 'type': 'map'}]]
> >>> + - [280, 1, '_space', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
> >>> + 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'engine',
> >>> + 'type': 'string'}, {'name': 'field_count', 'type': 'unsigned'}, {'name': 'flags',
> >>> + 'type': 'map'}, {'name': 'format', 'type': 'array'}]]
> >>> + - [281, 1, '_vspace', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
> >>> + 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'engine',
> >>> + 'type': 'string'}, {'name': 'field_count', 'type': 'unsigned'}, {'name': 'flags',
> >>> + 'type': 'map'}, {'name': 'format', 'type': 'array'}]]
> >>> + - [284, 1, '_sequence', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
> >>> + 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'step',
> >>> + 'type': 'integer'}, {'name': 'min', 'type': 'integer'}, {'name': 'max', 'type': 'integer'},
> >>> + {'name': 'start', 'type': 'integer'}, {'name': 'cache', 'type': 'integer'},
> >>> + {'name': 'cycle', 'type': 'boolean'}]]
> >>> + - [285, 1, '_sequence_data', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'},
> >>> + {'name': 'value', 'type': 'integer'}]]
> >>> + - [286, 1, '_vsequence', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'},
> >>> + {'name': 'owner', 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {
> >>> + 'name': 'step', 'type': 'integer'}, {'name': 'min', 'type': 'integer'}, {
> >>> + 'name': 'max', 'type': 'integer'}, {'name': 'start', 'type': 'integer'}, {
> >>> + 'name': 'cache', 'type': 'integer'}, {'name': 'cycle', 'type': 'boolean'}]]
> >>> + - [288, 1, '_index', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'iid',
> >>> + 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'type',
> >>> + 'type': 'string'}, {'name': 'opts', 'type': 'map'}, {'name': 'parts', 'type': 'array'}]]
> >>> + - [289, 1, '_vindex', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'iid',
> >>> + 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'type',
> >>> + 'type': 'string'}, {'name': 'opts', 'type': 'map'}, {'name': 'parts', 'type': 'array'}]]
> >>> + - [296, 1, '_func', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
> >>> + 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'setuid',
> >>> + 'type': 'unsigned'}]]
> >>> + - [297, 1, '_vfunc', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
> >>> + 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'setuid',
> >>> + 'type': 'unsigned'}]]
> >>> + - [304, 1, '_user', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
> >>> + 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'type',
> >>> + 'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
> >>> + - [305, 1, '_vuser', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
> >>> + 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'type',
> >>> + 'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
> >>> + - [312, 1, '_priv', 'memtx', 0, {}, [{'name': 'grantor', 'type': 'unsigned'}, {
> >>> + 'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
> >>> + {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
> >>> + - [313, 1, '_vpriv', 'sysview', 0, {}, [{'name': 'grantor', 'type': 'unsigned'},
> >>> + {'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
> >>> + {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
> >>> + - [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
> >>> + 'type': 'string'}]]
> >>> + - [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'space_id',
> >>> + 'type': 'unsigned'}, {'name': 'opts', 'type': 'map'}]]
> >>> + - [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
> >>> + 'type': 'unsigned'}]]
> >>> + - [340, 1, '_space_sequence', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'},
> >>> + {'name': 'sequence_id', 'type': 'unsigned'}, {'name': 'is_generated', 'type': 'boolean'}]]
> >>> + - [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. Done here as well.
diff --git a/src/box/index_def.c b/src/box/index_def.c
index f509443..476d9f8 100644
--- a/src/box/index_def.c
+++ b/src/box/index_def.c
@@ -247,7 +247,7 @@ index_def_cmp(const struct index_def *key1, const struct index_def *key2)
}
struct key_def **
-key_def_array(struct rlist *index_defs, int *size)
+index_def_to_key_def(struct rlist *index_defs, int *size)
{
int key_count = 0;
struct index_def *index_def;
diff --git a/src/box/index_def.h b/src/box/index_def.h
index 0eb9b45..6dac283 100644
--- a/src/box/index_def.h
+++ b/src/box/index_def.h
@@ -337,16 +337,16 @@ index_def_new(uint32_t space_id, uint32_t iid, const char *name,
struct key_def *key_def, struct key_def *pk_def);
/**
- * Create an array (on a region) of key_defs from array of index
+ * Create an array (on a region) of key_defs from list of index
* definitions.
*
* @param index_defs List head.
- * @param size[out] Array size.
+ * @param[out] size Array size.
* @retval not NULL Array of pointers to key_def
* @retval NULL Memory error.
*/
struct key_def **
-key_def_array(struct rlist *index_defs, int *size);
+index_def_to_key_def(struct rlist *index_defs, int *size);
/**
* One key definition is greater than the other if it's id is
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index fa02806..a28204d 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -976,7 +976,7 @@ memtx_space_new(struct memtx_engine *memtx,
/* Create a format from key and field definitions. */
int key_count = 0;
- struct key_def **keys = key_def_array(key_list, &key_count);
+ struct key_def **keys = index_def_to_key_def(key_list, &key_count);
if (keys == NULL) {
free(memtx_space);
return NULL;
diff --git a/src/box/sysview.c b/src/box/sysview.c
index d65137d..0b07c9f 100644
--- a/src/box/sysview.c
+++ b/src/box/sysview.c
@@ -525,7 +525,7 @@ sysview_engine_create_space(struct engine *engine, struct space_def *def,
* same machinery to do selects from such views from Lua
* land.
*/
- struct key_def **keys = key_def_array(key_list, &key_count);
+ struct key_def **keys = index_def_to_key_def(key_list, &key_count);
if (keys == NULL) {
free(space);
return NULL;
diff --git a/test/box/access_sysview.result b/test/box/access_sysview.result
index 6b51566..c6a2b22 100644
--- a/test/box/access_sysview.result
+++ b/test/box/access_sysview.result
@@ -641,74 +641,9 @@ box.space._vspace.index[1]:select('xxx')
---
- error: 'Supplied key type of part 0 does not match index part type: expected unsigned'
...
-box.space._vspace.index[1]:select(1)
+box.space._vspace.index[1]:count(1) > 0
---
-- - [257, 1, '_vinyl_deferred_delete', 'blackhole', 0, {'group_id': 1}, [{'name': 'space_id',
- 'type': 'unsigned'}, {'name': 'lsn', 'type': 'unsigned'}, {'name': 'tuple',
- 'type': 'array'}]]
- - [272, 1, '_schema', 'memtx', 0, {}, [{'type': 'string', 'name': 'key'}, {'type': 'any',
- 'name': 'value', 'is_nullable': true}]]
- - [276, 1, '_collation', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {
- 'name': 'name', 'type': 'string'}, {'name': 'owner', 'type': 'unsigned'},
- {'name': 'type', 'type': 'string'}, {'name': 'locale', 'type': 'string'}, {
- 'name': 'opts', 'type': 'map'}]]
- - [280, 1, '_space', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
- 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'engine',
- 'type': 'string'}, {'name': 'field_count', 'type': 'unsigned'}, {'name': 'flags',
- 'type': 'map'}, {'name': 'format', 'type': 'array'}]]
- - [281, 1, '_vspace', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
- 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'engine',
- 'type': 'string'}, {'name': 'field_count', 'type': 'unsigned'}, {'name': 'flags',
- 'type': 'map'}, {'name': 'format', 'type': 'array'}]]
- - [284, 1, '_sequence', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
- 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'step',
- 'type': 'integer'}, {'name': 'min', 'type': 'integer'}, {'name': 'max', 'type': 'integer'},
- {'name': 'start', 'type': 'integer'}, {'name': 'cache', 'type': 'integer'},
- {'name': 'cycle', 'type': 'boolean'}]]
- - [285, 1, '_sequence_data', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'},
- {'name': 'value', 'type': 'integer'}]]
- - [286, 1, '_vsequence', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'},
- {'name': 'owner', 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {
- 'name': 'step', 'type': 'integer'}, {'name': 'min', 'type': 'integer'}, {
- 'name': 'max', 'type': 'integer'}, {'name': 'start', 'type': 'integer'}, {
- 'name': 'cache', 'type': 'integer'}, {'name': 'cycle', 'type': 'boolean'}]]
- - [288, 1, '_index', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'iid',
- 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'type',
- 'type': 'string'}, {'name': 'opts', 'type': 'map'}, {'name': 'parts', 'type': 'array'}]]
- - [289, 1, '_vindex', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'iid',
- 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'type',
- 'type': 'string'}, {'name': 'opts', 'type': 'map'}, {'name': 'parts', 'type': 'array'}]]
- - [296, 1, '_func', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
- 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'setuid',
- 'type': 'unsigned'}]]
- - [297, 1, '_vfunc', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
- 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'setuid',
- 'type': 'unsigned'}]]
- - [304, 1, '_user', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
- 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'type',
- 'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
- - [305, 1, '_vuser', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
- 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'type',
- 'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
- - [312, 1, '_priv', 'memtx', 0, {}, [{'name': 'grantor', 'type': 'unsigned'}, {
- 'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
- {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
- - [313, 1, '_vpriv', 'sysview', 0, {}, [{'name': 'grantor', 'type': 'unsigned'},
- {'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
- {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
- - [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
- 'type': 'string'}]]
- - [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'space_id',
- 'type': 'unsigned'}, {'name': 'opts', 'type': 'map'}]]
- - [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
- 'type': 'unsigned'}]]
- - [340, 1, '_space_sequence', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'},
- {'name': 'sequence_id', 'type': 'unsigned'}, {'name': 'is_generated', 'type': 'boolean'}]]
- - [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'}]]
+- true
...
box.space._vspace.index[1]:alter({parts = { 2, 'unsigned' }})
---
diff --git a/test/box/access_sysview.test.lua b/test/box/access_sysview.test.lua
index 4cc5611..c624584 100644
--- a/test/box/access_sysview.test.lua
+++ b/test/box/access_sysview.test.lua
@@ -270,7 +270,7 @@ seq:drop()
box.space._vspace.index[1]:alter({parts = { 2, 'string' }})
box.space._vspace.index[1]:select('xxx')
-box.space._vspace.index[1]:select(1)
+box.space._vspace.index[1]:count(1) > 0
box.space._vspace.index[1]:alter({parts = { 2, 'unsigned' }})
box.space._space.index[1]:drop()
box.space._vspace.index[1]:select(1)
next prev parent reply other threads:[~2019-04-18 8:16 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-15 13:35 [tarantool-patches] " Kirill Yukhin
2019-04-16 15:13 ` [tarantool-patches] " Vladislav Shpilevoy
2019-04-17 8:13 ` Kirill Yukhin
2019-04-17 10:11 ` Vladislav Shpilevoy
2019-04-18 8:16 ` Kirill Yukhin [this message]
2019-04-18 10:43 ` Vladislav Shpilevoy
2019-04-18 11:14 ` Kirill Yukhin
2019-04-18 11:39 ` Vladislav Shpilevoy
2019-04-18 12:08 ` Kirill Yukhin
2019-04-18 12:43 ` Vladislav Shpilevoy
2019-04-18 13:25 ` Kirill Yukhin
2019-04-18 14:18 ` Konstantin Osipov
2019-04-19 11:46 ` Kirill Yukhin
2019-04-20 22:36 ` Alexander Turenko
2019-04-21 17:06 ` Vladislav Shpilevoy
2019-04-21 18:19 ` Konstantin Osipov
2019-04-18 14:16 ` Konstantin Osipov
2019-04-17 13:38 ` Konstantin Osipov
2019-04-18 8:23 ` Kirill Yukhin
2019-04-19 11:38 ` Kirill Yukhin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190418081630.yfe5hi7w2m3bu5lh@tarantool.org \
--to=kyukhin@tarantool.org \
--cc=tarantool-patches@freelists.org \
--subject='[tarantool-patches] Re: [PATCH] Set format for spaces with sysview engine' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox