From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34])
 (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 E8A24469719
 for <tarantool-patches@dev.tarantool.org>;
 Tue, 17 Mar 2020 01:53:35 +0300 (MSK)
References: <cover.1581940899.git.k.sosnin@tarantool.org>
 <36341694915f89597f2ac938077d0d10bcad0448.1581940900.git.k.sosnin@tarantool.org>
 <20200316170212.GH14628@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <add90e62-9347-8f37-7ebc-f363d55a56c3@tarantool.org>
Date: Mon, 16 Mar 2020 23:53:34 +0100
MIME-Version: 1.0
In-Reply-To: <20200316170212.GH14628@tarantool.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
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: Nikita Pettik <korablev@tarantool.org>, Chris Sosnin <k.sosnin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

On 16/03/2020 18: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.

However, on github you voted for this issue, and even agreed that at
least for SQL we need a simpler way to update settings. What changed?

In my opinion, none of system spaces should ever be allowed to be used
by users directly. Especially in such intricate and long way.

>> 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
>> @@ -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.

Should not we then change the grammar? Because it requires just SET to
change a session setting. Not SET SETTING.