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 3C8B82BBEF for ; Tue, 16 Apr 2019 11:13:05 -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 iNEOgE-q3gsF for ; Tue, 16 Apr 2019 11:13:05 -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 5FA032BA23 for ; Tue, 16 Apr 2019 11:13:04 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] Set format for spaces with sysview engine References: From: Vladislav Shpilevoy Message-ID: <265af30c-727f-e781-e2c3-4bc13c98eeae@tarantool.org> Date: Tue, 16 Apr 2019 18:13:00 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Kirill Yukhin Cc: tarantool-patches@freelists.org 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. 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. > 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. > > Closes #4111 > --- 4. Please, specify branch and issue links here. > src/box/index_def.c | 20 ++++++ > src/box/index_def.h | 11 ++++ > src/box/memtx_space.c | 15 ++--- > src/box/sysview.c | 25 +++++++- > src/box/tuple_format.c | 5 +- > test/box/access_sysview.result | 68 ++++++++++++++++++++- > test/sql/gh-4111-format-in-sysview.result | 51 ++++++++++++++++ > test/sql/gh-4111-format-in-sysview.test.lua | 13 ++++ > test/wal_off/alter.result | 2 +- > 9 files changed, 195 insertions(+), 15 deletions(-) > create mode 100644 test/sql/gh-4111-format-in-sysview.result > create mode 100644 test/sql/gh-4111-format-in-sysview.test.lua > > 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. > + 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). > + if (keys == NULL) > + return NULL; 7. Please, set diag. Region_alloc does not set it itself. > + *size = key_count; > + key_count = 0; > + rlist_foreach_entry(index_def, index_defs, link) > + keys[key_count++] = index_def->key_def; > + return keys; > +} > + > bool > index_def_is_valid(struct index_def *index_def, const char *space_name) > > 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. 9. It is not an array of index definitions. It is a list. The same below about @param index_defs. > + * > + * @param index_defs Array pointer. > + * @param size Array size. 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. A comment could be like: @param[out] size A location to store size of the result key_defs array. > + * @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); > + > /** > * One key definition is greater than the other if it's id is > * greater, it's name is greater, it's index type is greater > 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. > 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. > def->exact_field_count, def->dict, > def->opts.is_temporary, def->opts.is_ephemeral); > if (format == NULL) { > 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. > + * actually doesn't own tuples, setup of format will be > + * useful in order to unify it with SQL views and to use > + * same machinery to do selects from such views from Lua > + * land. > + */ > + struct key_def **keys = key_def_array(key_list, &key_count); > + if (keys == NULL) { > + free(space); > + return NULL; > + } > + struct tuple_format *format = > + tuple_format_new(NULL, NULL, keys, key_count, def->fields, > + def->field_count, def->exact_field_count, > + def->dict, def->opts.is_temporary, > + def->opts.is_ephemeral); > + if (format == NULL) { > + free(space); > + return NULL; > + } > + tuple_format_ref(format); > if (space_create(space, engine, &sysview_space_vtab, > - def, key_list, NULL) != 0) { > + def, key_list, format) != 0) { > free(space); > return NULL; > } > 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 > ... > box.space._vspace.index[1]:alter({parts = { 2, 'unsigned' }}) > --- > 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. > @@ -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. > +-- 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. > +... > +box.execute([[DROP VIEW t]]) > +--- > +- row_count: 1 > +...