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

[Vladislav Shpilevoy <v.shpilevoy@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
> + | ...
> +