From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E6BE9452566 for ; Thu, 7 Nov 2019 17:12:11 +0300 (MSK) Date: Thu, 7 Nov 2019 17:12:09 +0300 From: Mergen Imeev Message-ID: <20191107141209.GA10466@tarantool.org> References: <12ed4be2e7e433fdca58a43fc3b937eb9a54f52f.1573121685.git.imeevma@gmail.com> <9fe3bd05-17de-e878-4395-4d15cf2f0b38@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <9fe3bd05-17de-e878-4395-4d15cf2f0b38@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v3 3/5] sql: introduce SET statement List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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 > > 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 = ; > > > > 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 = ; 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';