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

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

[Kirill Yukhin]

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. To unify sysview's engine spaces with SQL views. This
     will allow to use sysview machinery to query SQL views from
     Lua land.

Closes #4111
---
 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
@@ -33,6 +33,7 @@
 #include "identifier.h"
 #include "tuple_format.h"
 #include "json/json.h"
+#include "fiber.h"
 
 const char *index_type_strs[] = { "HASH", "TREE", "BITSET", "RTREE" };
 
@@ -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. */
+	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);
+	if (keys == NULL)
+		return NULL;
+	*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.
+ *
+ * @param index_defs Array pointer.
+ * @param 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);
+
 /**
  * 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. */
+
 	int key_count = 0;
-	struct index_def *index_def;
-	rlist_foreach_entry(index_def, key_list, link)
-		key_count++;
-	struct key_def **keys = region_alloc(&fiber()->gc,
-					     sizeof(*keys) * key_count);
+	struct key_def **keys = key_def_array(key_list, &key_count);
 	if (keys == NULL) {
 		free(memtx_space);
 		return NULL;
 	}
-	key_count = 0;
-	rlist_foreach_entry(index_def, key_list, link)
-		keys[key_count++] = index_def->key_def;
-
 	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,
 				 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
+	 * 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/src/box/tuple_format.c b/src/box/tuple_format.c
index 093046b..804a678 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -699,7 +699,10 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine,
 		tuple_format_alloc(keys, key_count, space_field_count, dict);
 	if (format == NULL)
 		return NULL;
-	format->vtab = *vtab;
+	if (vtab != NULL)
+		format->vtab = *vtab;
+	else
+		memset(&format->vtab, 0, sizeof(format->vtab));
 	format->engine = engine;
 	format->is_temporary = is_temporary;
 	format->is_ephemeral = is_ephemeral;
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
@@ -635,6 +635,7 @@ seq:drop()
 --
 box.space._vspace.index[1]:alter({parts = { 2, 'string' }})
 ---
+- error: Field 2 has type 'unsigned' in space format, but type 'string' in index definition
 ...
 box.space._vspace.index[1]:select('xxx')
 ---
@@ -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'}]]
 ...
 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
@@ -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
+-- 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']
+...
+box.execute([[DROP VIEW t]])
+---
+- row_count: 1
+...
diff --git a/test/sql/gh-4111-format-in-sysview.test.lua b/test/sql/gh-4111-format-in-sysview.test.lua
new file mode 100644
index 0000000..644738b
--- /dev/null
+++ b/test/sql/gh-4111-format-in-sysview.test.lua
@@ -0,0 +1,13 @@
+-- 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
+-- failed due to lack of format in a space with sysview
+-- engine.
+
+test_run = require('test_run').new()
+
+box.space._vspace:format()
+
+box.execute([[CREATE VIEW t AS SELECT "name" FROM "_vspace" y]])
+box.execute([[SELECT * from t]])
+box.execute([[DROP VIEW t]])
diff --git a/test/wal_off/alter.result b/test/wal_off/alter.result
index becdf13..9257ccf 100644
--- a/test/wal_off/alter.result
+++ b/test/wal_off/alter.result
@@ -28,7 +28,7 @@ end;
 ...
 #spaces;
 ---
-- 65511
+- 65488
 ...
 -- cleanup
 for k, v in pairs(spaces) do
-- 
2.20.1

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

[Vladislav Shpilevoy]

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
> +...

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

[Kirill Yukhin]

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

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

[Vladislav Shpilevoy]

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.

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.

> 
>> 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

>>> 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.

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

[Kirill Yukhin]

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)

[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.

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

[Kirill Yukhin]

Hello,

On 18 апр 13:43, Vladislav Shpilevoy wrote:
> 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;
>  }
> ======================================================

Done.

Branch force-pushed and re-tested.

--
Regards, Kirill Yukhin

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

[Vladislav Shpilevoy]

On 18/04/2019 14:14, Kirill Yukhin wrote:
> Hello,
> 
> On 18 апр 13:43, Vladislav Shpilevoy wrote:
>> 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;
>>  }
>> ======================================================
> 
> Done.
> 
> Branch force-pushed and re-tested.

