[Tarantool-patches] [PATCH v2 3/5] sql: introduce SET statement

[imeevma@tarantool.org]

Thank you for review! My answers and new patch below.


On 10/19/19 1:08 AM, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 3 comments below.
>
> On 17/10/2019 16:40, imeevma@tarantool.org wrote:
>> This patch creates the SET command for SQL, which will be used
>> instead of pragmas that modify SQL settings.
>>
>> Part of #4511
>>
>> @TarantoolBot document
>> Title: SET SQL command
>> The SET SQL command is used to change SQL settings.
>
> 1. Please, provide SET syntax description. And say, that pragmas
> are dropped. And which of them are dropped, which are converted
> into SET.
>
Fixed.

>>
>> Currently available SQL settings:
>> 'defer_foreign_keys'
>> 'full_column_names'
>> 'recursive_triggers'
>> 'reverse_unordered_selects'
>> 'sql_compound_select_limit'
>> 'sql_default_engine'
>>
>> In addition, SQL debugging settings can also be changed using this
>> command in the debug build:
>> 'parser_trace'
>> 'select_trace'
>> 'sql_trace'
>> 'vdbe_addoptrace'
>> 'vdbe_debug'
>> 'vdbe_eqp'
>> 'vdbe_listing'
>> 'vdbe_trace'
>> 'where_trace'
>>
>> Example of usage:
>> SET full_column_names = true;
>> SET sql_compound_select_limit(10);
>> SET sql_default_engine = 'memtx';
>> ---
>>  src/box/sql/build.c         |  60 +++++++++++++++++++++
>>  src/box/sql/parse.y         |   8 +++
>>  src/box/sql/sqlInt.h        |  14 +++++
>>  test/sql/sql-debug.result   | 127 ++++++++++++++++++++++++++++++++++++++++++++
>>  test/sql/sql-debug.test.lua |  26 +++++++++
>>  5 files changed, 235 insertions(+)
>>
>> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>> index ed59a87..7bea68d 100644
>> --- a/src/box/sql/parse.y
>> +++ b/src/box/sql/parse.y
>> @@ -1535,6 +1535,14 @@ cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y).   {
>>    sql_drop_index(pParse);
>>  }
>>  
>> +///////////////////////////// The SET command ////////////////////////////////
>> +cmd ::= SET nm(X) EQ term(Y).  {
>> +    sql_set_settings(pParse,&X,Y.pExpr);
>> +}
>> +cmd ::= SET nm(X) LP term(Y) RP.         {
>> +    sql_set_settings(pParse,&X,Y.pExpr);
>
> 2. Do we really need both syntax variants?
>
No, I think. Fixed.

>> diff --git a/test/sql/sql-debug.result b/test/sql/sql-debug.result
>> index 2dba684..07542e3 100644
>> --- a/test/sql/sql-debug.result
>> +++ b/test/sql/sql-debug.result
>> @@ -54,3 +54,130 @@ box.execute('PRAGMA')
>>    - ['vdbe_trace', 0]
>>    - ['where_trace', 0]
>>  ...
>> +--
>> +-- gh-4511: make sure that SET works.
>> +--
>> +box.execute('SELECT "name" FROM "_vsql_settings";')
>> +---
>> +- metadata:
>> +  - name: name
>> +    type: string
>> +  rows:
>> +  - ['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']
>> +...
>> +engine = box.space._vsql_settings:get{'sql_default_engine'}[2]
>
> 3. Does field access by name work?
>
Yes, fixed.


New patch:

From 8444f08e9a52ec809208ae7e85a69b44014dc215 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
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 <name of the setting> = <value of the setting>;

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;
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 cb97106..3eef4dc 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3400,3 +3400,63 @@ sql_option_tuple(struct tuple_format *format, int option_id,
 	*result = tuple;
 	return 0;
 }
+
+void
+sql_set_settings(struct Parse *parse_context, struct Token *name,
+		 struct Expr *value)
+{
+	int option_id;
+	struct session *session = current_session();
+	char *name_str = sql_name_from_token(sql_get(), name);
+	if (name_str == NULL) {
+		parse_context->is_aborted = true;
+		return;
+	}
+	for (option_id = 0; option_id < SQL_OPTION_max; ++option_id) {
+		if (strcasecmp(sql_options[option_id].name, name_str) == 0)
+			break;
+	}
+	if (option_id == SQL_OPTION_max) {
+		diag_set(ClientError, ER_SQL_PARSER_GENERIC, "Setting is "
+			 "not found");
+		parse_context->is_aborted = true;
+		return;
+	}
+	struct sql_option_metadata *option = &sql_options[option_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 (option_id == SQL_OPTION_PARSER_TRACE) {
+			if (is_set)
+				sqlParserTrace(stdout, "parser: ");
+			else
+				sqlParserTrace(NULL, NULL);
+		}
+#endif
+	} else if (option_id == SQL_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;
+	} else {
+		assert(option_id == SQL_OPTION_COMPOUND_SELECT_LIMIT);
+		sql_limit(sql_get(), SQL_LIMIT_COMPOUND_SELECT,
+			  value->u.iValue);
+	}
+}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 1d0c95f..d0702d7 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1539,6 +1539,11 @@ cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y).   {
   sql_drop_index(pParse);
 }
 
