Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v2 2/5] box: introdice _vsession_settings sysview

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes!

See 7 comments below.

> diff --git a/src/box/session.cc b/src/box/session.cc
> index d69b657..6d301bf 100644
> --- a/src/box/session.cc
> +++ b/src/box/session.cc
> @@ -361,3 +364,135 @@ generic_session_sync(struct session *session)
>  	(void) session;
>  	return 0;
>  }
> +
> +int
> +session_options_get(struct index *index, const char *key, uint32_t part_count,
> +		    struct tuple **result)
> +{
> +	assert(part_count == 1);
> +	(void)part_count;
> +	struct space *space = space_cache_find(index->def->space_id);
> +	uint32_t len;
> +	const char *name = mp_decode_str(&key, &len);
> +	name = tt_cstr(name, len);
> +	/*
> +	 * Currently, the only session local options are SQL
> +	 * options.
> +	 */
> +	uint32_t id_max = sql_option_id_max();
> +	for (uint32_t id = 0; id < id_max; ++id) {
> +		if (sql_option_compare(name, id) == 0)
> +			return sql_option_tuple(space->format, id, result);
> +	}
> +	*result = NULL;
> +	return 0;
> +}
> +
> +/**
> + * An iterator that iterates over current session options.
> + */
> +struct session_options_iterator {
> +	/** Base iterator. Must be the first member. */
> +	struct iterator base;
> +	/** Format of the tuples this iterator returns. */
> +	struct tuple_format *format;
> +	/** Counter, count number of options already shown. */
> +	uint32_t counter;
> +	/** Decoded key. */
> +	char *key;
> +	/** Type of iterator. */
> +	enum iterator_type iterator_type;
> +};
> +
> +static int
> +session_options_iterator_next(struct iterator *itr, struct tuple **ret)
> +{
> +	struct session_options_iterator *it =
> +		(struct session_options_iterator *)itr;
> +	uint32_t id_max = sql_option_id_max();
> +	if (it->key == NULL)
> +		return sql_option_tuple(it->format, it->counter++, ret);
> +	for (; it->counter < id_max; ++it->counter) {
> +		int compare = sql_option_compare(it->key, it->counter);
> +		if (compare == 0 && (it->iterator_type == ITER_EQ ||
> +				     it->iterator_type == ITER_GE ||
> +				     it->iterator_type == ITER_ALL))
> +			break;
> +		if (compare > 0 && (it->iterator_type == ITER_GT ||
> +				    it->iterator_type == ITER_GE ||
> +				    it->iterator_type == ITER_ALL))
> +			break;
> +	}
> +	if (it->counter < id_max)
> +		return sql_option_tuple(it->format, it->counter++, ret);
> +	*ret = NULL;
> +	return 0;
> +}
> +
> +static int
> +session_options_iterator_prev(struct iterator *itr, struct tuple **ret)
> +{
> +	struct session_options_iterator *it =
> +		(struct session_options_iterator *)itr;
> +	uint32_t id_max = sql_option_id_max();
> +	if (it->key == NULL)
> +		return sql_option_tuple(it->format, id_max - 1 - it->counter++,
> +					ret);
> +	for (; it->counter < id_max; ++it->counter) {
> +		uint32_t id = id_max - it->counter - 1;

1. These 'id_max - 1 - counter' look really complex. Could you
simplify it? For example, rename it to 'current_id', and in
iterator_prev() decrement it instead of increment.

> +		int compare = sql_option_compare(it->key, id);
> +		if (compare == 0 && (it->iterator_type == ITER_REQ ||
> +				     it->iterator_type == ITER_LE))
> +			break;
> +		if (compare < 0 && (it->iterator_type == ITER_LT ||
> +				    it->iterator_type == ITER_LE))
> +			break;
> +	}
> +	if (it->counter < id_max)
> +		return sql_option_tuple(it->format, id_max - 1 - it->counter++,
> +					ret);
> +	*ret = NULL;
> +	return 0;
> +}
> +
> +static void
> +session_options_iterator_free(struct iterator *itr)
> +{
> +	struct session_options_iterator *it =
> +		(struct session_options_iterator *)itr;
> +	free(it->key);
> +	free(it);
> +}
> +
> +struct iterator *
> +session_options_create_iterator(struct index *index, enum iterator_type type,
> +				const char *key, uint32_t part_count)
> +{
> +	char *decoded_key = NULL;
> +	if (part_count > 0) {
> +		assert(part_count == 1);
> +		assert(mp_typeof(*key) == MP_STR);
> +		uint32_t len;
> +		const char *name = mp_decode_str(&key, &len);
> +		name = tt_cstr(name, len);
> +		decoded_key = (char *)malloc(len + 1);

2. I guess, you don't need tt_cstr() here, if you anyway
copy the key onto the heap memory.

3. You don't check malloc() result.

> +		memcpy(decoded_key, name, len + 1);
> +	}
> +	struct space *space = space_cache_find(index->def->space_id);
> +	struct session_options_iterator *it =
> +		(session_options_iterator *)malloc(sizeof(*it));
> +	if (it == NULL) {
> +		diag_set(OutOfMemory, sizeof(*it), "malloc", "it");
> +		return NULL;
> +	}
> +	iterator_create(&it->base, index);
> +	it->base.next = iterator_type_is_reverse(type) ?
> +			session_options_iterator_prev :
> +			session_options_iterator_next;
> +	it->base.free = session_options_iterator_free;
> +	it->key = decoded_key;
> +	it->iterator_type = type;
> +	it->format = space->format;
> +	it->counter = 0;
> +	return (struct iterator *)it;
> +}
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 51cd7ce..dceb092 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -3242,3 +3243,151 @@ sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
>  	*fieldno = i;
>  	return 0;
>  }
> +
> +/**
> + * Identifiers of all SQL options that can be viewed. The
> + * identifier of the option is equal to its place in the sorted
> + * list of options, which starts at 0.
> + *
> + * It is IMPORTANT that SQL options are sorted by name. If this is
> + * not the case, the result returned by the _vsession_settings
> + * space iterator will not be sorted properly.
> + */
> +enum {
> +	SQL_OPTION_COMPOUND_SELECT_LIMIT = 0,
> +	SQL_OPTION_DEFAULT_ENGINE,
> +	SQL_OPTION_DEFER_FOREIGN_KEYS,
> +	SQL_OPTION_FULL_COLUMN_NAMES,
> +#ifndef NDEBUG
> +	SQL_OPTION_PARSER_TRACE,
> +#endif
> +	SQL_OPTION_RECURSIVE_TRIGGERS,
> +	SQL_OPTION_REVERSE_UNORDERED_SELECTS,
> +#ifndef NDEBUG
> +	SQL_OPTION_SELECT_TRACE,
> +	SQL_OPTION_TRACE,
> +	SQL_OPTION_VDBE_ADDOPTRACE,
> +	SQL_OPTION_VDBE_DEBUG,
> +	SQL_OPTION_VDBE_EQP,
> +	SQL_OPTION_VDBE_LISTING,
> +	SQL_OPTION_VDBE_TRACE,
> +	SQL_OPTION_WHERE_TRACE,
> +#endif
> +	SQL_OPTION_max,
> +};
> +
> +/**
> + * A local structure that allows to establish a connection between
> + * the name of the parameter, its field type and mask, if it have
> + * one.
> + */
> +struct sql_option_metadata
> +{
> +	const char *name;
> +	uint32_t field_type;
> +	uint32_t mask;
> +};
> +
> +/**
> + * Variable that contains names of the SQL options, their field
> + * types and mask if they have one or 0 if don't have.
> + *
> + * It is IMPORTANT that these options sorted by name.
> + */
> +static struct sql_option_metadata sql_options[] = {
> +	/** SQL_OPTION_COMPOUND_SELECT_LIMIT */
> +	{"sql_compound_select_limit", FIELD_TYPE_INTEGER, 0},
> +	/** SQL_OPTION_DEFAULT_ENGINE */
> +	{"sql_default_engine", FIELD_TYPE_STRING, 0},
> +	/** SQL_OPTION_DEFER_FOREIGN_KEYS */
> +	{"sql_defer_foreign_keys", FIELD_TYPE_BOOLEAN, SQL_DeferFKs},
> +	/** SQL_OPTION_FULL_COLUMN_NAMES */
> +	{"sql_full_column_names", FIELD_TYPE_BOOLEAN, SQL_FullColNames},
> +#ifndef NDEBUG
> +	/** SQL_OPTION_PARSER_TRACE */
> +	{"sql_parser_trace", FIELD_TYPE_BOOLEAN, PARSER_TRACE_FLAG},
> +#endif
> +	/** SQL_OPTION_RECURSIVE_TRIGGERS */
> +	{"sql_recursive_triggers", FIELD_TYPE_BOOLEAN, SQL_RecTriggers},
> +	/** SQL_OPTION_REVERSE_UNORDERED_SELECTS */
> +	{"sql_reverse_unordered_selects", FIELD_TYPE_BOOLEAN, SQL_ReverseOrder},
> +#ifndef NDEBUG
> +	/** SQL_OPTION_SELECT_TRACE */
> +	{"sql_select_trace", FIELD_TYPE_BOOLEAN, SQL_SelectTrace},
> +	/** SQL_OPTION_TRACE */
> +	{"sql_trace", FIELD_TYPE_BOOLEAN, SQL_SqlTrace},
> +	/** SQL_OPTION_VDBE_ADDOPTRACE */
> +	{"sql_vdbe_addoptrace", FIELD_TYPE_BOOLEAN, SQL_VdbeAddopTrace},
> +	/** SQL_OPTION_VDBE_DEBUG */
> +	{"sql_vdbe_debug", FIELD_TYPE_BOOLEAN,
> +	 SQL_SqlTrace | SQL_VdbeListing | SQL_VdbeTrace},
> +	/** SQL_OPTION_VDBE_EQP */
> +	{"sql_vdbe_eqp", FIELD_TYPE_BOOLEAN, SQL_VdbeEQP},
> +	/** SQL_OPTION_VDBE_LISTING */
> +	{"sql_vdbe_listing", FIELD_TYPE_BOOLEAN, SQL_VdbeListing},
> +	/** SQL_OPTION_VDBE_TRACE */
> +	{"sql_vdbe_trace", FIELD_TYPE_BOOLEAN, SQL_VdbeTrace},
> +	/** SQL_OPTION_WHERE_TRACE */
> +	{"sql_where_trace", FIELD_TYPE_BOOLEAN, SQL_WhereTrace},
> +#endif
> +};
> +
> +uint32_t
> +sql_option_id_max()
> +{
> +	return SQL_OPTION_max;
> +}
> +
> +int
> +sql_option_compare(const char *key, uint32_t id)
> +{
> +	assert(id < SQL_OPTION_max);
> +	return strcmp(sql_options[id].name, key);
> +}
> +
> +int
> +sql_option_tuple(struct tuple_format *format, int option_id,
> +		 struct tuple **result)
> +{
> +	if (option_id < 0 || option_id >= SQL_OPTION_max) {
> +		*result = NULL;
> +		return 0;
> +	}
> +	int limit = 0;
> +	const char *engine = NULL;
> +	struct session *session = current_session();
> +	uint32_t flags = session->sql_flags;
> +	uint32_t mask = sql_options[option_id].mask;
> +	/* Tuple format contains two fields - name and value. */
> +	uint32_t column_count = format->min_field_count;
> +	assert(column_count == 2);
> +	size_t size = mp_sizeof_array(column_count) +
> +		      mp_sizeof_str(strlen(sql_options[option_id].name));
> +	if (sql_options[option_id].field_type == FIELD_TYPE_BOOLEAN) {
> +		size += mp_sizeof_bool(true);
> +	} else if (option_id == SQL_OPTION_DEFAULT_ENGINE) {
> +		engine = sql_storage_engine_strs[session->sql_default_engine];
> +		size += mp_sizeof_str(strlen(engine));
> +	} else {
> +		assert(option_id == SQL_OPTION_COMPOUND_SELECT_LIMIT);
> +		limit = sql_get()->aLimit[SQL_LIMIT_COMPOUND_SELECT];
> +		size += mp_sizeof_uint(limit);

4. I propose you to remove compound select setting from there. Since
it is not session local.

Another question is where to show its value then? Certainly not
here.

> +	}
> +
> +	char *pos_ret = static_alloc(size);
> +	assert(pos_ret != NULL);
> +	char *pos = mp_encode_array(pos_ret, column_count);
> +	pos = mp_encode_str(pos, sql_options[option_id].name,
> +			    strlen(sql_options[option_id].name));
> +	if (sql_options[option_id].field_type == FIELD_TYPE_BOOLEAN)
> +		pos = mp_encode_bool(pos, (flags & mask) == mask);
> +	else if (option_id == SQL_OPTION_DEFAULT_ENGINE)
> +		pos = mp_encode_str(pos, engine, strlen(engine));
> +	else
> +		pos = mp_encode_uint(pos, limit);
> +	struct tuple *tuple = tuple_new(format, pos_ret, pos_ret + size);
> +	if (tuple == NULL)
> +		return -1;
> +	*result = tuple;
> +	return 0;
> +}
> diff --git a/test/box/misc.result b/test/box/misc.result
> index b293051..1edd59e 100644
> --- a/test/box/misc.result
> +++ b/test/box/misc.result
> @@ -1330,3 +1330,174 @@ test_run:grep_log('default', 'test log')
>  ---
>  - test log
>  ...
> +--
> +-- gh-4511: make sure that _vsession_settings sysview works as
> +-- intended.

