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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

I used session_setting_find() in more places;
dropped P3 argument from OP_Set; replaced
region_alloc with static_alloc; added check that
vdbe is not NULL, because sql_set_setting may be the
first called function and may try to create a new
VDBE object; renamed sql_set_setting to sql_setting_set,
because in future we may add GET, and I want their C
emitters have the same prefix - sql_setting_*; made
SET argument expression instead of term, so I could use '?',
which is really useful when dealing with strings, against
SQL injections and to avoid string concatenations in
user's code.

================================================================================

commit 81182c89163581cb55fa40552bde779f0bbe025b
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Thu Feb 6 22:56:40 2020 +0100

    Review fixes

diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index c7cb4f6d4..3d06a191a 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -324,8 +324,8 @@ session_settings_index_get(struct index *base, const char *key,
 	uint32_t len;
 	key = mp_decode_str(&key, &len);
 	key = tt_cstr(key, len);
-	int sid = 0;
-	if (session_settings_set_forward(&sid, key, true, true) != 0) {
+	int sid = session_setting_find(key);
+	if (sid < 0) {
 		*result = NULL;
 		return 0;
 	}
@@ -426,7 +426,8 @@ session_settings_space_execute_update(struct space *space, struct txn *txn,
 	}
 	key = mp_decode_str(&key, &key_len);
 	key = tt_cstr(key, key_len);
-	if (session_settings_set_forward(&sid, key, true, true) != 0) {
+	sid = session_setting_find(key);
+	if (sid < 0) {
 		*result = NULL;
 		return 0;
 	}
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 259a16ab5..9cbe25443 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3471,20 +3471,21 @@ sql_session_settings_init()
 }
 
 void
-sql_set_setting(struct Parse *parse_context, struct Token *name,
+sql_setting_set(struct Parse *parse_context, struct Token *name,
 		struct Expr *expr)
 {
 	struct Vdbe *vdbe = sqlGetVdbe(parse_context);
-	assert(vdbe != NULL);
+	if (vdbe == NULL)
+		goto abort;
 	sqlVdbeCountChanges(vdbe);
-	char *key = sql_name_from_token(sql_get(), name);
+	char *key = sql_name_from_token(parse_context->db, 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);
+		sqlVdbeAddOp2(vdbe, OP_Set, index, target);
 		return;
 	}
 	diag_set(ClientError, ER_SQL_PARSER_GENERIC,
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 9d9b498a7..a01c37e19 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1542,8 +1542,8 @@ cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y).   {
 }
 
 ///////////////////////////// The SET command ////////////////////////////////
-cmd ::= SET nm(X) EQ term(Y).  {
-    sql_set_setting(pParse,&X,Y.pExpr);
+cmd ::= SET nm(X) EQ expr(Y).  {
+    sql_setting_set(pParse,&X,Y.pExpr);
 }
 
 ///////////////////////////// The PRAGMA command /////////////////////////////
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 5e192a7bb..aabada4a2 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4518,7 +4518,7 @@ sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
  * @param value New value of the session setting.
  */
 void
-sql_set_setting(struct Parse *parse_context, struct Token *name,
+sql_setting_set(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 77dcfc57a..282a02b69 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -5262,44 +5262,51 @@ case OP_IncMaxid: {
 	break;
 }
 
-/* Opcode: Set P1 P2 P3 * *
+/* Opcode: Set P1 P2 * * *
  *
- * 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.
+ * Set the new value of the session setting. P1 is the id of the
+ * setting in the session_settings array, P2 is the register
+ * holding a value.
  */
 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;
+	switch (setting->field_type) {
+	case FIELD_TYPE_BOOLEAN: {
+		if ((pIn1->flags & MEM_Bool) == 0)
+			goto invalid_type;
+		bool value = pIn1->u.b;
+		size_t size = mp_sizeof_bool(value);
+		char *mp_value = (char *) static_alloc(size);
+		mp_encode_bool(mp_value, value);
+		if (setting->set(sid, mp_value) != 0)
+			goto abort_due_to_error;
+		break;
+	}
+	case FIELD_TYPE_STRING: {
+		if ((pIn1->flags & MEM_Str) == 0)
+			goto invalid_type;
+		const char *str = pIn1->z;
+		size_t len = strlen(str);
+		uint32_t size = mp_sizeof_str(len);
+		char *mp_value = (char *) static_alloc(size);
+		if (mp_value == NULL) {
+			diag_set(OutOfMemory, size, "static_alloc", "mp_value");
+			goto abort_due_to_error;
 		}
-		default:
-			diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
-				 session_setting_strs[sid],
-				 field_type_strs[setting->field_type]);
+		mp_encode_str(mp_value, str, len);
+		if (setting->set(sid, mp_value) != 0)
 			goto abort_due_to_error;
+		break;
+	}
+	default:
+	invalid_type:
+		diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
+			 session_setting_strs[sid],
+			 field_type_strs[setting->field_type]);
+		goto abort_due_to_error;
 	}
 	p->nChange++;
 	break;
diff --git a/test/box/session_settings.result b/test/box/session_settings.result
index 9657ba58d..7c9802af6 100644
--- a/test/box/session_settings.result
+++ b/test/box/session_settings.result
@@ -363,7 +363,7 @@ s:get('sql_defer_foreign_keys').value
  | ---
  | - true
  | ...
-box.execute([[set "sql_defer_foreign_keys" = false]])
+box.execute([[set "sql_defer_foreign_keys" = ?]], {false})
  | ---
  | - row_count: 1
  | ...
diff --git a/test/box/session_settings.test.lua b/test/box/session_settings.test.lua
index d0f7ca0f7..f2d7e03f5 100644
--- a/test/box/session_settings.test.lua
+++ b/test/box/session_settings.test.lua
@@ -138,7 +138,7 @@ 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]])
+box.execute([[set "sql_defer_foreign_keys" = ?]], {false})
 s:get('sql_defer_foreign_keys').value
 
 settings.sql_default_engine:set(true)