From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 69F9E430D56 for ; Thu, 7 Nov 2019 15:34:45 +0300 (MSK) References: <12ed4be2e7e433fdca58a43fc3b937eb9a54f52f.1573121685.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: <9fe3bd05-17de-e878-4395-4d15cf2f0b38@tarantool.org> Date: Thu, 7 Nov 2019 15:40:39 +0300 MIME-Version: 1.0 In-Reply-To: <12ed4be2e7e433fdca58a43fc3b937eb9a54f52f.1573121685.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: imeevma@tarantool.org Cc: tarantool-patches@dev.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 > 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 ... ??? > 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.