5. I just realized that probably Kirill would want it to be in a
separate file. But if he doesn't, then probably access_sysview.test.lua
fits better than misc.

> +--
> +v = box.space._vsession_settings
> +---
> +...
> +option_count = v:count()
> +---
> +...
> +v:format()
> +---
> +- [{'name': 'name', 'type': 'string'}, {'name': 'value', 'type': 'any'}]
> +...
> +(#v:select()) == option_count
> +---
> +- true
> +...
> +(#v:select({}, {iterator = 'ALL'})) == option_count
> +---
> +- true
> +...
> +(#v:select({}, {iterator = 'REQ'})) == option_count
> +---
> +- true
> +...
> +(#v:select({}, {iterator = 'EQ'})) == option_count
> +---
> +- true
> +...
> +(#v:select({}, {iterator = 'GE'})) == option_count
> +---
> +- true
> +...
> +(#v:select({}, {iterator = 'GT'})) == option_count
> +---
> +- true
> +...
> +(#v:select({}, {iterator = 'LE'})) == option_count
> +---
> +- true
> +...
> +(#v:select({}, {iterator = 'LT'})) == option_count

6. Why do you need braces around #v:select?

> +---
> +- true
> +...
> +s = box.schema.space.create('settings', {format = v:format()})
> +---
> +...
> +_ = s:create_index('primary')
> +---
> +...
> +for _,value in v:pairs() do s:insert(value) end
> +---
> +...
> +(#v:select({'abcde'}, {iterator = 'ALL'})) == (#s:select({'abcde'}, {iterator = 'ALL'}))
> +---
> +- true
> +...
> +(#v:select({'abcde'}, {iterator = 'REQ'})) == (#s:select({'abcde'}, {iterator = 'REQ'}))
> +---
> +- true
> +...
> +(#v:select({'abcde'}, {iterator = 'EQ'})) == (#s:select({'abcde'}, {iterator = 'EQ'}))
> +---
> +- true
> +...
> +(#v:select({'abcde'}, {iterator = 'GE'})) == (#s:select({'abcde'}, {iterator = 'GE'}))
> +---
> +- true
> +...
> +(#v:select({'abcde'}, {iterator = 'GT'})) == (#s:select({'abcde'}, {iterator = 'GT'}))
> +---
> +- true
> +...
> +(#v:select({'abcde'}, {iterator = 'LE'})) == (#s:select({'abcde'}, {iterator = 'LE'}))
> +---
> +- true
> +...
> +(#v:select({'abcde'}, {iterator = 'LT'})) == (#s:select({'abcde'}, {iterator = 'LT'}))
> +---
> +- true

7. Sorry, but counts are rather useless here, they don't check order nor content.
Please, check them properly. Also there is still no a single test about partial
keys. You check for empty, full, not found, but not for partial matches like 'sql_v'
+ GE/LE/GT/LT. Which already appeared to be buggy two times.

I propose you to implement a function, which takes a key, and checks
it with different iterator types on both 's' and 'v', and compares their result
sets. Then you call it with different keys and without tests code duplication.

Additionally, you could find useful gcov build to check code coverage locally.
Use make distclean && cmake . -DENABLE_GCOV=1 && make -j. Then run a needed
test. Then call

    gcov <path_to_session.cc> -o <path_to_session.gcno>

Then a file session.gcov is generated where you can check whether you have
covered your code with tests properly. File session.gcno should appear somewhere
after you run a test.

> +...
> +v:select({'sql_defer_foreign_keys'})
> +---
> +- - ['sql_defer_foreign_keys', false]
> +...
> +v:select({'sql_recursive_triggers'})
> +---
> +- - ['sql_recursive_triggers', true]
> +...
> +v:select({'sql_reverse_unordered_selects'})
> +---
> +- - ['sql_reverse_unordered_selects', false]
> +...
> +v:select({'sql_compound_select_limit'})
> +---
> +- - ['sql_compound_select_limit', 30]
> +...
> +v:select({'sql_default_engine'})
> +---
> +- - ['sql_default_engine', 'memtx']
> +...
> +-- Check get() method.
> +v:get({'sql_defer_foreign_keys'})
> +---
> +- ['sql_defer_foreign_keys', false]
> +...
> +v:get({'sql_recursive_triggers'})
> +---
> +- ['sql_recursive_triggers', true]
> +...
> +v:get({'sql_reverse_unordered_selects'})
> +---
> +- ['sql_reverse_unordered_selects', false]
> +...
> +v:get({'sql_compound_select_limit'})
> +---
> +- ['sql_compound_select_limit', 30]
> +...
> +v:get({'sql_default_engine'})
> +---
> +- ['sql_default_engine', 'memtx']
> +...
> +-- Make sure that space is read-only.
> +new_record = v:frommap({name='abs', value=123})
> +---
> +...
> +v:insert(new_record)
> +---
> +- error: View '_vsession_settings' is read-only
> +...
> +-- Check pairs() method.
> +t = {}
> +---
> +...
> +for key, value in box.space._vsession_settings:pairs() do table.insert(t, {key, value}) end
> +---
> +...
> +#t == option_count
> +---
> +- true
> +...
> +-- Check sorting.
> +in_sysview = v:select()
> +---
> +...
> +in_space = s:select()
> +---
> +...
> +is_right = true
> +---
> +...
> +for key, value in pairs(in_sysview) do is_right = is_right and (in_space[key].name == value.name) end
> +---
> +...
> +is_right
> +---
> +- true
> +...
> +-- Make sure that SQL SELECT works.
> +box.execute([[SELECT * from "_vsession_settings" WHERE "name" = 'sql_default_engine']])
> +---
> +- metadata:
> +  - name: name
> +    type: string
> +  - name: value
> +    type: any
> +  rows:
> +  - ['sql_default_engine', 'memtx']
> +...