[Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings

Chris Sosnin k.sosnin at tarantool.org
Tue Feb 4 22:32:00 MSK 2020


Thank you for the review!
See my answers to your comments below:

> 1. All other places in SQL are case sensitive. I don't think this
> place should be an exception.

I thought it is convenient for users to omit quotes. Well, we can keep
it for consistency.

> 2. 1) struct Expr is a parsing stage object, it should not exist during
> execution, 2) it is generally a bad practice to save an object by a
> pointer, because that makes VDBE harder to serialize in future, when we
> will expropriate VDBE generation into a library. You should encode
> the expression as a set of VDBE operations storing result into a register,
> from where SET would read it. Ideally, it would support '?' parameters.
> See sqlExprCode().

> 3. Well, why not to expose session_settings_set_forward() into
> session_settings.h header and use it here?

Fixed:

+int
+session_setting_find(const char *name) {
+	int sid;
+	if (session_settings_set_forward(&sid, name, true, true) == 0)
+		return sid;
+	else
+		return -1;
+}
...
+	int index = session_setting_find(key);
+	if (index >= 0) {
+		int target = ++parse_context->nMem;
+		sqlExprCode(parse_context, expr, target);
+		sqlVdbeAddOp3(vdbe, OP_Set, index, target, expr->type);
+		return;
+	}

> 4. Please, avoid calling assert. Because in case it will fail,
> you won't get the real value, and it will be harder to debug.
> Especially if that test will become flaky some day. Assert is ok,
> if you know for sure what the other value is, or when there is a
> function/cycle, which can't print directly to the output.

Got it, tests fixed.

See new version below:
================================================================================

Currently if a user wants to change session setting with sql, he has
to execute non-obvious query, thus, we introduce a more native way to
do this.

Part of #4711
---
issue:https://github.com/tarantool/tarantool/issues/4711
branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings

 src/box/session_settings.c         |  9 ++++++
 src/box/session_settings.h         |  3 ++
 src/box/sql/build.c                | 24 +++++++++++++++
 src/box/sql/parse.y                |  5 +++
 src/box/sql/sqlInt.h               | 11 +++++++
 src/box/sql/vdbe.c                 | 44 +++++++++++++++++++++++++++
 test/box/session_settings.result   | 49 ++++++++++++++++++++++++++++++
 test/box/session_settings.test.lua | 13 ++++++++
 8 files changed, 158 insertions(+)

diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index 874fc304a..c7cb4f6d4 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -520,3 +520,12 @@ const struct space_vtab session_settings_space_vtab = {
 	/* .prepare_alter = */ generic_space_prepare_alter,
 	/* .invalidate = */ generic_space_invalidate,
 };
+
+int
+session_setting_find(const char *name) {
+	int sid;
+	if (session_settings_set_forward(&sid, name, true, true) == 0)
+		return sid;
+	else
+		return -1;
+}
diff --git a/src/box/session_settings.h b/src/box/session_settings.h
index 1f18f147b..34f4aeb7f 100644
--- a/src/box/session_settings.h
+++ b/src/box/session_settings.h
@@ -92,3 +92,6 @@ struct session_setting {
 
 extern struct session_setting session_settings[SESSION_SETTING_COUNT];
 extern const char *session_setting_strs[SESSION_SETTING_COUNT];
+
+int
+session_setting_find(const char *name);
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 3da60df86..f0e7df96f 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3459,3 +3459,27 @@ sql_session_settings_init()
 		setting->set = sql_session_setting_set;
 	}
 }
+
+void
+sql_set_setting(struct Parse *parse_context, struct Token *name,
+		struct Expr *expr)
+{
+	struct Vdbe *vdbe = sqlGetVdbe(parse_context);
+	assert(vdbe != NULL);
+	sqlVdbeCountChanges(vdbe);
+	char *key = sql_name_from_token(sql_get(), name);
+	if (key == NULL)
+		goto abort;
+	int index = session_setting_find(key);
+	if (index >= 0) {
+		int target = ++parse_context->nMem;
+		sqlExprCode(parse_context, expr, target);
+		sqlVdbeAddOp3(vdbe, OP_Set, index, target, expr->type);
+		return;
+	}
+	diag_set(ClientError, ER_SQL_PARSER_GENERIC,
+		 tt_sprintf("Session setting %s doesn't exist", key));
+abort:
+	parse_context->is_aborted = true;
+	return;
+}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index cfe1c0012..9d9b498a7 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1541,6 +1541,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_setting(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 d1fcf4761..5e192a7bb 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4510,4 +4510,15 @@ int
 sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
 		    uint32_t *fieldno);
 
+/**
+ * Create VDBE instructions to set new value of session setting.
+ *
+ * @param parse_context Parsing context.
+ * @param name Name of the session setting.
+ * @param value New value of the session setting.
+ */
+void
+sql_set_setting(struct Parse *parse_context, struct Token *name,
+		struct Expr *value);
+
 #endif				/* sqlINT_H */
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index eab74db4a..aa6e758c6 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -55,6 +55,7 @@
 #include "box/schema.h"
 #include "box/space.h"
 #include "box/sequence.h"
