From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 A29BD469719 for ; Tue, 17 Mar 2020 23:12:33 +0300 (MSK) Date: Tue, 17 Mar 2020 20:12:32 +0000 From: Nikita Pettik Message-ID: <20200317201232.GA4294@tarantool.org> References: <36341694915f89597f2ac938077d0d10bcad0448.1581940900.git.k.sosnin@tarantool.org> <20200316170212.GH14628@tarantool.org> <6B7EB5E1-B936-46F4-AB4F-28EEDB72408A@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <6B7EB5E1-B936-46F4-AB4F-28EEDB72408A@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chris Sosnin Cc: tarantool-patches@dev.tarantool.org 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.