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

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

Hi! Thanks for the fixes!

See 3 comments below.

>>> SET sql_compound_select_limit = 10;
>>> SET sql_default_engine = 'memtx';
>>
>> 2. It would be cool to sum up here what is a purpose of
>> PRAGMA and SET. How are they different. From the code I
>> see, that looks like PRAGMA never changes anything. It
>> only returns something. While SET only sets and never
>> returns, right?
>>
> How pragma works:
> PRAGMA vdbe_trace = true; -- Sets vdbe_trace, returns nothing.
> PRAGMA vdbe_trace; -- Returns current value of vdbe_trace flag.

1. These PRAGMAs don't exist anymore. I wanted to understand
how rest of the pragmas (not removed) coexist with SET. When a
user should try to look at PRAGMA list, and when at SET list?

> 
> How SET works:
> SET sql_vdbe_trace = true; -- Sets vdbe_trace, returns nothing.
> 
>> In the next patch I see diff like this:
>>
>>> -        return test:execsql "PRAGMA full_column_names"
>>> -    end, {
>>> +        return box.space._vsession_settings:get("sql_full_column_names").value
>>> +    end,
>>
>> Is there any plan how to make it simpler? Seems like it is
>> impossible for a user to get session settings other than via
>> direct select from a system space. It was simpler with PRAGMA.
>>
> As far as I know, there is no such plans. It definitely was
> simpler with PRAGMA, but it was decided to remove PRAGMA.
> I do not know exact reaso why.
> 

2. Ok, but now it looks really unusable when a user want's to
learn an option value. I will create a ticket, if this patchset
will be pushed and nobody will care about usability beforehand.

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