From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (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 88F7C452566 for ; Sun, 17 Nov 2019 20:20:26 +0300 (MSK) References: <12ed4be2e7e433fdca58a43fc3b937eb9a54f52f.1573121685.git.imeevma@gmail.com> <9fe3bd05-17de-e878-4395-4d15cf2f0b38@tarantool.org> <20191107141209.GA10466@tarantool.org> <93979a0b-1708-1125-1142-74f22734b088@tarantool.org> <20191115140609.GA15961@tarantool.org> From: Vladislav Shpilevoy Message-ID: <3d76e64a-cd03-1bf1-5830-0d0b142a5e27@tarantool.org> Date: Sun, 17 Nov 2019 18:26:48 +0100 MIME-Version: 1.0 In-Reply-To: <20191115140609.GA15961@tarantool.org> 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: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the fixes! See 4 comments below. >>>>> @@ -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. >> >> 3. This is bad and should be fixed. >> https://github.com/tarantool/tarantool/issues/4621 > Fixed. Since control pragmas will be deleted, SET is now > using VDBE I closed the issue in this commit. Still, it > may be better to close it in the last commit of the > patch-set. Should I move it to the last commit? 1. You can try to fix it now, it is ok. Talking of control pragmas, PRAGMA still exists after your patch. And still works during parsing. I don't know what to do with it, because I don't know what are we going to do with PRAGMA. And seems like nobody knows. > 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 > Closes #4621 2. This commit message is getting worse and worse. I think it needs to be totally rewritten. Still existing problems with the current message: - I don't understand, which pragmas are totally deleted, which moved to SET, which of the moved have a new name; - From the message it still is not clear when a user needs to use PRAGMA and when SET. Yes, you said PRAGMA is purely informative, but I don't see it in the message. And the technical writer, who will see this message out of all the discussion, will be very confused with what has happened, and will ask for more details, I can guarantee that; - Parts of the message before '@Tarantoolbot document' and after are in some ideas different, in some they are totally the same. I propose you to remove the part before doc request, and write the doc request more accurate. And check again that all deleted PRAGMAs really don't work (in the last patch), all new SETs really work, with exactly the same names as in the commit message; - I think you need to explain what is a result of SET - metadata like DQL? Sql_info like DML/DDL? > > @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'; > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index ce87b88..8e3ba6a 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -3382,3 +3397,86 @@ sql_session_opt_tuple(struct tuple_format *format, int option_id, > *result = tuple; > return 0; > } > + > +int > +sql_set_session_option(int id, void *opt_value) > +{ > + struct Expr *value = (struct Expr *)opt_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]); > + return -1; > + } > + 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) { > + diag_set(ClientError, ER_NO_SUCH_ENGINE, > + value->u.zToken); > + return -1; > + } > + current_session()->sql_default_engine = engine; > + } > + return 0; > +} > + > +int > +sql_set_global_option(int id, void *opt_value) > +{ > + struct Expr *value = (struct Expr *)opt_value; > + (void)id; > + assert(id == SQL_GLOBAL_OPTION_COMPOUND_SELECT_LIMIT); > + int limit = value->u.iValue; > + int rc = sql_limit(sql_get(), SQL_LIMIT_COMPOUND_SELECT, limit); > + assert(rc >= 0); > + return rc < 0 ? -1 : 0; > +} > + > +void > +sql_set_settings(struct Parse *parse_context, struct Token *name, > + struct Expr *value) > +{ > + int opt_id; > + const char *val = (const char*) value; > + struct Vdbe *vdbe = sqlGetVdbe(parse_context); > + char *name_str = sql_name_from_token(sql_get(), name); > + if (name_str == NULL) { > + parse_context->is_aborted = true; > + return; > + } > + /* Try to find the option among the session options. */ > + for (opt_id = 0; opt_id < SQL_SESSION_OPTION_max; ++opt_id) { > + if (strcasecmp(sql_session_opts[opt_id].name, name_str) == 0) { > + sqlVdbeAddOp4(vdbe, OP_Set, 0, opt_id, 0, val, P4_PTR); > + return; 3. Expr is a parser time structure. You can't use it at runtime. 4. There is a leak. 'val' is not freed. > + } > + } > + /* Try to find the option among the global options. */ > + for (opt_id = 0; opt_id < SQL_GLOBAL_OPTION_max; ++opt_id) { > + if (strcasecmp(sql_global_opts[opt_id].name, name_str) == 0) { > + sqlVdbeAddOp4(vdbe, OP_Set, 1, opt_id, 0, val, P4_PTR); > + return; > + } > + } > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, "Setting is not found"); > + parse_context->is_aborted = true; > + return; > +}