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 6116246970E for ; Mon, 30 Dec 2019 19:48:07 +0300 (MSK) Date: Mon, 30 Dec 2019 19:48:05 +0300 From: Mergen Imeev Message-ID: <20191230164805.GA27268@tarantool.org> References: <9b2bf828e3b58765ba76c59479a8bbd29d39b52b.1577455413.git.imeevma@gmail.com> <20191230112128.GO18639@tarantool.org> <20191230123813.GA28185@tarantool.org> <20191230124143.GA28330@tarantool.org> <20191230131518.GA74649@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20191230131518.GA74649@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v5 3/3] box: add SQL settings to _session_settings List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org Hi! Thank you for review. My answers and diff below. I send a new patch in a different trend. On Mon, Dec 30, 2019 at 03:15:18PM +0200, Nikita Pettik wrote: > On 30 Dec 15:41, Mergen Imeev wrote: > > Sorry, forgot to answer one question. The answer below. > > > > On Mon, Dec 30, 2019 at 03:38:13PM +0300, Mergen Imeev wrote: > > > Hi! Thank you fore review! My answers below. > > > > > > On Mon, Dec 30, 2019 at 01:21:28PM +0200, Nikita Pettik wrote: > > > > On 27 Dec 17:05, imeevma@tarantool.org wrote: > > > > > Part of #4511 > > > > > > > > > > Currently, this space contains only SQL settings, since the only > > > > > session settings are SQL settings. > > > > > > > > > > Debug build also have debug settings that could be obtained from > > > > > this sysview: > > > > > > > > > > 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 > > > > > > > > Imho there are too many debug options. We can easily merge > > > > half of them (where + select traces, vdbe eqp + debug). > > I agree, but I suggest to leave this to the next release. > > Should I create an issue? > > Why can't you do it right now? Nevertheless they are dubug > options it's not okay to release settings which are fated to > be remove the next day after release. > Done, now we have onle sql_vdbe_debug, sql_parser_debug and sql_select_debug. > > > > What is more, I think different set of options on debug and > > > > release builds is quite error prone. Mb it is worth keeping > > > > debug options all the time, but disallow setting their values > > > > to true? > > > > > > > It is possible, but I don’t think there will be problems, > > > since there are no numerical identifiers. Still, I do not > > > think that user should be able to see internal settings. Done. Diff: commit 97e24d0d342f287253ef3476954f3fb770d2005b Author: Mergen Imeev Date: Mon Dec 30 18:35:07 2019 +0300 Review fix diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 570a064..bc50ecb 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -3324,21 +3324,11 @@ enum { SQL_SESSION_SETTING_DEFER_FOREIGN_KEYS, SQL_SESSION_SETTING_FULL_COLUMN_NAMES, SQL_SESSION_SETTING_FULL_METADATA, -#ifndef NDEBUG - SQL_SESSION_SETTING_PARSER_TRACE, -#endif + SQL_SESSION_SETTING_PARSER_DEBUG, SQL_SESSION_SETTING_RECURSIVE_TRIGGERS, SQL_SESSION_SETTING_REVERSE_UNORDERED_SELECTS, -#ifndef NDEBUG - SQL_SESSION_SETTING_SELECT_TRACE, - SQL_SESSION_SETTING_TRACE, - SQL_SESSION_SETTING_VDBE_ADDOPTRACE, + SQL_SESSION_SETTING_SELECT_DEBUG, SQL_SESSION_SETTING_VDBE_DEBUG, - SQL_SESSION_SETTING_VDBE_EQP, - SQL_SESSION_SETTING_VDBE_LISTING, - SQL_SESSION_SETTING_VDBE_TRACE, - SQL_SESSION_SETTING_WHERE_TRACE, -#endif sql_session_setting_MAX, }; @@ -3347,21 +3337,11 @@ static const char *sql_session_setting_strs[sql_session_setting_MAX] = { "sql_defer_foreign_keys", "sql_full_column_names", "sql_full_metadata", -#ifndef NDEBUG - "sql_parser_trace", -#endif + "sql_parser_debug", "sql_recursive_triggers", "sql_reverse_unordered_selects", -#ifndef NDEBUG - "sql_select_trace", - "sql_trace", - "sql_vdbe_addoptrace", + "sql_select_debug", "sql_vdbe_debug", - "sql_vdbe_eqp", - "sql_vdbe_listing", - "sql_vdbe_trace", - "sql_where_trace", -#endif }; /** @@ -3389,33 +3369,18 @@ static struct sql_option_metadata sql_session_opts[] = { {FIELD_TYPE_BOOLEAN, SQL_FullColNames}, /** SQL_SESSION_SETTING_FULL_METADATA */ {FIELD_TYPE_BOOLEAN, SQL_FullMetadata}, -#ifndef NDEBUG - /** SQL_SESSION_SETTING_PARSER_TRACE */ - {FIELD_TYPE_BOOLEAN, PARSER_TRACE_FLAG}, -#endif + /** SQL_SESSION_SETTING_PARSER_DEBUG */ + {FIELD_TYPE_BOOLEAN, SQL_SqlTrace | PARSER_TRACE_FLAG}, /** SQL_SESSION_SETTING_RECURSIVE_TRIGGERS */ {FIELD_TYPE_BOOLEAN, SQL_RecTriggers}, /** SQL_SESSION_SETTING_REVERSE_UNORDERED_SELECTS */ {FIELD_TYPE_BOOLEAN, SQL_ReverseOrder}, -#ifndef NDEBUG - /** SQL_SESSION_SETTING_SELECT_TRACE */ - {FIELD_TYPE_BOOLEAN, SQL_SelectTrace}, - /** SQL_SESSION_SETTING_TRACE */ - {FIELD_TYPE_BOOLEAN, SQL_SqlTrace}, - /** SQL_SESSION_SETTING_VDBE_ADDOPTRACE */ - {FIELD_TYPE_BOOLEAN, SQL_VdbeAddopTrace}, + /** SQL_SESSION_SETTING_SELECT_DEBUG */ + {FIELD_TYPE_BOOLEAN, + SQL_SqlTrace | SQL_SelectTrace | SQL_WhereTrace}, /** SQL_SESSION_SETTING_VDBE_DEBUG */ {FIELD_TYPE_BOOLEAN, SQL_SqlTrace | SQL_VdbeListing | SQL_VdbeTrace}, - /** SQL_SESSION_SETTING_VDBE_EQP */ - {FIELD_TYPE_BOOLEAN, SQL_VdbeEQP}, - /** SQL_SESSION_SETTING_VDBE_LISTING */ - {FIELD_TYPE_BOOLEAN, SQL_VdbeListing}, - /** SQL_SESSION_SETTING_VDBE_TRACE */ - {FIELD_TYPE_BOOLEAN, SQL_VdbeTrace}, - /** SQL_SESSION_SETTING_WHERE_TRACE */ - {FIELD_TYPE_BOOLEAN, SQL_WhereTrace}, -#endif }; static void @@ -3463,12 +3428,19 @@ sql_set_boolean_option(int id, bool value) struct session *session = current_session(); struct sql_option_metadata *option = &sql_session_opts[id]; assert(option->field_type == FIELD_TYPE_BOOLEAN); +#ifdef NDEBUG + if ((session->sql_flags & SQL_SqlTrace) == 0) { + if (value) + session->sql_flags |= option->mask; + else + session->sql_flags &= ~option->mask; + } +#else if (value) session->sql_flags |= option->mask; else session->sql_flags &= ~option->mask; -#ifndef NDEBUG - if (id == SQL_SESSION_SETTING_PARSER_TRACE) { + if (id == SQL_SESSION_SETTING_PARSER_DEBUG) { if (value) sqlParserTrace(stdout, "parser: "); else diff --git a/test/box/gh-4511-access-settings-from-any-frontend.result b/test/box/gh-4511-access-settings-from-any-frontend.result index b021c29..64532a1 100644 --- a/test/box/gh-4511-access-settings-from-any-frontend.result +++ b/test/box/gh-4511-access-settings-from-any-frontend.result @@ -50,6 +50,19 @@ s:replace({'sql_defer_foreign_keys', true}) -- the same way as an ordinary space with an index of the type -- "TREE". -- +s:select() + | --- + | - - ['sql_default_engine', 'memtx'] + | - ['sql_defer_foreign_keys', false] + | - ['sql_full_column_names', false] + | - ['sql_full_metadata', false] + | - ['sql_parser_debug', false] + | - ['sql_recursive_triggers', true] + | - ['sql_reverse_unordered_selects', false] + | - ['sql_select_debug', false] + | - ['sql_vdbe_debug', false] + | ... + t = box.schema.space.create('settings', {format = s:format()}) | --- | ... diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/gh-4511-access-settings-from-any-frontend.test.lua index 9049fab..3668749 100644 --- a/test/box/gh-4511-access-settings-from-any-frontend.test.lua +++ b/test/box/gh-4511-access-settings-from-any-frontend.test.lua @@ -24,6 +24,8 @@ s:replace({'sql_defer_foreign_keys', true}) -- the same way as an ordinary space with an index of the type -- "TREE". -- +s:select() + t = box.schema.space.create('settings', {format = s:format()}) _ = t:create_index('primary') for _,value in s:pairs() do t:insert(value) end