+///////////////////////////// The SET command ////////////////////////////////
+cmd ::= SET nm(X) EQ term(Y).  {
+    sql_set_settings(pParse,&X,Y.pExpr);
+}
+
 ///////////////////////////// The PRAGMA command /////////////////////////////
 //
 cmd ::= PRAGMA nm(X).                        {
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index c32cacc..0bc8bbd 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4458,4 +4458,18 @@ int
 sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
 		    uint32_t *fieldno);
 
+/**
+ * Set new value for SQL setting.
+ *
+ * @param parse_context Parsing context.
+ * @param name Name of SQL setting to change.
+ * @param value New values of SQL setting.
+ *
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
+void
+sql_set_settings(struct Parse *parse_context, struct Token *name,
+		 struct Expr *value);
+
 #endif				/* sqlINT_H */
diff --git a/test/sql/sql-debug.result b/test/sql/sql-debug.result
index 2dba684..bfcc046 100644
--- a/test/sql/sql-debug.result
+++ b/test/sql/sql-debug.result
@@ -54,3 +54,134 @@ box.execute('PRAGMA')
   - ['vdbe_trace', 0]
   - ['where_trace', 0]
 ...
+--
+-- gh-4511: make sure that SET works.
+--
+box.execute('SELECT "name" FROM "_vsession_settings";')
+---
+- metadata:
+  - name: name
+    type: string
+  rows:
+  - ['sql_compound_select_limit']
+  - ['sql_default_engine']
+  - ['sql_defer_foreign_keys']
+  - ['sql_full_column_names']
+  - ['sql_parser_trace']
+  - ['sql_recursive_triggers']
+  - ['sql_reverse_unordered_selects']
+  - ['sql_select_trace']
+  - ['sql_trace']
+  - ['sql_vdbe_addoptrace']
+  - ['sql_vdbe_debug']
+  - ['sql_vdbe_eqp']
+  - ['sql_vdbe_listing']
+  - ['sql_vdbe_trace']
+  - ['sql_where_trace']
+...
+engine = box.space._vsession_settings:get{'sql_default_engine'}.value
+---
+...
+order = box.space._vsession_settings:get{'sql_reverse_unordered_selects'}.value
+---
+...
+box.execute('SET sql_default_engine = 1;')
+---
+- null
+- 'Inconsistent types: expected string got integer'
+...
+box.execute("SET sql_default_engine = 'some_engine';")
+---
+- null
+- Space engine 'some_engine' does not exist
+...
+box.execute("SET engine = 'vinyl';")
+---
+- null
+- Setting is not found
+...
+box.execute("SET sql_defer_foreign_keys = 'vinyl';")
+---
+- null
+- 'Inconsistent types: expected boolean got string'
+...
+engine == box.space._vsession_settings:get{'sql_default_engine'}.value
+---
+- true
+...
+order == box.space._vsession_settings:get{'sql_reverse_unordered_selects'}.value
+---
+- true
+...
+box.execute("SET sql_default_engine = 'vinyl';")
+---
+- row_count: 0
+...
+box.execute("SET sql_reverse_unordered_selects = true;")
+---
+- row_count: 0
+...
+box.execute('SELECT * FROM "_vsession_settings";')
+---
+- metadata:
+  - name: name
+    type: string
+  - name: value
+    type: any
+  rows:
+  - ['sql_where_trace', false]
+  - ['sql_vdbe_trace', false]
+  - ['sql_vdbe_listing', false]
+  - ['sql_vdbe_eqp', false]
+  - ['sql_vdbe_debug', false]
+  - ['sql_vdbe_addoptrace', false]
+  - ['sql_trace', false]
+  - ['sql_select_trace', false]
+  - ['sql_reverse_unordered_selects', true]
+  - ['sql_recursive_triggers', true]
+  - ['sql_parser_trace', false]
+  - ['sql_full_column_names', false]
+  - ['sql_defer_foreign_keys', false]
+  - ['sql_default_engine', 'vinyl']
+  - ['sql_compound_select_limit', 30]
+...
+box.execute("SET sql_default_engine = 'memtx';")
+---
+- row_count: 0
+...
+box.execute("SET sql_reverse_unordered_selects = false;")
+---
+- row_count: 0
+...
+box.execute('SELECT * FROM "_vsession_settings";')
+---
+- metadata:
+  - name: name
+    type: string
+  - name: value
+    type: any
+  rows:
+  - ['sql_compound_select_limit', 30]
+  - ['sql_default_engine', 'memtx']
+  - ['sql_defer_foreign_keys', false]
+  - ['sql_full_column_names', false]
+  - ['sql_parser_trace', false]
+  - ['sql_recursive_triggers', true]
+  - ['sql_reverse_unordered_selects', false]
+  - ['sql_select_trace', false]
+  - ['sql_trace', false]
+  - ['sql_vdbe_addoptrace', false]
+  - ['sql_vdbe_debug', false]
+  - ['sql_vdbe_eqp', false]
+  - ['sql_vdbe_listing', false]
+  - ['sql_vdbe_trace', false]
+  - ['sql_where_trace', false]
+...
+box.execute("SET sql_default_engine = '"..engine.."';")
+---
+- row_count: 0
+...
+box.execute("SET sql_reverse_unordered_selects = "..tostring(order)..";")
+---
+- row_count: 0
+...
diff --git a/test/sql/sql-debug.test.lua b/test/sql/sql-debug.test.lua
index edd0ef4..83746f0 100644
--- a/test/sql/sql-debug.test.lua
+++ b/test/sql/sql-debug.test.lua
@@ -15,3 +15,29 @@ box.execute('PRAGMA parser_trace = '.. result[1][1])
 -- Make PRAGMA command return the result as a result set.
 --
 box.execute('PRAGMA')
+
+--
+-- gh-4511: make sure that SET works.
+--
+box.execute('SELECT "name" FROM "_vsession_settings";')
+
+engine = box.space._vsession_settings:get{'sql_default_engine'}.value
+order = box.space._vsession_settings:get{'sql_reverse_unordered_selects'}.value
+
+box.execute('SET sql_default_engine = 1;')
+box.execute("SET sql_default_engine = 'some_engine';")
+box.execute("SET engine = 'vinyl';")
+box.execute("SET sql_defer_foreign_keys = 'vinyl';")
+engine == box.space._vsession_settings:get{'sql_default_engine'}.value
+order == box.space._vsession_settings:get{'sql_reverse_unordered_selects'}.value
+
+box.execute("SET sql_default_engine = 'vinyl';")
+box.execute("SET sql_reverse_unordered_selects = true;")
+box.execute('SELECT * FROM "_vsession_settings";')
+
+box.execute("SET sql_default_engine = 'memtx';")
+box.execute("SET sql_reverse_unordered_selects = false;")
+box.execute('SELECT * FROM "_vsession_settings";')
+
+box.execute("SET sql_default_engine = '"..engine.."';")
+box.execute("SET sql_reverse_unordered_selects = "..tostring(order)..";")
-- 
2.7.4