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

[Nikita Pettik <korablev@tarantool.org>]

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.