How was it retested?

[024] Test failed! Result content mismatch:
[024] --- wal_off/alter.result	Thu Apr 18 14:36:14 2019
[024] +++ wal_off/alter.reject	Thu Apr 18 14:37:03 2019
[024] @@ -28,7 +28,7 @@
[024]  ...
[024]  #spaces;
[024]  ---
[024] -- 65488
[024] +- 65505
[024]  ...
[024]  -- cleanup
[024]  for k, v in pairs(spaces) do
[024] 

Looks like some of the previously leaking formats are
back in the pool, good.

> 
> --
> Regards, Kirill Yukhin
> 

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

[Kirill Yukhin]

Hello,

On 18 апр 14:39, Vladislav Shpilevoy wrote:
> 
> 
> On 18/04/2019 14:14, Kirill Yukhin wrote:
> > Hello,
> > 
> > On 18 апр 13:43, Vladislav Shpilevoy wrote:
> >> 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;
> >>  }
> >> ======================================================
> > 
> > Done.
> > 
> > Branch force-pushed and re-tested.
> 
> How was it retested?

I didn't staged updated result. Done.

--
Regards, Kirill Yukhin

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

[Vladislav Shpilevoy]

Now finally technically is ok except that I will never agree
with the test-file-per-issue policy. It is ridiculous.

Following this logic I should name the whole swim subsystem
test file like gh-3234-swim and rename the binary protocol
test file into gh-3234-swim_proto - two files about the same
issue with an ugly prefix looking like it is a fix of a bug. 

Additionally, creation of a new file on each typo-like issue
will lead us sometime into the test-hell like it was in SQLite:

tkt-02a8e81d44.test	tkt-54844eea3f.test	tkt-9a8b09f8e6.test	tkt-d82e3f3721.test	tkt1514.test		tkt2450.test		tkt3121.test		tkt3541.test		tkt3838.test
tkt-26ff0c2d1e.test	tkt-5d863f876e.test	tkt-9d68c883.test	tkt-f3e5abed55.test	tkt1536.test		tkt2565.test		tkt3201.test		tkt3554.test		tkt3841.test
tkt-2a5629202f.test	tkt-5e10420e8d.test	tkt-9f2eb3abac.test	tkt-f67b41381a.test	tkt1537.test		tkt2640.test		tkt3292.test		tkt3581.test		tkt3871.test
tkt-2d1a5c67d.test	tkt-5ee23731f.test	tkt-a7b7803e.test	tkt-f777251dc7a.test	tkt1567.test		tkt2643.test		tkt3298.test		tkt35xx.test		tkt3879.test
tkt-2ea2425d34.test	tkt-6bfb98dfc0.test	tkt-a8a0d2996a.test	tkt-f7b4edec.test	tkt1644.test		tkt2686.test		tkt3334.test		tkt3630.test		tkt3911.test
tkt-31338dca7e.test	tkt-752e1646fc.test	tkt-b1d3a2e531.test	tkt-f973c7ac31.test	tkt1667.test		tkt2767.test		tkt3346.test		tkt3718.test		tkt3918.test
tkt-313723c356.test	tkt-78e04e52ea.test	tkt-b351d95f9.test	tkt-fa7bf5ec.test	tkt1873.test		tkt2817.test		tkt3357.test		tkt3731.test		tkt3922.test
tkt-385a5b56b9.test	tkt-7a31705a7e6.test	tkt-b72787b1.test	tkt-fc62af4523.test	tkt2141.test		tkt2820.test		tkt3419.test		tkt3757.test		tkt3929.test

Cool, isn't it? Very understandable and
E a S y   T o   R e P r O d U c E.

We will end like this. I've finished, LGTM.

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

[Kirill Yukhin]

Hello,

On 18 апр 15:43, Vladislav Shpilevoy wrote:
> Now finally technically is ok except that I will never agree
> with the test-file-per-issue policy. It is ridiculous.
Thanks, I'll check it into master and 2.1 branches.
 
> Following this logic I should name the whole swim subsystem
> test file like gh-3234-swim and rename the binary protocol
> test file into gh-3234-swim_proto - two files about the same
> issue with an ugly prefix looking like it is a fix of a bug.

