Re: [Tarantool-patches] [PATCH v3 3/5] sql: introduce SET statement

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

Hi! Thanks for the fixes!

See 5 comments below.

On 07/11/2019 13:36, imeevma@tarantool.org wrote:
> Due to the removal of sql_compound_select_limit from the session
> options, this patch has been modified.
> 
> New patch:
> 
> From 12ed4be2e7e433fdca58a43fc3b937eb9a54f52f Mon Sep 17 00:00:00 2001
> From: Mergen Imeev <imeevma@gmail.com>
> Date: Wed, 16 Oct 2019 16:43:10 +0300
> Subject: [PATCH] sql: introduce SET statement
> 
> This patch creates an SQL SET statement. This statement replaces
> pragmas that can modify SQL settings. List of pragmas that will
> have the corresponding option in SET:
> 'defer_foreign_keys'
> 'full_column_names'
> 'recursive_triggers'
> 'reverse_unordered_selects'
> 'sql_compound_select_limit'
> 'sql_default_engine'
> 
> 'parser_trace'
> 'select_trace'
> 'sql_trace'
> 'vdbe_addoptrace'
> 'vdbe_debug'
> 'vdbe_eqp'
> 'vdbe_listing'
> 'vdbe_trace'
> 'where_trace'
> 
> All these pragmas along with the pragmas 'short_column_names' and
> 'count_changes' will be removed in the next patch.
> 
> Part of #4511
> 
> @TarantoolBot document
> Title: SQL SET statement
> SQL SET statement is used to change SQL settings. To change the
> value of an SQL parameter, use the following syntax:
> 
> SET <name of the setting> = <value of the setting>;
> 
> Currently available SQL settings:
> 'sql_defer_foreign_keys'
> 'sql_full_column_names'
> 'sql_recursive_triggers'
> 'sql_reverse_unordered_selects'
> 'sql_compound_select_limit'
> 'sql_default_engine'
> 
> In addition, SQL debugging settings can also be changed using this
> statement in the debug build:
> 'sql_parser_trace'
> 'sql_select_trace'
> 'sql_trace'
> 'sql_vdbe_addoptrace'
> 'sql_vdbe_debug'
> 'sql_vdbe_eqp'
> 'sql_vdbe_listing'
> 'sql_vdbe_trace'
> 'sql_where_trace'
> 
> Example of usage:
> SET full_column_names = true;

1. I tried this:

tarantool> box.execute('SET full_column_names = true;')
---
- null
- Setting is not found
...

???

> SET sql_compound_select_limit = 10;
> SET sql_default_engine = 'memtx';

2. It would be cool to sum up here what is a purpose of
PRAGMA and SET. How are they different. From the code I
see, that looks like PRAGMA never changes anything. It
only returns something. While SET only sets and never
returns, right?

In the next patch I see diff like this:

> -        return test:execsql "PRAGMA full_column_names"
> -    end, {
> +        return box.space._vsession_settings:get("sql_full_column_names").value
> +    end,

Is there any plan how to make it simpler? Seems like it is
impossible for a user to get session settings other than via
direct select from a system space. It was simpler with PRAGMA.

> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index ce87b88..77f8dd4 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -3275,6 +3275,21 @@ enum {
>  	SQL_SESSION_OPTION_max,
>  };
>  
> +

3. Nit: unnecessary empty line.

> +/**
> + * Identifiers of all SQL global options that can be viewed. The
> + * identifier of the option is equal to its place in the sorted
> + * list of global options, which starts at 0.
> + *
> + * It is IMPORTANT that these 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.

4. First question - why should they be sorted? Second question -
aren't they removed from _vsession_settings space?

> + */
> +enum {
> +	SQL_GLOBAL_OPTION_COMPOUND_SELECT_LIMIT = 0,
> +	SQL_GLOBAL_OPTION_max,
> +};
> @@ -3382,3 +3408,82 @@ sql_session_opt_tuple(struct tuple_format *format, int option_id,
>  	*result = tuple;
>  	return 0;
>  }
> +
> +static void
> +sql_set_session_option(struct Parse *parse_context, int id, struct Expr *value)
> +{
> +	struct session *session = current_session();
> +	struct sql_option_metadata *option = &sql_session_opts[id];
> +	if (value->type != option->field_type) {
> +		diag_set(ClientError, ER_INCONSISTENT_TYPES,
> +			 field_type_strs[option->field_type],
> +			 field_type_strs[value->type]);
> +		parse_context->is_aborted = true;
> +		return;
> +	}
> +	if (value->type == FIELD_TYPE_BOOLEAN) {
> +		bool is_set = value->op == TK_TRUE;
> +		if (is_set)
> +			session->sql_flags |= option->mask;
> +		else
> +			session->sql_flags &= ~option->mask;
> +#ifndef NDEBUG
> +		if (id == SQL_SESSION_OPTION_PARSER_TRACE) {
> +			if (is_set)
> +				sqlParserTrace(stdout, "parser: ");
> +			else
> +				sqlParserTrace(NULL, NULL);
> +		}
> +#endif
> +	} else {
> +		assert(id == SQL_SESSION_OPTION_DEFAULT_ENGINE);
> +		enum sql_storage_engine engine =
> +			STR2ENUM(sql_storage_engine, value->u.zToken);
> +		if (engine == sql_storage_engine_MAX) {
> +			parse_context->is_aborted = true;
> +			diag_set(ClientError, ER_NO_SUCH_ENGINE,
> +				 value->u.zToken);
> +			return;
> +		}
> +		current_session()->sql_default_engine = engine;
> +	}
> +}

5. Wait. So the settings are set during parsing? Isn't it wrong?
'Parser only parses' and everything? PRAGMA works in runtime.