From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B34174696C3 for ; Fri, 10 Apr 2020 18:40:05 +0300 (MSK) From: Chris Sosnin Date: Fri, 10 Apr 2020 18:40:03 +0300 Message-Id: <20200410154003.23687-1-k.sosnin@tarantool.org> In-Reply-To: <736e6bf5-aea4-ffe4-be7b-2dcc9539065a@tarantool.org> References: <736e6bf5-aea4-ffe4-be7b-2dcc9539065a@tarantool.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: v.shpilevoy@tarantool.org Cc: tarantool-patches@dev.tarantool.org > > 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. = `. 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 "" = ]])`. Note, that this query is case sensitive so the name must be quoted. Also, SET SESSION doesn't provide any implicit casts, so 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)