I hope that will be part of SOP soon.

> Additionally, creation of a new file on each typo-like issue
> will lead us sometime into the test-hell like it was in SQLite:
> 
> tkt-02a8e81d44.test	tkt-54844eea3f.test	tkt-9a8b09f8e6.test	tkt-d82e3f3721.test	tkt1514.test		tkt2450.test		tkt3121.test		tkt3541.test		tkt3838.test
> tkt-26ff0c2d1e.test	tkt-5d863f876e.test	tkt-9d68c883.test	tkt-f3e5abed55.test	tkt1536.test		tkt2565.test		tkt3201.test		tkt3554.test		tkt3841.test
> tkt-2a5629202f.test	tkt-5e10420e8d.test	tkt-9f2eb3abac.test	tkt-f67b41381a.test	tkt1537.test		tkt2640.test		tkt3292.test		tkt3581.test		tkt3871.test
> tkt-2d1a5c67d.test	tkt-5ee23731f.test	tkt-a7b7803e.test	tkt-f777251dc7a.test	tkt1567.test		tkt2643.test		tkt3298.test		tkt35xx.test		tkt3879.test
> tkt-2ea2425d34.test	tkt-6bfb98dfc0.test	tkt-a8a0d2996a.test	tkt-f7b4edec.test	tkt1644.test		tkt2686.test		tkt3334.test		tkt3630.test		tkt3911.test
> tkt-31338dca7e.test	tkt-752e1646fc.test	tkt-b1d3a2e531.test	tkt-f973c7ac31.test	tkt1667.test		tkt2767.test		tkt3346.test		tkt3718.test		tkt3918.test
> tkt-313723c356.test	tkt-78e04e52ea.test	tkt-b351d95f9.test	tkt-fa7bf5ec.test	tkt1873.test		tkt2817.test		tkt3357.test		tkt3731.test		tkt3922.test
> tkt-385a5b56b9.test	tkt-7a31705a7e6.test	tkt-b72787b1.test	tkt-fc62af4523.test	tkt2141.test		tkt2820.test		tkt3419.test		tkt3757.test		tkt3929.test

How ofter do you scan list of regression tests? I bet - never.
But you very often try to extract 2-3 lines of failed case.

In case of aggregated testcases this might take a while.

Also, please keep in mind that number of cores are increasing on
this planet. So, we are trying to get some profit: more files ->
more processes -> more cores utilized.

Finally, filename should be accompanied w/ 2-3 key words, IMHO.
This will significantly increase readability of such strange listings.

--
Regards, Kirill Yukhin

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

[Konstantin Osipov]

* Kirill Yukhin <kyukhin@tarantool.org> [19/04/18 17:12]:
> How ofter do you scan list of regression tests? I bet - never.
> But you very often try to extract 2-3 lines of failed case.

If you work with the test suite as an active maintainer you work
with the files all the time. Otherwise your tests just rot.

> In case of aggregated testcases this might take a while.
> 
> Also, please keep in mind that number of cores are increasing on
> this planet. So, we are trying to get some profit: more files ->
> more processes -> more cores utilized.
> 
> Finally, filename should be accompanied w/ 2-3 key words, IMHO.
> This will significantly increase readability of such strange listings.

Kirill, the decision has been made in 2008 and you don't flip it
just like that. Please don't.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

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

[Kirill Yukhin]

On 18 Apr 17:18, Konstantin Osipov wrote:
> * Kirill Yukhin <kyukhin@tarantool.org> [19/04/18 17:12]:
> > How ofter do you scan list of regression tests? I bet - never.
> > But you very often try to extract 2-3 lines of failed case.
> 
> If you work with the test suite as an active maintainer you work
> with the files all the time. Otherwise your tests just rot.

I see no arguments here, sorry. Of course one work with files all the
time, but what's the point? Please, be more specific: which exact
problems you won't be able to solve w/ such an approach?
Or which of your regular actions will become complicated?

After you collected those problems, you might want to compare 
them w/ clear pros:
  1. more testing parallelism
  2. simple case extraction: one file, one issue, no surfing aroung
     thousands of lines.

--
Regards, Kirill Yukhin

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

[Alexander Turenko]

