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

[Mergen Imeev <imeevma@tarantool.org>]

Thank you for review! My answers, new commit-message and
diff below.

On Thu, Nov 07, 2019 at 03:40:39PM +0300, Vladislav Shpilevoy wrote:
> 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
> ...
> 
> ???
> 
Fixed commit-message. Now this option called
'sql_full_column_names'.

> > 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?
> 
How pragma works:
PRAGMA vdbe_trace = true; -- Sets vdbe_trace, returns nothing.
PRAGMA vdbe_trace; -- Returns current value of vdbe_trace flag.

How SET works:
SET sql_vdbe_trace = true; -- Sets vdbe_trace, returns nothing.

> 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.
> 
As far as I know, there is no such plans. It definitely was
simpler with PRAGMA, but it was decided to remove PRAGMA.
I do not know exact reaso why.

> > 
> > 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.
> 
Fixed. Diff below.

> > +/**
> > + * 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?
> 
Fixed:
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3275,16 +3275,7 @@ enum {
 	SQL_SESSION_OPTION_max,
 };
 
-
-/**
- * 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.
- */
+/**  Identifiers of all SQL global options that can be set. */
 enum {
 	SQL_GLOBAL_OPTION_COMPOUND_SELECT_LIMIT = 0,
 	SQL_GLOBAL_OPTION_max,
@@ -3347,8 +3338,6 @@ static struct sql_option_metadata sql_session_opts[] = {
 /**
  * Variable that contains names of the SQL global 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_global_opts[] = {
 	/** SQL_GLOBAL_OPTION_COMPOUND_SELECT_LIMIT */


> > + */
> > +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.
No, PRAGMA also set during parsing. I once suggested to
change it, so parser would only parse, but my suggestion
was declined.

Examples of pragma:

tarantool> box.execute('pragma vdbe_trace = 1')
---
- row_count: 0
...

tarantool> box.execute('pragma full_column_names = 1')
SQL: [pragma full_column_names = 1]
VDBE Trace:
 102>    0 Init             0    1    0               00 Start at 1
 102>    1 Halt             0    0    0               00 
---
- row_count: 0
...

tarantool> box.execute('pragma vdbe_trace = 1')
SQL: [pragma vdbe_trace = 1]
VDBE Trace:
 102>    0 Init             0    1    0               00 Start at 1
 102>    1 Halt             0    0    0               00 
---
- row_count: 0
...

New description:

    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'
    
    All of these setting with exception of 'sql_compound_select_limit'
    are session-local settings. Their value can be viewed in
    _vsession_settings sysview.
    
    Example of usage:
    SET sql_full_column_names = true;
    SET sql_compound_select_limit = 10;
    SET sql_default_engine = 'memtx';