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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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
> +...