Re: [Tarantool-patches] [PATCH v5 3/3] box: add SQL settings to _session_settings

[Mergen Imeev <imeevma@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 <imeevma@gmail.com>
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