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

Revision history for this message
Vlad Lesin (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.

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

My suggestions:
1) Forbid using "SET" as a statement after "FOR" keyword. But the problem will not go because SET SESSION can be used in SP or prepared query.
2) Use separate list of parsed variables for SET and SET STATEMENT ... FOR ... statements
3) Check if SET STATEMENT ... FOR ... was parsed in SQLCOM_SET_OPTION handler in mysql_execute_command(). If it was then return error.

Here is new code testing results:
http://jenkins.percona.com/view/PS 5.6/job/percona-server-5.6-param/322

« Back to merge proposal