[Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Feb 7 01:16:48 MSK 2020
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 at 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)
More information about the Tarantool-patches
mailing list