Re: [Tarantool-patches] [PATCH v3 3/5] sql: introduce SET statement

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes!

See 4 comments below.

>>>>> @@ -3382,3 +3408,82 @@ sql_session_opt_tuple(struct tuple_format *format, int option_id,
>>>>>  	*result = tuple;
>>>>>  	return 0;
>>>>>  }
>>>>> +
>>>>> +static void
>>>>> +sql_set_session_option(struct Parse *parse_context, int id, struct Expr *value)
>>>>> +{
>>>>> +	struct session *session = current_session();
>>>>> +	struct sql_option_metadata *option = &sql_session_opts[id];
>>>>> +	if (value->type != option->field_type) {
>>>>> +		diag_set(ClientError, ER_INCONSISTENT_TYPES,
>>>>> +			 field_type_strs[option->field_type],
>>>>> +			 field_type_strs[value->type]);
>>>>> +		parse_context->is_aborted = true;
>>>>> +		return;
>>>>> +	}
>>>>> +	if (value->type == FIELD_TYPE_BOOLEAN) {
>>>>> +		bool is_set = value->op == TK_TRUE;
>>>>> +		if (is_set)
>>>>> +			session->sql_flags |= option->mask;
>>>>> +		else
>>>>> +			session->sql_flags &= ~option->mask;
>>>>> +#ifndef NDEBUG
>>>>> +		if (id == SQL_SESSION_OPTION_PARSER_TRACE) {
>>>>> +			if (is_set)
>>>>> +				sqlParserTrace(stdout, "parser: ");
>>>>> +			else
>>>>> +				sqlParserTrace(NULL, NULL);
>>>>> +		}
>>>>> +#endif
>>>>> +	} else {
>>>>> +		assert(id == SQL_SESSION_OPTION_DEFAULT_ENGINE);
>>>>> +		enum sql_storage_engine engine =
>>>>> +			STR2ENUM(sql_storage_engine, value->u.zToken);
>>>>> +		if (engine == sql_storage_engine_MAX) {
>>>>> +			parse_context->is_aborted = true;
>>>>> +			diag_set(ClientError, ER_NO_SUCH_ENGINE,
>>>>> +				 value->u.zToken);
>>>>> +			return;
>>>>> +		}
>>>>> +		current_session()->sql_default_engine = engine;
>>>>> +	}
>>>>> +}
>>>>
>>>> 5. Wait. So the settings are set during parsing? Isn't it wrong?
>>>> 'Parser only parses' and everything? PRAGMA works in runtime.
>>> No, PRAGMA also set during parsing. I once suggested to
>>> change it, so parser would only parse, but my suggestion
>>> was declined.
>>
>> 3. This is bad and should be fixed.
>> https://github.com/tarantool/tarantool/issues/4621
> Fixed. Since control pragmas will be deleted, SET is now
> using VDBE I closed the issue in this commit. Still, it
> may be better to close it in the last commit of the
> patch-set. Should I move it to the last commit?

1. You can try to fix it now, it is ok.

Talking of control pragmas, PRAGMA still exists after your
patch. And still works during parsing. I don't know what to
do with it, because I don't know what are we going to do
with PRAGMA. And seems like nobody knows.

> This patch creates an SQL SET statement. This statement replaces
> pragmas that can modify SQL settings. List of pragmas that will
> have the corresponding option in SET:
> 'defer_foreign_keys'
> 'full_column_names'
> 'recursive_triggers'
> 'reverse_unordered_selects'
> 'sql_compound_select_limit'
> 'sql_default_engine'
> 
> 'parser_trace'
> 'select_trace'
> 'sql_trace'
> 'vdbe_addoptrace'
> 'vdbe_debug'
> 'vdbe_eqp'
> 'vdbe_listing'
> 'vdbe_trace'
> 'where_trace'
> 
> All these pragmas along with the pragmas 'short_column_names' and
> 'count_changes' will be removed in the next patch.
> 
> Part of #4511
> Closes #4621

2. This commit message is getting worse and worse.
I think it needs to be totally rewritten. Still existing
problems with the current message:

- I don't understand, which pragmas are totally deleted,
  which moved to SET, which of the moved have a new name;

- From the message it still is not clear when a user needs
  to use PRAGMA and when SET. Yes, you said PRAGMA is purely
  informative, but I don't see it in the message. And the
  technical writer, who will see this message out of all
  the discussion, will be very confused with what has
  happened, and will ask for more details, I can guarantee
  that;

- Parts of the message before '@Tarantoolbot document' and
  after are in some ideas different, in some they are totally
  the same. I propose you to remove the part before doc
  request, and write the doc request more accurate. And check
  again that all deleted PRAGMAs really don't work (in the last
  patch), all new SETs really work, with exactly the same names
  as in the commit message;

- I think you need to explain what is a result of SET - metadata
  like DQL? Sql_info like DML/DDL?