On Thu, Apr 18, 2019 at 05:18:04PM +0300, Konstantin Osipov wrote:
> * Kirill Yukhin <kyukhin@tarantool.org> [19/04/18 17:12]:
> > How ofter do you scan list of regression tests? I bet - never.
> > But you very often try to extract 2-3 lines of failed case.
> 
> If you work with the test suite as an active maintainer you work
> with the files all the time. Otherwise your tests just rot.

It does not matter much whether test cases spread around its own files
or grouped into categories within one file. The key point here is to
make cases independent. Even when you have independent test cases you
still can extract common preparation code that will be run before each
test case (or before a group of test cases).

Look how test cases are organized in our projects with pytest or with
failsafe maven plugin. They are grouped into files, but I still able to
do something like `pytest test/test_foo.py -k test_bar` or `mvn verify
-Dit.test=FooIT#testBar` and concentrate on one case.

When test cases depend on each other you need to manually find and
extract all needed parts of a test file to run a case. When you do it
several times per day, hey, it becomes annoying.

Now even if developers highly self-disciplined and write independent
test cases, an extra work is still necessary to run a case if a harness
is unable to run just what you need.

And while we are here there is other problem with console-input-like
tests: often you don't sure which statements results are checked
intentionally and which ones are run only for its side effects.
Sometimes you need to git-blame over several attempts to rewrite a test
to understand what the idea was.

Maybe we are near to the point when such simple approach gives more
problems then solves.

Yep, I stated problems here, but don't propose anything. I don't know
where is a right balance between pushing developers to write structured
tests and the freedom developers have now.

I see that some developers like ability to test something with injecting
just one line for one case into another one. I think that it will make
tests more and more embrassing. So, I don't know.

WBR, Alexander Turenko.

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

[Vladislav Shpilevoy]

On 21/04/2019 01:36, Alexander Turenko wrote:
> On Thu, Apr 18, 2019 at 05:18:04PM +0300, Konstantin Osipov wrote:
>> * Kirill Yukhin <kyukhin@tarantool.org> [19/04/18 17:12]:
>>> How ofter do you scan list of regression tests? I bet - never.
>>> But you very often try to extract 2-3 lines of failed case.
>>
>> If you work with the test suite as an active maintainer you work
>> with the files all the time. Otherwise your tests just rot.
> 
> It does not matter much whether test cases spread around its own files
> or grouped into categories within one file. The key point here is to
> make cases independent. Even when you have independent test cases you
> still can extract common preparation code that will be run before each
> test case (or before a group of test cases).
> 
> Look how test cases are organized in our projects with pytest or with
> failsafe maven plugin. They are grouped into files, but I still able to
> do something like `pytest test/test_foo.py -k test_bar` or `mvn verify
> -Dit.test=FooIT#testBar` and concentrate on one case.
> 
> When test cases depend on each other you need to manually find and
> extract all needed parts of a test file to run a case. When you do it
> several times per day, hey, it becomes annoying.
> 
> Now even if developers highly self-disciplined and write independent
> test cases, an extra work is still necessary to run a case if a harness
> is unable to run just what you need.
> 
> And while we are here there is other problem with console-input-like
> tests: often you don't sure which statements results are checked
> intentionally and which ones are run only for its side effects.
> Sometimes you need to git-blame over several attempts to rewrite a test
> to understand what the idea was.
> 
> Maybe we are near to the point when such simple approach gives more
> problems then solves.
> 
> Yep, I stated problems here, but don't propose anything. I don't know
> where is a right balance between pushing developers to write structured
> tests and the freedom developers have now.
> 
> I see that some developers like ability to test something with injecting
> just one line for one case into another one. I think that it will make
> tests more and more embrassing. So, I don't know.

My point is that many issues are because of typos or another extra simple
mistakes, and what is more - they are linked somehow to a swarm of other
similar issues, already tested somewhere.

For example, the test you've fixed for netbox about 'unique' index
flag. It was just a typo, and we have a place, where other space/index
properties are tested. It is logically to test one another index property
together with others. Just imagine how many tests you would have had if we
had dedicated one test file to each space and index option. In such a
situation parallel test run will not help you, when number of test files
will be thousands. You just do not have so many cores so as to swiftly
start and shutdown new processes and threads.

