From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <korablev@tarantool.org>
Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96])
 (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 84598469719
 for <tarantool-patches@dev.tarantool.org>;
 Mon, 16 Mar 2020 20:02:13 +0300 (MSK)
Date: Mon, 16 Mar 2020 17:02:12 +0000
From: Nikita Pettik <korablev@tarantool.org>
Message-ID: <20200316170212.GH14628@tarantool.org>
References: <cover.1581940899.git.k.sosnin@tarantool.org>
 <36341694915f89597f2ac938077d0d10bcad0448.1581940900.git.k.sosnin@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <36341694915f89597f2ac938077d0d10bcad0448.1581940900.git.k.sosnin@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly
 frontend for accessing session settings
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Chris Sosnin <k.sosnin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org

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.