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

[Chris Sosnin <k.sosnin@tarantool.org>]

> 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?

>