Even SQLite does not dedicate new test file per each single tiny typo-like
issue. They have dedicated file per issue, but only for really hard ones.

> 
> WBR, Alexander Turenko.
> 

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

[Konstantin Osipov]

* Alexander Turenko <alexander.turenko@tarantool.org> [19/04/21 01:38]:

Adding tap-like commands to tell the harness about start/end of an
individual test case to our .test files is a great
idea. In fact, if you look at most of the .test files, they
already contain well-written individual cases. 
I think we should just do it.

> On Thu, Apr 18, 2019 at 05:18:04PM +0300, Konstantin Osipov wrote:
> > * Kirill Yukhin <kyukhin@tarantool.org> [19/04/18 17:12]:
> > > How ofter do you scan list of regression tests? I bet - never.
> > > But you very often try to extract 2-3 lines of failed case.
> > 
> > If you work with the test suite as an active maintainer you work
> > with the files all the time. Otherwise your tests just rot.
> 
> It does not matter much whether test cases spread around its own files
> or grouped into categories within one file. The key point here is to
> make cases independent. Even when you have independent test cases you
> still can extract common preparation code that will be run before each
> test case (or before a group of test cases).
> 
> Look how test cases are organized in our projects with pytest or with
> failsafe maven plugin. They are grouped into files, but I still able to
> do something like `pytest test/test_foo.py -k test_bar` or `mvn verify
> -Dit.test=FooIT#testBar` and concentrate on one case.
> 
> When test cases depend on each other you need to manually find and
> extract all needed parts of a test file to run a case. When you do it
> several times per day, hey, it becomes annoying.
> 
> Now even if developers highly self-disciplined and write independent
> test cases, an extra work is still necessary to run a case if a harness
> is unable to run just what you need.
> 
> And while we are here there is other problem with console-input-like
> tests: often you don't sure which statements results are checked
> intentionally and which ones are run only for its side effects.
> Sometimes you need to git-blame over several attempts to rewrite a test
> to understand what the idea was.
> 
> Maybe we are near to the point when such simple approach gives more
> problems then solves.
> 
> Yep, I stated problems here, but don't propose anything. I don't know
> where is a right balance between pushing developers to write structured
> tests and the freedom developers have now.
> 
> I see that some developers like ability to test something with injecting
> just one line for one case into another one. I think that it will make
> tests more and more embrassing. So, I don't know.
> 
> WBR, Alexander Turenko.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

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

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/18 17:12]:
> Now finally technically is ok except that I will never agree
> with the test-file-per-issue policy. It is ridiculous.

We should not use it, Kirill, please don't.
> 
> Following this logic I should name the whole swim subsystem
> test file like gh-3234-swim and rename the binary protocol
> test file into gh-3234-swim_proto - two files about the same
> issue with an ugly prefix looking like it is a fix of a bug. 
> 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

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

[Konstantin Osipov]

* Kirill Yukhin <kyukhin@tarantool.org> [19/04/15 17:06]:
> +/**
> + * Create an array of key_defs from array of index definitions.

Why allocate on region? We usually allocate key_defs with
malloc().

The function should be called index_def_to_key_def() or similar.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

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

[Kirill Yukhin]

Hello,

On 17 апр 16:38, Konstantin Osipov wrote:
> * Kirill Yukhin <kyukhin@tarantool.org> [19/04/15 17:06]:
> > +/**
> > + * Create an array of key_defs from array of index definitions.
> 
> Why allocate on region? We usually allocate key_defs with
> malloc().

I didn't change this peace of code. It was always alloacted on a region.
See [1].

> 
> The function should be called index_def_to_key_def() or similar.

Done (in another mail).

--
Regards, Kirill Yukhin

[1] - https://github.com/tarantool/tarantool/blob/master/src/box/memtx_space.c#L982

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

[Kirill Yukhin]

Hello,

On 15 Apr 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. To unify sysview's engine spaces with SQL views. This
>      will allow to use sysview machinery to query SQL views from
>      Lua land.
> 
> Closes #4111

I've checked the patch into 2.1 and master.

--
Regards, Kirill Yukhin