[Tarantool-patches] [PATCH v5 3/3] box: add SQL settings to _session_settings
Mergen Imeev
imeevma at tarantool.org
Mon Dec 30 19:48:05 MSK 2019
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 at 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 at 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
More information about the Tarantool-patches
mailing list