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

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

Hi! Thanks for the fixes!

On 25/10/2019 17:45, imeevma@tarantool.org wrote:
> Thank you for review! My answers, one question and new patch
> below.
> 
> On 10/19/19 1:08 AM, Vladislav Shpilevoy wrote:
>> Thanks for the patch!
>>
>> See 14 comments below.
>>
>> On 17/10/2019 16:40, imeevma@tarantool.org wrote:
>>> This patch creates the _vsql_settings sysview. This system view
>>> has custom methods get() and create_iterator(). This allows us
>>> to get SQL settings by creating a tuple on the fly without having
>>> to save it anywhere.
>>>
>>> Part of #4511
>>>
>>> @TarantoolBot document
>>
>> 1. I think, it is worth to create one doc request for the
>> whole feature - both SET command and _vsql_settings space.
>> They are useless without each other.
>>
> Fixed. However, do I need to write a doc-bot request now when the
> sysview purpose has changed?

Nah, I still think one request for the whole _vsession_settings
feature is enough. You describe what is _vsession_settings,
which options it has already, how to set them, and it should be
sufficient.

> diff --git a/src/box/session.cc b/src/box/session.cc
> index 59bf226..32dc87a 100644
> --- a/src/box/session.cc
> +++ b/src/box/session.cc

Recently I've seen a ticket https://github.com/tarantool/tarantool/issues/4205
about read-only sessions. That looks like a first candidate on a
non-SQL session local setting.

That means we probably need to design how to make session
settings iterator more generic. Currently all the settings
iteration code consists of explicit SQL things.

On the other hand I don't really know whether we should
consider read-only option a session setting.
And do we need to create a more generic session settings
iterator in scope of this patchset?

Here is my personal opinion how it should be done:

    - We push this patchset with current SQL-centric design of
      the settings iteration to stabilize the public API as
      soon as possible;

    - Later in scope of 4205 we add session-local option
      read-only and reimplement session settings iterator so
      as it would not depend on SQL so much.


See 6 comments below.

> @@ -360,3 +363,116 @@ generic_session_sync(struct session *session)
> +
> +struct iterator *
> +session_options_create_iterator(struct index *index, enum iterator_type type,
> +				const char *key, uint32_t part_count)
> +{
> +	int max_tuples_to_show = -1;
> +	uint32_t sql_option_id;
> +	if (part_count > 0) {
> +		assert(part_count == 1);
> +		uint32_t len;
> +		const char *name = mp_decode_str(&key, &len);
> +		name = tt_cstr(name, len);
> +		sql_option_id = sql_option_id_by_name(name);
> +		if (type == ITER_EQ || type == ITER_REQ)
> +			max_tuples_to_show = 1;
> +		else if (type == ITER_LT)
> +			--sql_option_id;
> +		else if (type == ITER_GT)
> +			++sql_option_id;
> +		if (sql_option_id == sql_option_id_max() && type == ITER_LE)
> +			--sql_option_id;

1. Strangely GE does not return anything here:

tarantool> box.space._vsession_settings:select({'sql_'}, {iterator = 'GE'})
---
- []
...

LE also behaves strange:

tarantool> box.space._vsession_settings:select({'sql_d'}, {iterator = 'LE'})
---
- - ['sql_where_trace', false]
  - ['sql_vdbe_trace', false]
  - ['sql_vdbe_listing', false]
  - ['sql_vdbe_eqp', false]
  - ['sql_vdbe_debug', false]
  - ['sql_vdbe_addoptrace', false]
  - ['sql_trace', false]
  - ['sql_select_trace', false]
  - ['sql_reverse_unordered_selects', false]
  - ['sql_recursive_triggers', true]
  - ['sql_parser_trace', false]
  - ['sql_full_column_names', false]
  - ['sql_defer_foreign_keys', false]
  - ['sql_default_engine', 'memtx']
  - ['sql_compound_select_limit', 30]
...

'sql_where_trace' definitely is not <= 'sql_d'.

This is why I was thinking that probably our _vsession_settings
iterator API should be very limited. After all, it is a fixed
size space with settings. A user don't need to be able to dance
with it using different iterators. I would allow only EQ, and either
empty key, or a subsystem name like 'sql', or a full key for a
concrete setting.

Up to you. And probably to Kirill since it is a public API.

> +	} else {
> +		sql_option_id = iterator_type_is_reverse(type) ?
> +				sql_option_id_max() - 1 : 0;
> +	}
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 51cd7ce..cb97106 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -3242,3 +3243,160 @@ sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
> +/**
> + * 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.
> + */

2. Please, add a test that selects all the options and checks
that they are sorted. It would prevent accidental reorders.

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

3. What is the option mask?

> +};
> +
> +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 region *region = &fiber()->gc;
> +	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];

4. So the SQL_LIMIT_COMPOUND_SELECT is not session local? Then
why is it in the session settings?

> +		size += mp_sizeof_uint(limit);
> +	}
> +
> +	size_t svp = region_used(region);
> +	char *pos_ret = region_alloc(region, size);

5. Up to you, but probably static_alloc() would work well here.
Size of option tuples is limited, and static_alloc() returns up
to 12KB that should be more than enough here. We could assert,
that static_alloc() returns not NULL for the given size.

> +	if (pos_ret == NULL) {
> +		diag_set(OutOfMemory, size, "region_alloc", "pos_ret");
> +		return -1;
> +	}
> +	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);
> +	region_truncate(region, svp);
> +	if (tuple == NULL)
> +		return -1;
> +	*result = tuple;
> +	return 0;
> +}
> diff --git a/test/box/misc.result b/test/box/misc.result
> index b293051..b41ffa4 100644
> --- a/test/box/misc.result
> +++ b/test/box/misc.result
> @@ -1330,3 +1330,124 @@ test_run:grep_log('default', 'test log')
>  ---
>  - test log
>  ...
> +--
> +-- gh-4511: make sure that _vsession_settings sysview works as
> +-- intended.
> +--
> +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
> +---
> +- true
> +...
> +(#v:select({'abcde'}, {iterator = 'ALL'})) == 0
> +---
> +- true
> +...
> +(#v:select({'abcde'}, {iterator = 'REQ'})) == 0
> +---
> +- true
> +...
> +(#v:select({'abcde'}, {iterator = 'EQ'})) == 0
> +---
> +- true
> +...
> +(#v:select({'abcde'}, {iterator = 'GE'})) == 0
> +---
> +- true
> +...
> +(#v:select({'abcde'}, {iterator = 'GT'})) == 0
> +---
> +- true
> +...
> +(#v:select({'abcde'}, {iterator = 'LE'})) == option_count
> +---
> +- true
> +...

6. Are you sure? We have no options, whose name is <= 'a'.
At least because all of them start with 'sql_' which > 'a'.

If you want to keep these iterator types, then probably
the most correct test would fill a normal space with
session settings, and check for each of these keys above,
that the virtual space and the real one return the same.