From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp31.i.mail.ru (smtp31.i.mail.ru [94.100.177.91]) (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 9579C46970E for ; Tue, 4 Feb 2020 01:17:19 +0300 (MSK) References: <20200130111009.35857-1-k.sosnin@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Mon, 3 Feb 2020 23:17:17 +0100 MIME-Version: 1.0 In-Reply-To: <20200130111009.35857-1-k.sosnin@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] 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 , tarantool-patches@dev.tarantool.org Thanks for the patch! See 4 comments below. On 30/01/2020 12:10, 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. > > Part of #4711 > --- > This patch is a follow-up for session settings patchset. > > issue:https://github.com/tarantool/tarantool/issues/4711 > branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings > > src/box/sql/build.c | 31 ++++++++++++ > src/box/sql/parse.y | 5 ++ > src/box/sql/sqlInt.h | 11 +++++ > src/box/sql/vdbe.c | 45 +++++++++++++++++ > ...1-access-settings-from-any-frontend.result | 49 +++++++++++++++++++ > ...access-settings-from-any-frontend.test.lua | 13 +++++ > 6 files changed, 154 insertions(+) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 7dcf7b858..cb7733cfc 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -3474,3 +3474,34 @@ sql_session_settings_init() > setting->set = sql_session_setting_set; > } > } > + > +void > +sql_set_setting(struct Parse *parse_context, struct Token *name, > + struct Expr *value) > +{ > + struct Vdbe *vdbe = sqlGetVdbe(parse_context); > + assert(vdbe != NULL); > + sqlVdbeCountChanges(vdbe); > + char *key = sql_name_from_token(sql_get(), name); > + if (key == NULL) > + goto abort; > + int low = 0, high = session_setting_MAX - 1; > + while (low <= high) { > + int index = (high + low) / 2; > + int cmp = strcasecmp(session_settings[index].name, key); 1. All other places in SQL are case sensitive. I don't think this place should be an exception. > + if (cmp == 0) { > + sqlVdbeAddOp4(vdbe, OP_Set, index, 0, 0, > + (const char *)value, P4_PTR); 2. 1) struct Expr is a parsing stage object, it should not exist during execution, 2) it is generally a bad practice to save an object by a pointer, because that makes VDBE harder to serialize in future, when we will expropriate VDBE generation into a library. You should encode the expression as a set of VDBE operations storing result into a register, from where SET would read it. Ideally, it would support '?' parameters. See sqlExprCode(). > + return; > + } > + if (cmp < 0) > + low = index + 1; > + else > + high = index - 1; > + } 3. Well, why not to expose session_settings_set_forward() into session_settings.h header and use it here? > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, > + tt_sprintf("Session setting %s doesn't exist", key)); > +abort: > + parse_context->is_aborted = true; > + return; > +} > diff --git a/test/box/gh-4511-access-settings-from-any-frontend.result b/test/box/gh-4511-access-settings-from-any-frontend.result > index 1c3ca7661..0597365a1 100644 > --- a/test/box/gh-4511-access-settings-from-any-frontend.result > +++ b/test/box/gh-4511-access-settings-from-any-frontend.result > @@ -343,6 +343,39 @@ settings.sql_defer_foreign_keys > | - false > | ... > > +box.execute([[set sql_default_engine = 'vinyl']]) > + | --- > + | - row_count: 1 > + | ... > +assert(s:get('sql_default_engine').value == 'vinyl') 4. Please, avoid calling assert. Because in case it will fail, you won't get the real value, and it will be harder to debug. Especially if that test will become flaky some day. Assert is ok, if you know for sure what the other value is, or when there is a function/cycle, which can't print directly to the output. > + | --- > + | - true > + | ... > +box.execute([[set sql_default_engine = 'memtx']]) > + | --- > + | - row_count: 1 > + | ... > +assert(s:get('sql_default_engine').value == 'memtx') > + | --- > + | - true > + | ... > +box.execute([[set sql_defer_foreign_keys = true]]) > + | --- > + | - row_count: 1 > + | ... > +assert(s:get('sql_defer_foreign_keys').value == true) > + | --- > + | - true > + | ... > +box.execute([[set sql_defer_foreign_keys = false]]) > + | --- > + | - row_count: 1 > + | ... > +assert(s:get('sql_defer_foreign_keys').value == false) > + | --- > + | - true > + | ... > +