+#include "box/session_settings.h"
 
 /*
  * Invoke this macro on memory cells just prior to changing the
@@ -5261,6 +5262,49 @@ case OP_IncMaxid: {
 	break;
 }
 
+/* Opcode: Set P1 P2 P3 * *
+ *
+ * Set the new value of the session setting. P1 is the id of the setting
+ * in the session_settings array, P2 is the memory location of the
+ * new value, P3 is its field_type.
+ */
+case OP_Set: {
+	int sid = pOp->p1;
+	pIn1 = &aMem[pOp->p2];
+	assert(sid >= 0 && sid < SESSION_SETTING_COUNT);
+	struct session_setting *setting = &session_settings[sid];
+	switch (pOp->p3) {
+		case FIELD_TYPE_BOOLEAN: {
+			bool value = pIn1->u.b;
+			size_t size = mp_sizeof_bool(value);
+			char *mp_value = (char *)region_alloc(&fiber()->gc,
+							      size);
+			mp_encode_bool(mp_value, value);
+			if (setting->set(sid, mp_value) != 0)
+				goto abort_due_to_error;
+			break;
+		}
+		case FIELD_TYPE_STRING: {
+			const char *str = pIn1->z;
+			size_t len = strlen(str);
+			uint32_t size = mp_sizeof_str(len);
+			char *mp_value = (char *)region_alloc(&fiber()->gc,
+							      size);
+			mp_encode_str(mp_value, str, len);
+			if (setting->set(sid, mp_value) != 0)
+				goto abort_due_to_error;
+			break;
+		}
+		default:
+			diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
+				 session_setting_strs[sid],
+				 field_type_strs[setting->metadata.field_type]);
+			goto abort_due_to_error;
+	}
+	p->nChange++;
+	break;
+}
+
 /* Opcode: Noop * * * * *
  *
  * Do nothing.  This instruction is often useful as a jump
diff --git a/test/box/session_settings.result b/test/box/session_settings.result
index 403a4506c..42cc123a4 100644
--- a/test/box/session_settings.result
+++ b/test/box/session_settings.result
@@ -343,6 +343,39 @@ settings.sql_defer_foreign_keys
  | - false
  | ...
 
+box.execute([[set "sql_default_engine" = 'vinyl']])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - vinyl
+ | ...
+box.execute([[set "sql_default_engine" = 'memtx']])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - memtx
+ | ...
+box.execute([[set "sql_defer_foreign_keys" = true]])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_defer_foreign_keys').value
+ | ---
+ | - true
+ | ...
+box.execute([[set "sql_defer_foreign_keys" = false]])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_defer_foreign_keys').value
+ | ---
+ | - false
+ | ...
+
 settings.sql_default_engine:set(true)
  | ---
  | - null
@@ -357,3 +390,19 @@ settings.sql_parser_debug:set('string')
  | - null
  | - Session setting sql_parser_debug expected a value of type boolean
  | ...
+
+box.execute([[set "sql_def_engine" = true]])
+ | ---
+ | - null
+ | - Session setting sql_def_engine doesn't exist
+ | ...
+box.execute([[set "sql_default_engine" = true]])
+ | ---
+ | - null
+ | - Session setting sql_default_engine expected a value of type string
+ | ...
+box.execute([[set "sql_defer_foreign_keys" = 'true']])
+ | ---
+ | - null
+ | - Session setting sql_defer_foreign_keys expected a value of type boolean
+ | ...
diff --git a/test/box/session_settings.test.lua b/test/box/session_settings.test.lua
index 99559100d..2dfb201e5 100644
--- a/test/box/session_settings.test.lua
+++ b/test/box/session_settings.test.lua
@@ -132,6 +132,19 @@ s:get('sql_defer_foreign_keys').value
 s:update('sql_defer_foreign_keys', {{'=', 2, false}})
 settings.sql_defer_foreign_keys
 
+box.execute([[set "sql_default_engine" = 'vinyl']])
+s:get('sql_default_engine').value
+box.execute([[set "sql_default_engine" = 'memtx']])
+s:get('sql_default_engine').value
+box.execute([[set "sql_defer_foreign_keys" = true]])
+s:get('sql_defer_foreign_keys').value
+box.execute([[set "sql_defer_foreign_keys" = false]])
+s:get('sql_defer_foreign_keys').value
+
 settings.sql_default_engine:set(true)
 settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
 settings.sql_parser_debug:set('string')
+
+box.execute([[set "sql_def_engine" = true]])
+box.execute([[set "sql_default_engine" = true]])
+box.execute([[set "sql_defer_foreign_keys" = 'true']])
-- 
2.21.1 (Apple Git-122.3)



More information about the Tarantool-patches mailing list