> 
> @TarantoolBot document
> Title: SQL SET statement
> SQL SET statement is used to change SQL settings. To change the
> value of an SQL parameter, use the following syntax:
> 
> SET <name of the setting> = <value of the setting>;
> 
> Currently available SQL settings:
> 'sql_defer_foreign_keys'
> 'sql_full_column_names'
> 'sql_recursive_triggers'
> 'sql_reverse_unordered_selects'
> 'sql_compound_select_limit'
> 'sql_default_engine'
> 
> In addition, SQL debugging settings can also be changed using this
> statement in the debug build:
> 'sql_parser_trace'
> 'sql_select_trace'
> 'sql_trace'
> 'sql_vdbe_addoptrace'
> 'sql_vdbe_debug'
> 'sql_vdbe_eqp'
> 'sql_vdbe_listing'
> 'sql_vdbe_trace'
> 'sql_where_trace'
> 
> All of these setting with exception of 'sql_compound_select_limit'
> are session-local settings. Their value can be viewed in
> _vsession_settings sysview.
> 
> Example of usage:
> SET sql_full_column_names = true;
> SET sql_compound_select_limit = 10;
> SET sql_default_engine = 'memtx';
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index ce87b88..8e3ba6a 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -3382,3 +3397,86 @@ sql_session_opt_tuple(struct tuple_format *format, int option_id,
>  	*result = tuple;
>  	return 0;
>  }
> +
> +int
> +sql_set_session_option(int id, void *opt_value)
> +{
> +	struct Expr *value = (struct Expr *)opt_value;
> +	struct session *session = current_session();
> +	struct sql_option_metadata *option = &sql_session_opts[id];
> +	if (value->type != option->field_type) {
> +		diag_set(ClientError, ER_INCONSISTENT_TYPES,
> +			 field_type_strs[option->field_type],
> +			 field_type_strs[value->type]);
> +		return -1;
> +	}
> +	if (value->type == FIELD_TYPE_BOOLEAN) {
> +		bool is_set = value->op == TK_TRUE;
> +		if (is_set)
> +			session->sql_flags |= option->mask;
> +		else
> +			session->sql_flags &= ~option->mask;
> +#ifndef NDEBUG
> +		if (id == SQL_SESSION_OPTION_PARSER_TRACE) {
> +			if (is_set)
> +				sqlParserTrace(stdout, "parser: ");
> +			else
> +				sqlParserTrace(NULL, NULL);
> +		}
> +#endif
> +	} else {
> +		assert(id == SQL_SESSION_OPTION_DEFAULT_ENGINE);
> +		enum sql_storage_engine engine =
> +			STR2ENUM(sql_storage_engine, value->u.zToken);
> +		if (engine == sql_storage_engine_MAX) {
> +			diag_set(ClientError, ER_NO_SUCH_ENGINE,
> +				 value->u.zToken);
> +			return -1;
> +		}
> +		current_session()->sql_default_engine = engine;
> +	}
> +	return 0;
> +}
> +
> +int
> +sql_set_global_option(int id, void *opt_value)
> +{
> +	struct Expr *value = (struct Expr *)opt_value;
> +	(void)id;
> +	assert(id == SQL_GLOBAL_OPTION_COMPOUND_SELECT_LIMIT);
> +	int limit = value->u.iValue;
> +	int rc = sql_limit(sql_get(), SQL_LIMIT_COMPOUND_SELECT, limit);
> +	assert(rc >= 0);
> +	return rc < 0 ? -1 : 0;
> +}
> +
> +void
> +sql_set_settings(struct Parse *parse_context, struct Token *name,
> +		 struct Expr *value)
> +{
> +	int opt_id;
> +	const char *val = (const char*) value;
> +	struct Vdbe *vdbe = sqlGetVdbe(parse_context);
> +	char *name_str = sql_name_from_token(sql_get(), name);
> +	if (name_str == NULL) {
> +		parse_context->is_aborted = true;
> +		return;
> +	}
> +	/* Try to find the option among the session options. */
> +	for (opt_id = 0; opt_id < SQL_SESSION_OPTION_max; ++opt_id) {
> +		if (strcasecmp(sql_session_opts[opt_id].name, name_str) == 0) {
> +			sqlVdbeAddOp4(vdbe, OP_Set, 0, opt_id, 0, val, P4_PTR);
> +			return;

3. Expr is a parser time structure. You can't use it at runtime.

4. There is a leak. 'val' is not freed.

> +		}
> +	}
> +	/* Try to find the option among the global options. */
> +	for (opt_id = 0; opt_id < SQL_GLOBAL_OPTION_max; ++opt_id) {
> +		if (strcasecmp(sql_global_opts[opt_id].name, name_str) == 0) {
> +			sqlVdbeAddOp4(vdbe, OP_Set, 1, opt_id, 0, val, P4_PTR);
> +			return;
> +		}
> +	}
> +	diag_set(ClientError, ER_SQL_PARSER_GENERIC, "Setting is not found");
> +	parse_context->is_aborted = true;
> +	return;
> +}