From: Kirill Yukhin <kyukhin@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH] Set format for spaces with sysview engine
Date: Wed, 17 Apr 2019 11:13:20 +0300 [thread overview]
Message-ID: <20190417081320.xwcf3ogkvrspgdu3@tarantool.org> (raw)
In-Reply-To: <265af30c-727f-e781-e2c3-4bc13c98eeae@tarantool.org>
Hello,
Thanks for review! I'll send updated patch as v2.
My answers are inlined.
On 16 апр 18:13, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch! See 17 comments below.
>
> 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.
> On 15/04/2019 16:35, Kirill Yukhin wrote:
> > The patch sets format for spaces with sysview engine.
> > This is done due to following reasons:
> > 1. To allow to create an SQL view, which looks at format of
> > space being used.
>
> 2. But it does exactly this even before your patch. Elaborate
> please, that before your patch not all spaces had formats - the
> ones of sysview engine. This is because sysview was invented as
> a filter-engine for normal engines and nothing more. Therefore it
> hasn't its own tuples and didn't need a format.
>
> But SQL may want to work with sysview spaces, and it expects that
> every space has struct tuple_format, no exceptions. So as not to
> patch SQL with shit like
>
> if engine == sysview then do not use tuple_format
>
> we've decided to create bogus formats even for sysview spaces.
I've updated commment.
> > 2. To unify sysview's engine spaces with SQL views. This
> > will allow to use sysview machinery to query SQL views from
> > Lua land.
>
> 3. This will *completely replace* SQL views. The latter will become
> just another interface to sysview.
Updated comment.
> >
> > Closes #4111
> > ---
>
> 4. Please, specify branch and issue links here.
Done.
> > diff --git a/src/box/index_def.c b/src/box/index_def.c
> > index c743d12..31dac33 100644
> > --- a/src/box/index_def.c
> > +++ b/src/box/index_def.c
> > @@ -245,6 +246,25 @@ index_def_cmp(const struct index_def *key1, const struct index_def *key2)
> > key2->key_def->parts, key2->key_def->part_count);
> > }
> >
> > +struct key_def **
> > +key_def_array(struct rlist *index_defs, int *size)
> > +{
> > + /* Create a format from key and field definitions. */
>
> 5. You do not create a format here. Just a key_def array.
> The comment can be dropped.
Done.
> > + int key_count = 0;
> > + struct index_def *index_def;
> > + rlist_foreach_entry(index_def, index_defs, link)
> > + key_count++;
> > + struct key_def **keys = (struct key_def **) region_alloc(&fiber()->gc,
> > + sizeof(*keys) * key_count);
>
> 6. Please, align the code according to the code style. We violate
> the alignment only in rare cases when indentation is huge (usually it
> is about internal still raw SQLite functions).
Done.
> > + if (keys == NULL)
> > + return NULL;
>
> 7. Please, set diag. Region_alloc does not set it itself.
Done.
> > 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.
>
> 8. Please, add a comment that the array is created on region.
Done.
> 9. It is not an array of index definitions. It is a list. The
> same below about @param index_defs.
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.
> > diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
> > index 7bb46c7..89b04c1 100644
> > --- a/src/box/memtx_space.c
> > +++ b/src/box/memtx_space.c
> > @@ -975,23 +975,16 @@ memtx_space_new(struct memtx_engine *memtx,
> > }
> >
> > /* Create a format from key and field definitions. */
> > +
>
> 11. Stray empty line.
Removed.
> > struct tuple_format *format =
> > - tuple_format_new(&memtx_tuple_format_vtab, memtx, keys, key_count,
> > - def->fields, def->field_count,
> > + tuple_format_new(&memtx_tuple_format_vtab, memtx, keys,
> > + key_count, def->fields, def->field_count,
>
> 12. This diff changes nothing, even names are the same. Please, drop.
Reverted.
> > diff --git a/src/box/sysview.c b/src/box/sysview.c
> > index f9edd2d..768cd02 100644
> > --- a/src/box/sysview.c
> > +++ b/src/box/sysview.c
> > @@ -517,8 +517,31 @@ sysview_engine_create_space(struct engine *engine, struct space_def *def,
> > "malloc", "struct space");
> > return NULL;
> > }
> > + int key_count = 0;
> > + /*
> > + * Despite of the fact that space with sysview engine
>
> 13. In English 'despite' is never followed by 'of'. You either write
> 'despite', or 'in spite of', no exceptions.
Fixed.
> > 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.
> > diff --git a/test/sql/gh-4111-format-in-sysview.result b/test/sql/gh-4111-format-in-sysview.result
> > new file mode 100644
> > index 0000000..1d6c1f0
> > --- /dev/null
> > +++ b/test/sql/gh-4111-format-in-sysview.result
>
> 15. Please, move it to 'access_sysview' test. Because
> basically it is exactly about this: accessing a sysview
> from SQL.
No, this is a regression test for the bug #4111. I can move it to another suite if you like.
> > @@ -0,0 +1,51 @@
> > +-- Make sure, that it is possible to create a VIEW which
> > +-- refers to "_v" space, i.e. to sysview engine.
> > +-- Boefore gh-4111 was fixed, attempt to create such a view
>
> 16. Boefore -> before.
Fixed.
> > +-- failed due to lack of format in a space with sysview
> > +-- engine.
> > +test_run = require('test_run').new()
> > +---
> > +...
> > +box.space._vspace:format()
> > +---
> > +- [{'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'}]
> > +...
> > +box.execute([[CREATE VIEW t AS SELECT "name" FROM "_vspace" y]])
> > +---
> > +- row_count: 1
> > +...
> > +box.execute([[SELECT * from t]])
> > +---
> > +- metadata:
> > + - name: name
> > + type: string
> > + rows:
> > + - ['_vinyl_deferred_delete']
> > + - ['_schema']
> > + - ['_collation']
> > + - ['_space']
> > + - ['_vspace']
> > + - ['_sequence']
> > + - ['_sequence_data']
> > + - ['_vsequence']
> > + - ['_index']
> > + - ['_vindex']
> > + - ['_func']
> > + - ['_vfunc']
> > + - ['_user']
> > + - ['_vuser']
> > + - ['_priv']
> > + - ['_vpriv']
> > + - ['_cluster']
> > + - ['_trigger']
> > + - ['_truncate']
> > + - ['_space_sequence']
> > + - ['_fk_constraint']
> > + - ['T']
>
> 17. The same objection as 14. Probably it would be less
> flaky to call
>
> box.execute([[SELECT * from t WHERE "name" = "T"]])
>
> Then it will print one line only and still test the issue
> good.
Done.
--
Regards, Kirill Yukhin
next prev parent reply other threads:[~2019-04-17 8:13 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 [this message]
2019-04-17 10:11 ` Vladislav Shpilevoy
2019-04-18 8:16 ` Kirill Yukhin
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=20190417081320.xwcf3ogkvrspgdu3@tarantool.org \
--to=kyukhin@tarantool.org \
--cc=tarantool-patches@freelists.org \
--cc=v.shpilevoy@tarantool.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