* [Tarantool-patches] [PATCH] sql: provide a user friendly frontend for accessing session settings @ 2020-01-30 11:10 Chris Sosnin 2020-02-03 22:17 ` Vladislav Shpilevoy 0 siblings, 1 reply; 21+ messages in thread From: Chris Sosnin @ 2020-01-30 11:10 UTC (permalink / raw) To: v.shpilevoy, tarantool-patches 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 --- This patch is a follow-up for session settings patchset. issue:https://github.com/tarantool/tarantool/issues/4711 branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings src/box/sql/build.c | 31 ++++++++++++ src/box/sql/parse.y | 5 ++ src/box/sql/sqlInt.h | 11 +++++ src/box/sql/vdbe.c | 45 +++++++++++++++++ ...1-access-settings-from-any-frontend.result | 49 +++++++++++++++++++ ...access-settings-from-any-frontend.test.lua | 13 +++++ 6 files changed, 154 insertions(+) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 7dcf7b858..cb7733cfc 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -3474,3 +3474,34 @@ sql_session_settings_init() setting->set = sql_session_setting_set; } } + +void +sql_set_setting(struct Parse *parse_context, struct Token *name, + struct Expr *value) +{ + 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 low = 0, high = session_setting_MAX - 1; + while (low <= high) { + int index = (high + low) / 2; + int cmp = strcasecmp(session_settings[index].name, key); + if (cmp == 0) { + sqlVdbeAddOp4(vdbe, OP_Set, index, 0, 0, + (const char *)value, P4_PTR); + return; + } + if (cmp < 0) + low = index + 1; + else + high = index - 1; + } + 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..0ee90de47 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,50 @@ case OP_IncMaxid: { break; } +/* Opcode: Set P1 * * P4 * + * + * Set new value to session setting. P1 is ID of the setting. + * P4 is actually of type "struct Expr *" and contains value + * of the setting. + */ +case OP_Set: { + assert(pOp->p4type == P4_PTR); + struct Expr *expr = (struct Expr *)pOp->p4.p; + int sid = pOp->p1; + assert(sid >= 0 && sid < session_setting_MAX); + struct session_setting *setting = &session_settings[sid]; + switch (expr->type) { + case FIELD_TYPE_BOOLEAN: { + bool value = expr->op == TK_TRUE; + 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 = expr->u.zToken; + 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, + setting->name, + 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/gh-4511-access-settings-from-any-frontend.result b/test/box/gh-4511-access-settings-from-any-frontend.result index 1c3ca7661..0597365a1 100644 --- a/test/box/gh-4511-access-settings-from-any-frontend.result +++ b/test/box/gh-4511-access-settings-from-any-frontend.result @@ -343,6 +343,39 @@ settings.sql_defer_foreign_keys | - false | ... +box.execute([[set sql_default_engine = 'vinyl']]) + | --- + | - row_count: 1 + | ... +assert(s:get('sql_default_engine').value == 'vinyl') + | --- + | - true + | ... +box.execute([[set sql_default_engine = 'memtx']]) + | --- + | - row_count: 1 + | ... +assert(s:get('sql_default_engine').value == 'memtx') + | --- + | - true + | ... +box.execute([[set sql_defer_foreign_keys = true]]) + | --- + | - row_count: 1 + | ... +assert(s:get('sql_defer_foreign_keys').value == true) + | --- + | - true + | ... +box.execute([[set sql_defer_foreign_keys = false]]) + | --- + | - row_count: 1 + | ... +assert(s:get('sql_defer_foreign_keys').value == false) + | --- + | - true + | ... + settings.sql_default_engine:set(true) | --- | - error: Session setting sql_default_engine expected a value of type string @@ -355,3 +388,19 @@ settings.sql_parser_debug:set('string') | --- | - error: 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/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/gh-4511-access-settings-from-any-frontend.test.lua index 53f03450d..e93edaf98 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 @@ -132,6 +132,19 @@ assert(s:get('sql_defer_foreign_keys').value == true) s:update('sql_defer_foreign_keys', {{'=', 2, false}}) settings.sql_defer_foreign_keys +box.execute([[set sql_default_engine = 'vinyl']]) +assert(s:get('sql_default_engine').value == 'vinyl') +box.execute([[set sql_default_engine = 'memtx']]) +assert(s:get('sql_default_engine').value == 'memtx') +box.execute([[set sql_defer_foreign_keys = true]]) +assert(s:get('sql_defer_foreign_keys').value == true) +box.execute([[set sql_defer_foreign_keys = false]]) +assert(s:get('sql_defer_foreign_keys').value == false) + 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: provide a user friendly frontend for accessing session settings 2020-01-30 11:10 [Tarantool-patches] [PATCH] sql: provide a user friendly frontend for accessing session settings Chris Sosnin @ 2020-02-03 22:17 ` Vladislav Shpilevoy 2020-02-04 19:32 ` [Tarantool-patches] [PATCH 4/4] " Chris Sosnin 0 siblings, 1 reply; 21+ messages in thread From: Vladislav Shpilevoy @ 2020-02-03 22:17 UTC (permalink / raw) To: Chris Sosnin, tarantool-patches Thanks for the patch! See 4 comments below. On 30/01/2020 12:10, Chris Sosnin wrote: > 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 > --- > This patch is a follow-up for session settings patchset. > > issue:https://github.com/tarantool/tarantool/issues/4711 > branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings > > src/box/sql/build.c | 31 ++++++++++++ > src/box/sql/parse.y | 5 ++ > src/box/sql/sqlInt.h | 11 +++++ > src/box/sql/vdbe.c | 45 +++++++++++++++++ > ...1-access-settings-from-any-frontend.result | 49 +++++++++++++++++++ > ...access-settings-from-any-frontend.test.lua | 13 +++++ > 6 files changed, 154 insertions(+) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 7dcf7b858..cb7733cfc 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -3474,3 +3474,34 @@ sql_session_settings_init() > setting->set = sql_session_setting_set; > } > } > + > +void > +sql_set_setting(struct Parse *parse_context, struct Token *name, > + struct Expr *value) > +{ > + 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 low = 0, high = session_setting_MAX - 1; > + while (low <= high) { > + int index = (high + low) / 2; > + int cmp = strcasecmp(session_settings[index].name, key); 1. All other places in SQL are case sensitive. I don't think this place should be an exception. > + if (cmp == 0) { > + sqlVdbeAddOp4(vdbe, OP_Set, index, 0, 0, > + (const char *)value, P4_PTR); 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(). > + return; > + } > + if (cmp < 0) > + low = index + 1; > + else > + high = index - 1; > + } 3. Well, why not to expose session_settings_set_forward() into session_settings.h header and use it here? > + 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/test/box/gh-4511-access-settings-from-any-frontend.result b/test/box/gh-4511-access-settings-from-any-frontend.result > index 1c3ca7661..0597365a1 100644 > --- a/test/box/gh-4511-access-settings-from-any-frontend.result > +++ b/test/box/gh-4511-access-settings-from-any-frontend.result > @@ -343,6 +343,39 @@ settings.sql_defer_foreign_keys > | - false > | ... > > +box.execute([[set sql_default_engine = 'vinyl']]) > + | --- > + | - row_count: 1 > + | ... > +assert(s:get('sql_default_engine').value == 'vinyl') 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. > + | --- > + | - true > + | ... > +box.execute([[set sql_default_engine = 'memtx']]) > + | --- > + | - row_count: 1 > + | ... > +assert(s:get('sql_default_engine').value == 'memtx') > + | --- > + | - true > + | ... > +box.execute([[set sql_defer_foreign_keys = true]]) > + | --- > + | - row_count: 1 > + | ... > +assert(s:get('sql_defer_foreign_keys').value == true) > + | --- > + | - true > + | ... > +box.execute([[set sql_defer_foreign_keys = false]]) > + | --- > + | - row_count: 1 > + | ... > +assert(s:get('sql_defer_foreign_keys').value == false) > + | --- > + | - true > + | ... > + ^ permalink raw reply [flat|nested] 21+ messages in thread
* [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-02-03 22:17 ` Vladislav Shpilevoy @ 2020-02-04 19:32 ` Chris Sosnin 2020-02-06 22:16 ` Vladislav Shpilevoy 0 siblings, 1 reply; 21+ messages in thread From: Chris Sosnin @ 2020-02-04 19:32 UTC (permalink / raw) To: v.shpilevoy, tarantool-patches 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-02-04 19:32 ` [Tarantool-patches] [PATCH 4/4] " Chris Sosnin @ 2020-02-06 22:16 ` Vladislav Shpilevoy 2020-02-07 9:40 ` Chris Sosnin 0 siblings, 1 reply; 21+ messages in thread From: Vladislav Shpilevoy @ 2020-02-06 22:16 UTC (permalink / raw) To: Chris Sosnin, tarantool-patches 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-02-06 22:16 ` Vladislav Shpilevoy @ 2020-02-07 9:40 ` Chris Sosnin 2020-02-10 22:09 ` Vladislav Shpilevoy 0 siblings, 1 reply; 21+ messages in thread From: Chris Sosnin @ 2020-02-07 9:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 714 bytes --] Hi! Thank you very much for your fixes! I squashed all of your 4 commits and added some minor changes: — Fixed indentation in session.c for switch statement — Removed computation of string length from OP_Set since in struct Mem we have int n; /* size (in bytes) of string value, excluding trailing '\0' */ branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings <https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings> issue #1: https://github.com/tarantool/tarantool/issues/4711 <https://github.com/tarantool/tarantool/issues/4711> issue #2: https://github.com/tarantool/tarantool/issues/4712 <https://github.com/tarantool/tarantool/issues/4712> [-- Attachment #2: Type: text/html, Size: 1175 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-02-07 9:40 ` Chris Sosnin @ 2020-02-10 22:09 ` Vladislav Shpilevoy 2020-02-17 11:46 ` Chris Sosnin 0 siblings, 1 reply; 21+ messages in thread From: Vladislav Shpilevoy @ 2020-02-10 22:09 UTC (permalink / raw) To: Chris Sosnin; +Cc: tarantool-patches Hi! Thanks for the fixes! On the branch you said this commit is 'Part of #4711'. Why not 'Closes #4711'? What else is left? > sql: provide a user friendly frontend for accessing session settings > > 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 > > @TarantoolBot document > Title: API for accessing _session_settings space. > There are two ways of updating values of session settings: > via Lua and SQL. > > Lua: > box.session.settings is a table, which is accessible right after > session creation. The syntax is the following: Lets say it is available always. Because a user can't live without a session anyway. It always exists for a user. > `box.session.settings.<setting_name>:set(<new_value>)`. > > Example of usage: > ``` > tarantool> box.session.settings.sql_default_engine > --- > - memtx > ... > > tarantool> box.session.settings.sql_default_engine:set('vinyl') > --- > ... > > ``` > > The table itself represents the (unordered) result of select > from _session_settings space. Every setting is implemented as > a table, so there is no way to retrieve an actual value and use > it until :get() method is introduced. > > SQL: > Instead of typing long UPDATE query one can use the SET statement: > `box.execute([[SET "<setting_name>" = <new_value>]])`. > Note, that this query is case sensitive so the name must be quoted. > > Example: > ``` > tarantool> box.execute([[set "sql_default_engine" = 'memtx']]) > --- > - row_count: 1 > ... > > tarantool> box.execute([[set "sql_defer_foreign_keys" = true]]) > --- > - row_count: 1 > ... > > ``` I am not so sure about binary search anymore. I just found another probably much faster way - perfect hash function. There is a GNU tool 'gperf' to generate C code for a given keyset with hash calculation and search functions. I fed our settings to it and obtained not so scary result. File: test.gperf %% sql_default_engine sql_defer_foreign_keys sql_full_column_names sql_full_metadata sql_parser_debug sql_recursive_triggers sql_reverse_unordered_selects sql_select_debug sql_vdbe_debug %% Command: gperf test.gperf > test.c The test.c file is 122 lines long including some {}, useless checks, macros, and comments. It could easily be compacted a lot. This is probably overkill though. It needs to be completely regenerated each time when the keyset is changed, consumes more static memory. The patchset LGTM. I propose to send it to Nikita on a second review. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-02-10 22:09 ` Vladislav Shpilevoy @ 2020-02-17 11:46 ` Chris Sosnin 2020-02-17 11:56 ` Nikita Pettik 0 siblings, 1 reply; 21+ messages in thread From: Chris Sosnin @ 2020-02-17 11:46 UTC (permalink / raw) To: Vladislav Shpilevoy, tarantool-patches, Nikita Pettik [-- Attachment #1: Type: text/plain, Size: 3823 bytes --] > 11 февр. 2020 г., в 01:09, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а): > > Hi! Thanks for the fixes! > > On the branch you said this commit is 'Part of #4711'. Why > not 'Closes #4711'? What else is left? > Replaced with ‘Closes’. >> sql: provide a user friendly frontend for accessing session settings >> >> 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 >> >> @TarantoolBot document >> Title: API for accessing _session_settings space. >> There are two ways of updating values of session settings: >> via Lua and SQL. >> >> Lua: >> box.session.settings is a table, which is accessible right after >> session creation. The syntax is the following: > > Lets say it is available always. Because a user can't live without > a session anyway. It always exists for a user. Commited the following change: which is accessible right after session creation —> which is always accessible to user >> `box.session.settings.<setting_name>:set(<new_value>)`. >> >> Example of usage: >> ``` >> tarantool> box.session.settings.sql_default_engine >> --- >> - memtx >> ... >> >> tarantool> box.session.settings.sql_default_engine:set('vinyl') >> --- >> ... >> >> ``` >> >> The table itself represents the (unordered) result of select >> from _session_settings space. Every setting is implemented as >> a table, so there is no way to retrieve an actual value and use >> it until :get() method is introduced. >> >> SQL: >> Instead of typing long UPDATE query one can use the SET statement: >> `box.execute([[SET "<setting_name>" = <new_value>]])`. >> Note, that this query is case sensitive so the name must be quoted. >> >> Example: >> ``` >> tarantool> box.execute([[set "sql_default_engine" = 'memtx']]) >> --- >> - row_count: 1 >> ... >> >> tarantool> box.execute([[set "sql_defer_foreign_keys" = true]]) >> --- >> - row_count: 1 >> ... >> >> ``` > > I am not so sure about binary search anymore. I just found another > probably much faster way - perfect hash function. There is a GNU > tool 'gperf' to generate C code for a given keyset with hash > calculation and search functions. > > I fed our settings to it and obtained not so scary result. > > File: test.gperf > > %% > sql_default_engine > sql_defer_foreign_keys > sql_full_column_names > sql_full_metadata > sql_parser_debug > sql_recursive_triggers > sql_reverse_unordered_selects > sql_select_debug > sql_vdbe_debug > %% > > Command: gperf test.gperf > test.c > > The test.c file is 122 lines long including some {}, useless > checks, macros, and comments. It could easily be compacted a lot. I like the idea of speeding search up, but it would make adding new settings even more complicated (+regenerate file with gpref, +mannualy format it). > > This is probably overkill though. It needs to be completely > regenerated each time when the keyset is changed, consumes more > static memory. I agree. > > The patchset LGTM. I propose to send it to Nikita on a second > review. Nikita, please do a second review. branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings <https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings> issue #1: https://github.com/tarantool/tarantool/issues/4711 <https://github.com/tarantool/tarantool/issues/4711> issue #2: https://github.com/tarantool/tarantool/issues/4712 <https://github.com/tarantool/tarantool/issues/4712> [-- Attachment #2: Type: text/html, Size: 6686 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-02-17 11:46 ` Chris Sosnin @ 2020-02-17 11:56 ` Nikita Pettik 0 siblings, 0 replies; 21+ messages in thread From: Nikita Pettik @ 2020-02-17 11:56 UTC (permalink / raw) To: Chris Sosnin; +Cc: tarantool-patches, Vladislav Shpilevoy On 17 Feb 14:46, Chris Sosnin wrote: > > > > 11 февр. 2020 г., в 01:09, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а): > > > > The patchset LGTM. I propose to send it to Nikita on a second > > review. > > Nikita, please do a second review. Could you resend whole patch-set? > branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings <https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings> > issue #1: https://github.com/tarantool/tarantool/issues/4711 <https://github.com/tarantool/tarantool/issues/4711> > issue #2: https://github.com/tarantool/tarantool/issues/4712 <https://github.com/tarantool/tarantool/issues/4712> ^ permalink raw reply [flat|nested] 21+ messages in thread
* [Tarantool-patches] [PATCH 0/4] box: session settings fixes
@ 2020-02-17 12:12 Chris Sosnin
2020-02-17 12:12 ` [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings Chris Sosnin
0 siblings, 1 reply; 21+ messages in thread
From: Chris Sosnin @ 2020-02-17 12:12 UTC (permalink / raw)
To: korablev, tarantool-patches
The first patch just merges all modules into one array, so the
binary search will work once for all settings.
The second patch is implementation of the binary search.
The last two patches add frontend for accessing session settings:
Lua table and SQL statement respectively.
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings
issue #1: https://github.com/tarantool/tarantool/issues/4711
issue #2: https://github.com/tarantool/tarantool/issues/4712
Chris Sosnin (4):
box: replace session_settings modules with a single array
box: add binary search for _session_settings space
box: provide a user friendly frontend for accessing session settings
sql: provide a user friendly frontend for accessing session settings
src/box/lua/session.c | 92 ++++++++
src/box/session.cc | 1 +
src/box/session.h | 2 +
src/box/session_settings.c | 214 +++++++++++-------
src/box/session_settings.h | 53 +++--
src/box/sql.c | 5 -
src/box/sql/build.c | 104 ++++-----
src/box/sql/parse.y | 5 +
src/box/sql/sqlInt.h | 11 +
src/box/sql/vdbe.c | 50 ++++
...rontend.result => session_settings.result} | 149 ++++++++++--
...end.test.lua => session_settings.test.lua} | 61 ++++-
12 files changed, 571 insertions(+), 176 deletions(-)
rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (71%)
rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (65%)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 21+ messages in thread* [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-02-17 12:12 [Tarantool-patches] [PATCH 0/4] box: session settings fixes Chris Sosnin @ 2020-02-17 12:12 ` Chris Sosnin 2020-03-16 17:02 ` Nikita Pettik 0 siblings, 1 reply; 21+ messages in thread From: Chris Sosnin @ 2020-02-17 12:12 UTC (permalink / raw) To: korablev, tarantool-patches 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. Closes #4711 @TarantoolBot document Title: API for accessing _session_settings space. There are two ways of updating values of session settings: via Lua and SQL. Lua: box.session.settings is a table, which is always accessible to user. The syntax is the following: `box.session.settings.<setting_name>:set(<new_value>)`. Example of usage: ``` tarantool> box.session.settings.sql_default_engine --- - memtx ... tarantool> box.session.settings.sql_default_engine:set('vinyl') --- ... ``` The table itself represents the (unordered) result of select from _session_settings space. Every setting is implemented as a table, so there is no way to retrieve an actual value and use it until :get() method is introduced. SQL: Instead of typing long UPDATE query one can use the SET statement: `box.execute([[SET "<setting_name>" = <new_value>]])`. Note, that this query is case sensitive so the name must be quoted. Example: ``` tarantool> box.execute([[set "sql_default_engine" = 'memtx']]) --- - row_count: 1 ... tarantool> box.execute([[set "sql_defer_foreign_keys" = true]]) --- - row_count: 1 ... ``` --- src/box/session_settings.c | 16 ++++++++-- src/box/session_settings.h | 3 ++ src/box/sql/build.c | 25 +++++++++++++++ src/box/sql/parse.y | 5 +++ src/box/sql/sqlInt.h | 11 +++++++ src/box/sql/vdbe.c | 50 ++++++++++++++++++++++++++++++ test/box/session_settings.result | 49 +++++++++++++++++++++++++++++ test/box/session_settings.test.lua | 13 ++++++++ 8 files changed, 169 insertions(+), 3 deletions(-) diff --git a/src/box/session_settings.c b/src/box/session_settings.c index 874fc304a..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; } @@ -520,3 +521,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 de24e3c6f..e2adc5289 100644 --- a/src/box/session_settings.h +++ b/src/box/session_settings.h @@ -84,3 +84,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 e5fde08cc..9cbe25443 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -3469,3 +3469,28 @@ sql_session_settings_init() setting->set = sql_session_setting_set; } } + +void +sql_setting_set(struct Parse *parse_context, struct Token *name, + struct Expr *expr) +{ + struct Vdbe *vdbe = sqlGetVdbe(parse_context); + if (vdbe == NULL) + goto abort; + sqlVdbeCountChanges(vdbe); + 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); + sqlVdbeAddOp2(vdbe, OP_Set, index, target); + 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..a01c37e19 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 expr(Y). { + sql_setting_set(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..3ffae5970 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 the new value of the session setting. + * + * @param parse_context Parsing context. + * @param name Name of the session setting. + * @param value New value of the session setting. + */ +void +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 620d74e66..c81486fa6 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 @@ -5248,6 +5249,55 @@ case OP_IncMaxid: { break; } +/* 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 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 (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; + uint32_t size = mp_sizeof_str(pIn1->n); + char *mp_value = (char *) static_alloc(size); + if (mp_value == NULL) { + diag_set(OutOfMemory, size, "static_alloc", "mp_value"); + goto abort_due_to_error; + } + mp_encode_str(mp_value, str, pIn1->n); + 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; +} + /* 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 6d7074e8c..7c9802af6 100644 --- a/test/box/session_settings.result +++ b/test/box/session_settings.result @@ -339,6 +339,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 @@ -361,3 +394,19 @@ box.session.settings.sql_default_engine:set(str) | --- | - error: Failed to allocate 20483 bytes in static_alloc for mp_value | ... + +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 23799874a..f2d7e03f5 100644 --- a/test/box/session_settings.test.lua +++ b/test/box/session_settings.test.lua @@ -132,9 +132,22 @@ 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') str = string.rep('a', 20 * 1024) box.session.settings.sql_default_engine:set(str) + +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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-02-17 12:12 ` [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings Chris Sosnin @ 2020-03-16 17:02 ` Nikita Pettik 2020-03-16 22:53 ` Vladislav Shpilevoy 2020-03-17 17:26 ` Chris Sosnin 0 siblings, 2 replies; 21+ messages in thread From: Nikita Pettik @ 2020-03-16 17:02 UTC (permalink / raw) To: Chris Sosnin; +Cc: tarantool-patches, v.shpilevoy On 17 Feb 15:12, Chris Sosnin wrote: > 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. It is not about non-obvious queries, but it is all about documentation: the better feature is described, the clearer its usage turns out to be for user. > Closes #4711 > > @TarantoolBot document > Title: API for accessing _session_settings space. > There are two ways of updating values of session settings: > via Lua and SQL. > > SQL: > Instead of typing long UPDATE query one can use the SET statement: > `box.execute([[SET "<setting_name>" = <new_value>]])`. > Note, that this query is case sensitive so the name must be quoted. > > Example: > ``` > tarantool> box.execute([[set "sql_default_engine" = 'memtx']]) > --- > - row_count: 1 > ... > > tarantool> box.execute([[set "sql_defer_foreign_keys" = true]]) Why didn't you consider show/get command to retrieve setting value? Otherwise, you simplify way to set session local options, but doesn't provide the same way to extract current values. > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index d1fcf4761..3ffae5970 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); > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 620d74e66..c81486fa6 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 > @@ -5248,6 +5249,55 @@ case OP_IncMaxid: { > break; > } > > +/* 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 register > + * holding a value. > + */ > +case OP_Set: { Please, use more specific opcode name. Like OP_SettingSet. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-03-16 17:02 ` Nikita Pettik @ 2020-03-16 22:53 ` Vladislav Shpilevoy 2020-03-17 17:26 ` Chris Sosnin 1 sibling, 0 replies; 21+ messages in thread From: Vladislav Shpilevoy @ 2020-03-16 22:53 UTC (permalink / raw) To: Nikita Pettik, Chris Sosnin; +Cc: tarantool-patches On 16/03/2020 18:02, Nikita Pettik wrote: > On 17 Feb 15:12, Chris Sosnin wrote: >> 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. > > It is not about non-obvious queries, but it is all about documentation: > the better feature is described, the clearer its usage turns out to be > for user. However, on github you voted for this issue, and even agreed that at least for SQL we need a simpler way to update settings. What changed? In my opinion, none of system spaces should ever be allowed to be used by users directly. Especially in such intricate and long way. >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >> index 620d74e66..c81486fa6 100644 >> --- a/src/box/sql/vdbe.c >> +++ b/src/box/sql/vdbe.c >> @@ -5248,6 +5249,55 @@ case OP_IncMaxid: { >> break; >> } >> >> +/* 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 register >> + * holding a value. >> + */ >> +case OP_Set: { > > Please, use more specific opcode name. Like OP_SettingSet. Should not we then change the grammar? Because it requires just SET to change a session setting. Not SET SETTING. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-03-16 17:02 ` Nikita Pettik 2020-03-16 22:53 ` Vladislav Shpilevoy @ 2020-03-17 17:26 ` Chris Sosnin 2020-03-17 20:12 ` Nikita Pettik 1 sibling, 1 reply; 21+ messages in thread From: Chris Sosnin @ 2020-03-17 17:26 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches > On 16 Mar 2020, at 20:02, Nikita Pettik <korablev@tarantool.org> wrote: > > On 17 Feb 15:12, Chris Sosnin wrote: >> 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. > > It is not about non-obvious queries, but it is all about documentation: > the better feature is described, the clearer its usage turns out to be > for user. Should I change the commit message? I though this patchset is about simplifying the way session settings are updated? > >> Closes #4711 >> >> @TarantoolBot document >> Title: API for accessing _session_settings space. >> There are two ways of updating values of session settings: >> via Lua and SQL. >> >> SQL: >> Instead of typing long UPDATE query one can use the SET statement: >> `box.execute([[SET "<setting_name>" = <new_value>]])`. >> Note, that this query is case sensitive so the name must be quoted. >> >> Example: >> ``` >> tarantool> box.execute([[set "sql_default_engine" = 'memtx']]) >> --- >> - row_count: 1 >> ... >> >> tarantool> box.execute([[set "sql_defer_foreign_keys" = true]]) > > Why didn't you consider show/get command to retrieve setting value? > Otherwise, you simplify way to set session local options, but doesn't > provide the same way to extract current values. SELECT * FROM “_session_settings” is simple enough, isn’t it? > >> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h >> index d1fcf4761..3ffae5970 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); >> >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >> index 620d74e66..c81486fa6 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 >> @@ -5248,6 +5249,55 @@ case OP_IncMaxid: { >> break; >> } >> >> +/* 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 register >> + * holding a value. >> + */ >> +case OP_Set: { > > Please, use more specific opcode name. Like OP_SettingSet. Didn’t we accept ’set’ syntax? > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-03-17 17:26 ` Chris Sosnin @ 2020-03-17 20:12 ` Nikita Pettik 2020-03-17 21:00 ` Chris Sosnin 2020-03-18 10:00 ` Chris Sosnin 0 siblings, 2 replies; 21+ messages in thread From: Nikita Pettik @ 2020-03-17 20:12 UTC (permalink / raw) To: Chris Sosnin; +Cc: tarantool-patches On 17 Mar 20:26, Chris Sosnin wrote: > > > > On 16 Mar 2020, at 20:02, Nikita Pettik <korablev@tarantool.org> wrote: > > > > On 17 Feb 15:12, Chris Sosnin wrote: > >> 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. > > > > It is not about non-obvious queries, but it is all about documentation: > > the better feature is described, the clearer its usage turns out to be > > for user. > > Should I change the commit message? I though this patchset is about simplifying > the way session settings are updated? I would say: Currently if a user wants to change session settings via SQL, one has to execute UPDATE query like: [[UPDATE "_session_settings" SET "value" = true WHERE "name" = 'name']] However, direct access to system spaces isn't considered to be good practice. To avoid that and a bit simplify user's life, we introduce SQL shortcut command SET with following syntax: SET <setting_name> = <value>. <setting_name> is supposed to be name of setting (note that it is uppercased as any other non-quoted indentifiers in SQL) and <value> - value of corresponding setting to be set; <value> should be either literal or binding marker. Also it is worth noting that SET doesn't provide any implicit casts, so <value> must be of the type corresponding to the setting being updated. Example: ... ^ It is up to you. > > > >> Closes #4711 > >> > >> @TarantoolBot document > >> Title: API for accessing _session_settings space. > >> There are two ways of updating values of session settings: > >> via Lua and SQL. > >> > >> SQL: > >> Instead of typing long UPDATE query one can use the SET statement: > >> `box.execute([[SET "<setting_name>" = <new_value>]])`. > >> Note, that this query is case sensitive so the name must be quoted. > >> > >> Example: > >> ``` > >> tarantool> box.execute([[set "sql_default_engine" = 'memtx']]) > >> --- > >> - row_count: 1 > >> ... > >> > >> tarantool> box.execute([[set "sql_defer_foreign_keys" = true]]) > > > > Why didn't you consider show/get command to retrieve setting value? > > Otherwise, you simplify way to set session local options, but doesn't > > provide the same way to extract current values. > > SELECT * FROM “_session_settings” is simple enough, isn’t it? In this case you'll get list of all settings. To get specific one we should use this query: SELECT value FROM "_session_settings" WHERE "name" = 'xxx' I do not insist on implementing GET/SHOW SQL syntax now tho. > >> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > >> index d1fcf4761..3ffae5970 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); > >> > >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > >> index 620d74e66..c81486fa6 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 > >> @@ -5248,6 +5249,55 @@ case OP_IncMaxid: { > >> break; > >> } > >> > >> +/* 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 register > >> + * holding a value. > >> + */ > >> +case OP_Set: { > > > > Please, use more specific opcode name. Like OP_SettingSet. > > Didn’t we accept ’set’ syntax? SET is okay, but I don't mind SET SETTING as well. OP_Set is too general name - SET keyword is also part of UPDATE syntax, so it may confuse other developers. Except for this nit patch LGTM. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-03-17 20:12 ` Nikita Pettik @ 2020-03-17 21:00 ` Chris Sosnin 2020-03-18 10:00 ` Chris Sosnin 1 sibling, 0 replies; 21+ messages in thread From: Chris Sosnin @ 2020-03-17 21:00 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 4231 bytes --] Thank you for your answers! > On 17 Mar 2020, at 23:12, Nikita Pettik <korablev@tarantool.org> wrote: > > On 17 Mar 20:26, Chris Sosnin wrote: >> >> >>> On 16 Mar 2020, at 20:02, Nikita Pettik <korablev@tarantool.org> wrote: >>> >>> On 17 Feb 15:12, Chris Sosnin wrote: >>>> 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. >>> >>> It is not about non-obvious queries, but it is all about documentation: >>> the better feature is described, the clearer its usage turns out to be >>> for user. >> >> Should I change the commit message? I though this patchset is about simplifying >> the way session settings are updated? > > I would say: > > Currently if a user wants to change session settings via SQL, one has > to execute UPDATE query like: > [[UPDATE "_session_settings" SET "value" = true WHERE "name" = 'name']] > However, direct access to system spaces isn't considered to be good practice. > To avoid that and a bit simplify user's life, we introduce SQL shortcut command > SET with following syntax: SET <setting_name> = <value>. <setting_name> is > supposed to be name of setting (note that it is uppercased as any other > non-quoted indentifiers in SQL) and <value> - value of corresponding setting to > be set; <value> should be either literal or binding marker. Also it is worth > noting that SET doesn't provide any implicit casts, so <value> must be of the > type corresponding to the setting being updated. > > Example: > ... > > ^ It is up to you. > >>> >>>> Closes #4711 >>>> >>>> @TarantoolBot document >>>> Title: API for accessing _session_settings space. >>>> There are two ways of updating values of session settings: >>>> via Lua and SQL. >>>> >>>> SQL: >>>> Instead of typing long UPDATE query one can use the SET statement: >>>> `box.execute([[SET "<setting_name>" = <new_value>]])`. >>>> Note, that this query is case sensitive so the name must be quoted. >>>> >>>> Example: >>>> ``` >>>> tarantool> box.execute([[set "sql_default_engine" = 'memtx']]) >>>> --- >>>> - row_count: 1 >>>> ... >>>> >>>> tarantool> box.execute([[set "sql_defer_foreign_keys" = true]]) >>> >>> Why didn't you consider show/get command to retrieve setting value? >>> Otherwise, you simplify way to set session local options, but doesn't >>> provide the same way to extract current values. >> >> SELECT * FROM “_session_settings” is simple enough, isn’t it? > > In this case you'll get list of all settings. To get specific one > we should use this query: > > SELECT value FROM "_session_settings" WHERE "name" = 'xxx' > > I do not insist on implementing GET/SHOW SQL syntax now tho. > >>>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h >>>> index d1fcf4761..3ffae5970 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); >>>> >>>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >>>> index 620d74e66..c81486fa6 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 >>>> @@ -5248,6 +5249,55 @@ case OP_IncMaxid: { >>>> break; >>>> } >>>> >>>> +/* 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 register >>>> + * holding a value. >>>> + */ >>>> +case OP_Set: { >>> >>> Please, use more specific opcode name. Like OP_SettingSet. >> >> Didn’t we accept ’set’ syntax? > > SET is okay, but I don't mind SET SETTING as well. OP_Set is too general > name - SET keyword is also part of UPDATE syntax, so it may confuse other > developers. Except for this nit patch LGTM. I don’t mind this changes, I will work on them tomorrow. [-- Attachment #2: Type: text/html, Size: 26502 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-03-17 20:12 ` Nikita Pettik 2020-03-17 21:00 ` Chris Sosnin @ 2020-03-18 10:00 ` Chris Sosnin 1 sibling, 0 replies; 21+ messages in thread From: Chris Sosnin @ 2020-03-18 10:00 UTC (permalink / raw) To: Nikita Pettik, Vladislav Shpilevoy; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 5352 bytes --] > On 17 Mar 2020, at 23:12, Nikita Pettik <korablev@tarantool.org> wrote: > > On 17 Mar 20:26, Chris Sosnin wrote: >> >> >>> On 16 Mar 2020, at 20:02, Nikita Pettik <korablev@tarantool.org> wrote: >>> >>> On 17 Feb 15:12, Chris Sosnin wrote: >>>> 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. >>> >>> It is not about non-obvious queries, but it is all about documentation: >>> the better feature is described, the clearer its usage turns out to be >>> for user. >> >> Should I change the commit message? I though this patchset is about simplifying >> the way session settings are updated? > > I would say: > > Currently if a user wants to change session settings via SQL, one has > to execute UPDATE query like: > [[UPDATE "_session_settings" SET "value" = true WHERE "name" = 'name']] > However, direct access to system spaces isn't considered to be good practice. > To avoid that and a bit simplify user's life, we introduce SQL shortcut command > SET with following syntax: SET <setting_name> = <value>. <setting_name> is > supposed to be name of setting (note that it is uppercased as any other > non-quoted indentifiers in SQL) and <value> - value of corresponding setting to > be set; <value> should be either literal or binding marker. Also it is worth > noting that SET doesn't provide any implicit casts, so <value> must be of the > type corresponding to the setting being updated. > > Example: > ... > > ^ It is up to you. I took the beginning as long as the rest is described in the doc request (except for implicit casts note, thanks for pointing that out, fixed): Currently if a user wants to change session setting with SQL, one has to execute UPDATE query like: [[UPDATE "_session_settings" SET "value" = true WHERE "name" = 'name']] However, direct access to system spaces isn't considered to be a good practice. To avoid that and a bit simplify user's life, we introduce SQL shortcut command SETTING SET. > >>> >>>> Closes #4711 >>>> >>>> @TarantoolBot document >>>> Title: API for accessing _session_settings space. >>>> There are two ways of updating values of session settings: >>>> via Lua and SQL. >>>> >>>> SQL: >>>> Instead of typing long UPDATE query one can use the SET statement: >>>> `box.execute([[SET "<setting_name>" = <new_value>]])`. >>>> Note, that this query is case sensitive so the name must be quoted. >>>> >>>> Example: >>>> ``` >>>> tarantool> box.execute([[set "sql_default_engine" = 'memtx']]) >>>> --- >>>> - row_count: 1 >>>> ... >>>> >>>> tarantool> box.execute([[set "sql_defer_foreign_keys" = true]]) >>> >>> Why didn't you consider show/get command to retrieve setting value? >>> Otherwise, you simplify way to set session local options, but doesn't >>> provide the same way to extract current values. >> >> SELECT * FROM “_session_settings” is simple enough, isn’t it? > > In this case you'll get list of all settings. To get specific one > we should use this query: > > SELECT value FROM "_session_settings" WHERE "name" = 'xxx' > > I do not insist on implementing GET/SHOW SQL syntax now tho. > >>>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h >>>> index d1fcf4761..3ffae5970 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); >>>> >>>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >>>> index 620d74e66..c81486fa6 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 >>>> @@ -5248,6 +5249,55 @@ case OP_IncMaxid: { >>>> break; >>>> } >>>> >>>> +/* 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 register >>>> + * holding a value. >>>> + */ >>>> +case OP_Set: { >>> >>> Please, use more specific opcode name. Like OP_SettingSet. >> >> Didn’t we accept ’set’ syntax? > > SET is okay, but I don't mind SET SETTING as well. OP_Set is too general > name - SET keyword is also part of UPDATE syntax, so it may confuse other > developers. Except for this nit patch LGTM. Here are the fixes: - sqlVdbeAddOp2(vdbe, OP_Set, index, target); + sqlVdbeAddOp2(vdbe, OP_SetSetting, index, target); -///////////////////////////// The SET command //////////////////////////////// -cmd ::= SET nm(X) EQ expr(Y). { +///////////////////////////// The SETTING SET command //////////////////////// +cmd ::= SETTING SET nm(X) EQ expr(Y). { -/* Opcode: Set P1 P2 * * * +/* Opcode: SetSetting P1 P2 * * * -case OP_Set: { +case OP_SetSetting: { And of course I had to change the grammar and tests. Changes are on the branch with the second version of Lua patch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-session-settings-v2 [-- Attachment #2: Type: text/html, Size: 28033 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* [Tarantool-patches] [PATCH 0/4] session settings fixes
@ 2020-03-30 9:13 Chris Sosnin
2020-03-30 9:13 ` [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings Chris Sosnin
0 siblings, 1 reply; 21+ messages in thread
From: Chris Sosnin @ 2020-03-30 9:13 UTC (permalink / raw)
To: v.shpilevoy, korablev, tarantool-patches
issue #1:https://github.com/tarantool/tarantool/issues/4711
issue #2:https://github.com/tarantool/tarantool/issues/4712
branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-session-settings-v2
Chris Sosnin (4):
box: replace session_settings modules with a single array
box: add binary search for _session_settings space
box: provide a user friendly frontend for accessing session settings
sql: provide a user friendly frontend for accessing session settings
extra/mkkeywordhash.c | 1 +
src/box/lua/session.c | 111 +++++++++
src/box/session.cc | 1 +
src/box/session.h | 2 +
src/box/session_settings.c | 214 +++++++++++-------
src/box/session_settings.h | 53 +++--
src/box/sql.c | 5 -
src/box/sql/build.c | 104 ++++-----
src/box/sql/parse.y | 5 +
src/box/sql/sqlInt.h | 11 +
src/box/sql/vdbe.c | 50 ++++
...rontend.result => session_settings.result} | 147 ++++++++++--
...end.test.lua => session_settings.test.lua} | 61 ++++-
13 files changed, 589 insertions(+), 176 deletions(-)
rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (71%)
rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (64%)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 21+ messages in thread* [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-03-30 9:13 [Tarantool-patches] [PATCH 0/4] session settings fixes Chris Sosnin @ 2020-03-30 9:13 ` Chris Sosnin 2020-04-03 15:19 ` Nikita Pettik 2020-04-04 21:56 ` Vladislav Shpilevoy 0 siblings, 2 replies; 21+ messages in thread From: Chris Sosnin @ 2020-03-30 9:13 UTC (permalink / raw) To: v.shpilevoy, korablev, tarantool-patches Currently if a user wants to change session setting with SQL, one has to execute UPDATE query like: [[UPDATE "_session_settings" SET "value" = true WHERE "name" = 'name']] However, direct access to system spaces isn't considered to be a good practice. To avoid that and a bit simplify user's life, we introduce SQL shortcut command SETTING SET. Closes #4711 @TarantoolBot document Title: API for accessing _session_settings space. There are two ways of updating values of session settings: via Lua and SQL. Lua: box.session.settings is a table, which is always accessible to user. The syntax is the following: `box.session.settings.<setting_name> = <new_value>`. Example of usage: ``` tarantool> box.session.settings.sql_default_engine --- - memtx ... tarantool> box.session.settings.sql_default_engine = 'vinyl' --- ... ``` The table itself represents the (unordered) result of select from _session_settings space. SQL: Instead of typing long UPDATE query one can use the SETTING SET command: `box.execute([[SETTING SET "<setting_name>" = <new_value>]])`. Note, that this query is case sensitive so the name must be quoted. Also, SETTING SET doesn't prove any implicit casts, so <new_value> must be of the type corresponding to the setting being updated. Example: ``` tarantool> box.execute([[setting set "sql_default_engine" = 'memtx']]) --- - row_count: 1 ... tarantool> box.execute([[setting set "sql_defer_foreign_keys" = true]]) --- - row_count: 1 ... ``` --- extra/mkkeywordhash.c | 1 + src/box/sql/build.c | 25 +++++++++++++++ src/box/sql/parse.y | 5 +++ src/box/sql/sqlInt.h | 11 +++++++ src/box/sql/vdbe.c | 50 ++++++++++++++++++++++++++++++ test/box/session_settings.result | 49 +++++++++++++++++++++++++++++ test/box/session_settings.test.lua | 13 ++++++++ 7 files changed, 154 insertions(+) diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c index 006285622..887853529 100644 --- a/extra/mkkeywordhash.c +++ b/extra/mkkeywordhash.c @@ -159,6 +159,7 @@ static Keyword aKeywordTable[] = { { "SCALAR", "TK_SCALAR", true }, { "SELECT", "TK_SELECT", true }, { "SET", "TK_SET", true }, + { "SETTING", "TK_SETTING", false }, { "SIMPLE", "TK_SIMPLE", true }, { "START", "TK_START", true }, { "STRING", "TK_STRING_KW", true }, diff --git a/src/box/sql/build.c b/src/box/sql/build.c index a00da31f9..cdcf8b6d8 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -3481,3 +3481,28 @@ sql_session_settings_init() setting->set = sql_session_setting_set; } } + +void +sql_setting_set(struct Parse *parse_context, struct Token *name, + struct Expr *expr) +{ + struct Vdbe *vdbe = sqlGetVdbe(parse_context); + if (vdbe == NULL) + goto abort; + sqlVdbeCountChanges(vdbe); + 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); + sqlVdbeAddOp2(vdbe, OP_SetSetting, index, target); + 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 1a0e89703..56977b003 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 SETTING SET command //////////////////////// +cmd ::= SETTING SET nm(X) EQ expr(Y). { + sql_setting_set(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 1579cc92e..a700dd9c9 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -4513,4 +4513,15 @@ int sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name, uint32_t *fieldno); +/** + * Create VDBE instructions to set the new value of the session setting. + * + * @param parse_context Parsing context. + * @param name Name of the session setting. + * @param value New value of the session setting. + */ +void +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 e8a029a8a..d6b753a51 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 @@ -5252,6 +5253,55 @@ case OP_IncMaxid: { break; } +/* Opcode: SetSetting 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 register + * holding a value. + */ +case OP_SetSetting: { + int sid = pOp->p1; + pIn1 = &aMem[pOp->p2]; + assert(sid >= 0 && sid < SESSION_SETTING_COUNT); + struct session_setting *setting = &session_settings[sid]; + 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; + uint32_t size = mp_sizeof_str(pIn1->n); + char *mp_value = (char *) static_alloc(size); + if (mp_value == NULL) { + diag_set(OutOfMemory, size, "static_alloc", "mp_value"); + goto abort_due_to_error; + } + mp_encode_str(mp_value, str, pIn1->n); + 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; +} + /* 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 b32a0becb..28c79379b 100644 --- a/test/box/session_settings.result +++ b/test/box/session_settings.result @@ -339,6 +339,39 @@ settings.sql_defer_foreign_keys | - false | ... +box.execute([[setting set "sql_default_engine" = 'vinyl']]) + | --- + | - row_count: 1 + | ... +s:get('sql_default_engine').value + | --- + | - vinyl + | ... +box.execute([[setting set "sql_default_engine" = 'memtx']]) + | --- + | - row_count: 1 + | ... +s:get('sql_default_engine').value + | --- + | - memtx + | ... +box.execute([[setting set "sql_defer_foreign_keys" = true]]) + | --- + | - row_count: 1 + | ... +s:get('sql_defer_foreign_keys').value + | --- + | - true + | ... +box.execute([[setting set "sql_defer_foreign_keys" = ?]], {false}) + | --- + | - row_count: 1 + | ... +s:get('sql_defer_foreign_keys').value + | --- + | - false + | ... + settings.sql_default_engine = true | --- | - error: Session setting sql_default_engine expected a value of type string @@ -359,3 +392,19 @@ box.session.settings.sql_default_engine = str | --- | - error: Failed to allocate 20483 bytes in static_alloc for mp_value | ... + +box.execute([[setting set "sql_def_engine" = true]]) + | --- + | - null + | - Session setting sql_def_engine doesn't exist + | ... +box.execute([[setting set "sql_default_engine" = true]]) + | --- + | - null + | - Session setting sql_default_engine expected a value of type string + | ... +box.execute([[setting 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 440bef7ce..c477139f9 100644 --- a/test/box/session_settings.test.lua +++ b/test/box/session_settings.test.lua @@ -132,9 +132,22 @@ s:get('sql_defer_foreign_keys').value s:update('sql_defer_foreign_keys', {{'=', 2, false}}) settings.sql_defer_foreign_keys +box.execute([[setting set "sql_default_engine" = 'vinyl']]) +s:get('sql_default_engine').value +box.execute([[setting set "sql_default_engine" = 'memtx']]) +s:get('sql_default_engine').value +box.execute([[setting set "sql_defer_foreign_keys" = true]]) +s:get('sql_defer_foreign_keys').value +box.execute([[setting set "sql_defer_foreign_keys" = ?]], {false}) +s:get('sql_defer_foreign_keys').value + settings.sql_default_engine = true settings.sql_defer_foreign_keys = 'false' settings.sql_parser_debug = 'string' str = string.rep('a', 20 * 1024) box.session.settings.sql_default_engine = str + +box.execute([[setting set "sql_def_engine" = true]]) +box.execute([[setting set "sql_default_engine" = true]]) +box.execute([[setting set "sql_defer_foreign_keys" = 'true']]) -- 2.21.1 (Apple Git-122.3) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-03-30 9:13 ` [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings Chris Sosnin @ 2020-04-03 15:19 ` Nikita Pettik 2020-04-04 21:56 ` Vladislav Shpilevoy 1 sibling, 0 replies; 21+ messages in thread From: Nikita Pettik @ 2020-04-03 15:19 UTC (permalink / raw) To: Chris Sosnin; +Cc: tarantool-patches, v.shpilevoy On 30 Mar 12:13, Chris Sosnin wrote: > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index a00da31f9..cdcf8b6d8 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -3481,3 +3481,28 @@ sql_session_settings_init() > setting->set = sql_session_setting_set; > } > } > + > +void > +sql_setting_set(struct Parse *parse_context, struct Token *name, > + struct Expr *expr) > +{ > + struct Vdbe *vdbe = sqlGetVdbe(parse_context); > + if (vdbe == NULL) > + goto abort; > + sqlVdbeCountChanges(vdbe); > + 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); > + sqlVdbeAddOp2(vdbe, OP_SetSetting, index, target); > + return; > + } > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, > + tt_sprintf("Session setting %s doesn't exist", key)); Mb it is worth moving this check to runtime? I.e. pass to OP_SetSetting name of setting to be set. The less appeals to system spaces and cache look ups are made, the better parsing process is organized. The rest is ok. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-03-30 9:13 ` [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings Chris Sosnin 2020-04-03 15:19 ` Nikita Pettik @ 2020-04-04 21:56 ` Vladislav Shpilevoy 2020-04-10 15:40 ` Chris Sosnin 1 sibling, 1 reply; 21+ messages in thread From: Vladislav Shpilevoy @ 2020-04-04 21:56 UTC (permalink / raw) To: Chris Sosnin, korablev, tarantool-patches Hi! Thanks for the patch! > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index a00da31f9..cdcf8b6d8 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -3481,3 +3481,28 @@ sql_session_settings_init() > setting->set = sql_session_setting_set; > } > } > + > +void > +sql_setting_set(struct Parse *parse_context, struct Token *name, > + struct Expr *expr) > +{ > + struct Vdbe *vdbe = sqlGetVdbe(parse_context); > + if (vdbe == NULL) > + goto abort; > + sqlVdbeCountChanges(vdbe); > + char *key = sql_name_from_token(parse_context->db, name); > + if (key == NULL) > + goto abort; > + int index = session_setting_find(key); I agree with Nikita. Better make this call part of OP_SetSetting. > + if (index >= 0) { > + int target = ++parse_context->nMem; > + sqlExprCode(parse_context, expr, target); > + sqlVdbeAddOp2(vdbe, OP_SetSetting, index, target); > + return; > + } > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, > + tt_sprintf("Session setting %s doesn't exist", key)); > +abort: > + parse_context->is_aborted = true; > + return; > +} Talking of the syntax, I don't have a strong opinion here regarding any particular option. I only want the new statement be short, and not involving _session_settings manipulations. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-04-04 21:56 ` Vladislav Shpilevoy @ 2020-04-10 15:40 ` Chris Sosnin 2020-04-11 17:18 ` Vladislav Shpilevoy 2020-04-13 7:50 ` Timur Safin 0 siblings, 2 replies; 21+ messages in thread From: Chris Sosnin @ 2020-04-10 15:40 UTC (permalink / raw) To: v.shpilevoy; +Cc: tarantool-patches > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > > index a00da31f9..cdcf8b6d8 100644 > > --- a/src/box/sql/build.c > > +++ b/src/box/sql/build.c > > @@ -3481,3 +3481,28 @@ sql_session_settings_init() > > setting->set = sql_session_setting_set; > > } > > } > > + > > +void > > +sql_setting_set(struct Parse *parse_context, struct Token *name, > > + struct Expr *expr) > > +{ > > + struct Vdbe *vdbe = sqlGetVdbe(parse_context); > > + if (vdbe == NULL) > > + goto abort; > > + sqlVdbeCountChanges(vdbe); > > + char *key = sql_name_from_token(parse_context->db, name); > > + if (key == NULL) > > + goto abort; > > + int index = session_setting_find(key); > > I agree with Nikita. Better make this call part of OP_SetSetting. I agree, fixed: + +void +sql_setting_set(struct Parse *parse_context, struct Token *name, + struct Expr *expr) +{ + struct Vdbe *vdbe = sqlGetVdbe(parse_context); + if (vdbe == NULL) + goto abort; + sqlVdbeCountChanges(vdbe); + char *key = sql_name_from_token(parse_context->db, name); + if (key == NULL) + goto abort; + int target = ++parse_context->nMem; + sqlExprCode(parse_context, expr, target); + sqlVdbeAddOp4(vdbe, OP_SetSession, target, 0, 0, key, P4_DYNAMIC); + return; +abort: + parse_context->is_aborted = true; + return; +} +/* Opcode: SetSession P1 * * P4 * + * + * Set new value of the session setting. P4 is the name of the + * setting being updated, P1 is the register holding a value. + */ +case OP_SetSession: { + assert(pOp->p4type == P4_DYNAMIC); + const char *setting_name = pOp->p4.z; + int sid = session_setting_find(setting_name); + if (sid < 0) { + diag_set(ClientError, ER_NO_SUCH_SESSION_SETTING, setting_name); + goto abort_due_to_error; + } + pIn1 = &aMem[pOp->p1]; ... > Talking of the syntax, I don't have a strong opinion here > regarding any particular option. I only want the new statement > be short, and not involving _session_settings manipulations. I changed it to the following (the main problem with reserved word persists): +///////////////////////////// The SET SESSION command //////////////////////// +// +cmd ::= SET SESSION nm(X) EQ term(Y). { + sql_setting_set(pParse,&X,Y.pExpr); +} + I also changed the 3rd commit (Lua frontend) in the series to introduce new error code (it's present in the diff above): + /*213 */_(ER_NO_SUCH_SESSION_SETTING, "Session setting %s doesn't exist") \ as long as searching is moved to vdbe.c and raising parsing error is not correct. You can see the whole patch below: =============================================================================== Currently if a user wants to change session setting with SQL, one has to execute UPDATE query like: [[UPDATE "_session_settings" SET "value" = true WHERE "name" = 'name']] However, direct access to system spaces isn't considered to be a good practice. To avoid that and a bit simplify user's life, we introduce SQL shortcut command SET SESSION. Closes #4711 @TarantoolBot document Title: API for accessing _session_settings space. There are two ways of updating values of session settings: via Lua and SQL. Lua: box.session.settings is a table, which is always accessible to user. The syntax is the following: `box.session.settings.<setting_name> = <new_value>`. Example of usage: ``` tarantool> box.session.settings.sql_default_engine --- - memtx ... tarantool> box.session.settings.sql_default_engine = 'vinyl' --- ... ``` The table itself represents the (unordered) result of select from _session_settings space. SQL: Instead of typing long UPDATE query one can use the SET SESSION command: `box.execute([[SET SESSION "<setting_name>" = <new_value>]])`. Note, that this query is case sensitive so the name must be quoted. Also, SET SESSION doesn't provide any implicit casts, so <new_value> must be of the type corresponding to the setting being updated. Example: ``` tarantool> box.execute([[set session "sql_default_engine" = 'memtx']]) --- - row_count: 1 ... tarantool> box.execute([[set session "sql_defer_foreign_keys" = true]]) --- - row_count: 1 ... ``` --- extra/mkkeywordhash.c | 1 + src/box/sql/build.c | 20 +++++++++++ src/box/sql/parse.y | 6 ++++ src/box/sql/sqlInt.h | 11 ++++++ src/box/sql/vdbe.c | 54 ++++++++++++++++++++++++++++++ test/box/session_settings.result | 49 +++++++++++++++++++++++++++ test/box/session_settings.test.lua | 13 +++++++ 7 files changed, 154 insertions(+) diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c index 006285622..dd42c8f5f 100644 --- a/extra/mkkeywordhash.c +++ b/extra/mkkeywordhash.c @@ -158,6 +158,7 @@ static Keyword aKeywordTable[] = { { "SAVEPOINT", "TK_SAVEPOINT", true }, { "SCALAR", "TK_SCALAR", true }, { "SELECT", "TK_SELECT", true }, + { "SESSION", "TK_SESSION", false }, { "SET", "TK_SET", true }, { "SIMPLE", "TK_SIMPLE", true }, { "START", "TK_START", true }, diff --git a/src/box/sql/build.c b/src/box/sql/build.c index a00da31f9..aecba4177 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -3481,3 +3481,23 @@ sql_session_settings_init() setting->set = sql_session_setting_set; } } + +void +sql_setting_set(struct Parse *parse_context, struct Token *name, + struct Expr *expr) +{ + struct Vdbe *vdbe = sqlGetVdbe(parse_context); + if (vdbe == NULL) + goto abort; + sqlVdbeCountChanges(vdbe); + char *key = sql_name_from_token(parse_context->db, name); + if (key == NULL) + goto abort; + int target = ++parse_context->nMem; + sqlExprCode(parse_context, expr, target); + sqlVdbeAddOp4(vdbe, OP_SetSession, target, 0, 0, key, P4_DYNAMIC); + return; +abort: + parse_context->is_aborted = true; + return; +} diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 1a0e89703..995875566 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -1541,6 +1541,12 @@ cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y). { sql_drop_index(pParse); } +///////////////////////////// The SET SESSION command //////////////////////// +// +cmd ::= SET SESSION nm(X) EQ term(Y). { + sql_setting_set(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 2d6bad424..e45a7671d 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -4517,4 +4517,15 @@ int sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name, uint32_t *fieldno); +/** + * Create VDBE instructions to set the new value of the session setting. + * + * @param parse_context Parsing context. + * @param name Name of the session setting. + * @param value New value of the session setting. + */ +void +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 e8a029a8a..c3256c15a 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 @@ -5252,6 +5253,59 @@ case OP_IncMaxid: { break; } +/* Opcode: SetSession P1 * * P4 * + * + * Set new value of the session setting. P4 is the name of the + * setting being updated, P1 is the register holding a value. + */ +case OP_SetSession: { + assert(pOp->p4type == P4_DYNAMIC); + const char *setting_name = pOp->p4.z; + int sid = session_setting_find(setting_name); + if (sid < 0) { + diag_set(ClientError, ER_NO_SUCH_SESSION_SETTING, setting_name); + goto abort_due_to_error; + } + pIn1 = &aMem[pOp->p1]; + struct session_setting *setting = &session_settings[sid]; + 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; + uint32_t size = mp_sizeof_str(pIn1->n); + char *mp_value = (char *) static_alloc(size); + if (mp_value == NULL) { + diag_set(OutOfMemory, size, "static_alloc", "mp_value"); + goto abort_due_to_error; + } + mp_encode_str(mp_value, str, pIn1->n); + 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; +} + /* 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 b32a0becb..b82bdda94 100644 --- a/test/box/session_settings.result +++ b/test/box/session_settings.result @@ -339,6 +339,39 @@ settings.sql_defer_foreign_keys | - false | ... +box.execute([[set session "sql_default_engine" = 'vinyl']]) + | --- + | - row_count: 1 + | ... +s:get('sql_default_engine').value + | --- + | - vinyl + | ... +box.execute([[set session "sql_default_engine" = 'memtx']]) + | --- + | - row_count: 1 + | ... +s:get('sql_default_engine').value + | --- + | - memtx + | ... +box.execute([[set session "sql_defer_foreign_keys" = true]]) + | --- + | - row_count: 1 + | ... +s:get('sql_defer_foreign_keys').value + | --- + | - true + | ... +box.execute([[set session "sql_defer_foreign_keys" = false]]) + | --- + | - row_count: 1 + | ... +s:get('sql_defer_foreign_keys').value + | --- + | - false + | ... + settings.sql_default_engine = true | --- | - error: Session setting sql_default_engine expected a value of type string @@ -359,3 +392,19 @@ box.session.settings.sql_default_engine = str | --- | - error: Failed to allocate 20483 bytes in static_alloc for mp_value | ... + +box.execute([[set session "sql_def_engine" = true]]) + | --- + | - null + | - Session setting sql_def_engine doesn't exist + | ... +box.execute([[set session "sql_default_engine" = true]]) + | --- + | - null + | - Session setting sql_default_engine expected a value of type string + | ... +box.execute([[set session "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 440bef7ce..d4aac0286 100644 --- a/test/box/session_settings.test.lua +++ b/test/box/session_settings.test.lua @@ -132,9 +132,22 @@ s:get('sql_defer_foreign_keys').value s:update('sql_defer_foreign_keys', {{'=', 2, false}}) settings.sql_defer_foreign_keys +box.execute([[set session "sql_default_engine" = 'vinyl']]) +s:get('sql_default_engine').value +box.execute([[set session "sql_default_engine" = 'memtx']]) +s:get('sql_default_engine').value +box.execute([[set session "sql_defer_foreign_keys" = true]]) +s:get('sql_defer_foreign_keys').value +box.execute([[set session "sql_defer_foreign_keys" = false]]) +s:get('sql_defer_foreign_keys').value + settings.sql_default_engine = true settings.sql_defer_foreign_keys = 'false' settings.sql_parser_debug = 'string' str = string.rep('a', 20 * 1024) box.session.settings.sql_default_engine = str + +box.execute([[set session "sql_def_engine" = true]]) +box.execute([[set session "sql_default_engine" = true]]) +box.execute([[set session "sql_defer_foreign_keys" = 'true']]) -- 2.21.1 (Apple Git-122.3) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-04-10 15:40 ` Chris Sosnin @ 2020-04-11 17:18 ` Vladislav Shpilevoy 2020-04-13 7:50 ` Timur Safin 1 sibling, 0 replies; 21+ messages in thread From: Vladislav Shpilevoy @ 2020-04-11 17:18 UTC (permalink / raw) To: Chris Sosnin; +Cc: tarantool-patches Hi! Thanks for the patchset! I force pushed the style fix below. LGTM now. diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 995875566..380fb83d1 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -1544,7 +1544,7 @@ cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y). { ///////////////////////////// The SET SESSION command //////////////////////// // cmd ::= SET SESSION nm(X) EQ term(Y). { - sql_setting_set(pParse,&X,Y.pExpr); + sql_setting_set(pParse, &X, Y.pExpr); } ///////////////////////////// The PRAGMA command ///////////////////////////// ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings 2020-04-10 15:40 ` Chris Sosnin 2020-04-11 17:18 ` Vladislav Shpilevoy @ 2020-04-13 7:50 ` Timur Safin 1 sibling, 0 replies; 21+ messages in thread From: Timur Safin @ 2020-04-13 7:50 UTC (permalink / raw) To: 'Chris Sosnin', v.shpilevoy; +Cc: tarantool-patches : : I changed it to the following (the main problem with reserved word : persists): : : +///////////////////////////// The SET SESSION command : //////////////////////// : +// : +cmd ::= SET SESSION nm(X) EQ term(Y). { : + sql_setting_set(pParse,&X,Y.pExpr); : +} : + : .. : ========================================================================== : ===== : : Currently if a user wants to change session setting with SQL, one has : to execute UPDATE query like: : [[UPDATE "_session_settings" SET "value" = true WHERE "name" = 'name']] : However, direct access to system spaces isn't considered to be a good : practice. : To avoid that and a bit simplify user's life, we introduce SQL shortcut : command : SET SESSION. : : Closes #4711 : : @TarantoolBot document : Title: API for accessing _session_settings space. : There are two ways of updating values of session settings: : via Lua and SQL. : ... : SQL: : Instead of typing long UPDATE query one can use the SET SESSION command: : `box.execute([[SET SESSION "<setting_name>" = <new_value>]])`. : Note, that this query is case sensitive so the name must be quoted. : Also, SET SESSION doesn't provide any implicit casts, so <new_value> must : be of the type corresponding to the setting being updated. : : Example: : ``` : tarantool> box.execute([[set session "sql_default_engine" = 'memtx']]) : --- : - row_count: 1 : ... : : tarantool> box.execute([[set session "sql_defer_foreign_keys" = true]]) : --- : - row_count: 1 : ... : Like it! (The least intrusive way possible, but still is short for user to write) Timur ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2020-04-13 7:50 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-30 11:10 [Tarantool-patches] [PATCH] sql: provide a user friendly frontend for accessing session settings Chris Sosnin 2020-02-03 22:17 ` Vladislav Shpilevoy 2020-02-04 19:32 ` [Tarantool-patches] [PATCH 4/4] " Chris Sosnin 2020-02-06 22:16 ` Vladislav Shpilevoy 2020-02-07 9:40 ` Chris Sosnin 2020-02-10 22:09 ` Vladislav Shpilevoy 2020-02-17 11:46 ` Chris Sosnin 2020-02-17 11:56 ` Nikita Pettik 2020-02-17 12:12 [Tarantool-patches] [PATCH 0/4] box: session settings fixes Chris Sosnin 2020-02-17 12:12 ` [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings Chris Sosnin 2020-03-16 17:02 ` Nikita Pettik 2020-03-16 22:53 ` Vladislav Shpilevoy 2020-03-17 17:26 ` Chris Sosnin 2020-03-17 20:12 ` Nikita Pettik 2020-03-17 21:00 ` Chris Sosnin 2020-03-18 10:00 ` Chris Sosnin 2020-03-30 9:13 [Tarantool-patches] [PATCH 0/4] session settings fixes Chris Sosnin 2020-03-30 9:13 ` [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings Chris Sosnin 2020-04-03 15:19 ` Nikita Pettik 2020-04-04 21:56 ` Vladislav Shpilevoy 2020-04-10 15:40 ` Chris Sosnin 2020-04-11 17:18 ` Vladislav Shpilevoy 2020-04-13 7:50 ` Timur Safin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox