Code review comment for lp:~vlad-lesin/percona-server/5.6-per_query_variables_settings

Alexey Kopytov (akopytov) wrote :

Hi Vlad,

On Fri, 27 Sep 2013 21:29:46 -0000, Vlad Lesin wrote:>> Hi Vlad,
>>
>> On Mon, 23 Sep 2013 10:27:28 +0400, Vlad Lesin wrote:
>>
>>>> - I guess changes in sql_prepare.cc are no longer required and can be
>>>> reverted?
>>> No, they are still required because objects of "Item" class are
>>> allocated during parsing and thd->free_list contains list of those objects.
>>>
>>
>> OK, I see it now. Shouldn't we instead of adjusting the assertion in
>> sql_prepare.cc just reset thd->free_list after calling
>> sql_set_variables() in mysql_execute_command()? It was impossible with
>> the previous implementation, but is possible now, right?
>
> Resetting thd->free_list after each sql_set_variables() call does not work right because thd->free_list can contain "Item" class instances not only from SET STATEMENT parsing. If fixes in sql_prepare.cc are not desirable thd->free_list can be released in SQLCOM_EXECUTE command handler in mysql_execute_command() function. As it can be seen I have done it. But I have some doubts about this because there is no common solution and the issue which is in sql_prepare.cc is fixed in sql_parse.cc, but can be fixed in the code which is the source of the issue. I do not think such approach will add understanding in the code even if it is documented in comments because referring from one part of code to another can lead to the situation when some part of code is changed but depended part is not changed.
>

Right, but I think the current way is more correct than adjusting the assertion in Prepared_statement::execute_loop(), because this way we keep the invariant that code relies on (no externally allocated objects when executing a prepared statement).

> The following note was ignored but I consider it is very important and it should be discussed:
> ---
> But this implementation has disadvantage. In the case of "SET STATEMENT
> ... FOR SET SESSION ..." query lex->var_list will be cleared when SET
> SESSION is parsed, so all variables settings in SET STATEMENT section
> will be cleared. The solution could be in using separate list for
> statement variables. But in this case all variables which was set in
> "SET SESSION" part will be reset to pre-statement state as
> thd->variables will be completely restored after statement execution.
> ---
>

I don't think SET STATEMENT ... FOR SET SESSSION ... is a "very important" case. I'm fine with documenting it as a known limitation and figuring out a way to fix it, if and when someone gets unhappy about it.

Approved.

review: Approve

« Back to merge proposal