Thank you for your answers! > On 17 Mar 2020, at 23:12, Nikita Pettik wrote: > > On 17 Mar 20:26, Chris Sosnin wrote: >> >> >>> On 16 Mar 2020, at 20: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. >> >> 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 = . is > supposed to be name of setting (note that it is uppercased as any other > non-quoted indentifiers in SQL) and - value of corresponding setting to > be set; should be either literal or binding marker. Also it is worth > noting that SET doesn't provide any implicit casts, so 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 "" = ]])`. >>>> 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.