Hi Sanja,
Please find my review below.
Lots of typos, but the two important comments are at the bottom, for the changes
in sql_select.cc and sql_trigger.cc
On Sat, Jan 27, 2018 at 11:39:10AM +0100, Oleksandr Byelkin wrote:
> revision-id: a0ea3a5822486fa1702d6615f1be30684c3d412f
> (mariadb-10.1.30-26-ga0ea3a58224)
> parent(s): 524221e7a34d42214e82dd868348b453f2ef1591
> author: Oleksandr Byelkin
> committer: Oleksandr Byelkin
> timestamp: 2018-01-27 11:38:38 +0100
> message:
>
> MDEV-14857: problem with 10.2.11 server crashing when executing stored
> procedure
>
> Counter for select numbering made stored with the statement (before was
> global)
> So now it does have always accurate value which does not depend on
> interruption of statement prepare by errors like lack of table in
> a view definition.
>
> ---
> diff --git a/sql/sql_class.h b/sql/sql_class.h
> index 4556487bdfe..2dc2fb61e9d 100644
> --- a/sql/sql_class.h
> +++ b/sql/sql_class.h
> @@ -1027,6 +1027,21 @@ class Statement: public ilink, public Query_arena
>
> LEX_STRING name; /* name for named prepared statements */
> LEX *lex; // parse tree descriptor
> + /*
> + LEX whith represent curent statement (conventional, SP or PS)
s/whith/which/
s/curent/current/
s/represent/represents/
> +
> + For example during view parsing THD::lex will point to the views LEX and
> + THD::stmt_lex will point on LEX of the statement where the view will be
s/point on/point to/
> + included
> +
> + Now used to have alwais correct select numbering inside statemet
s/Now used/Currently it is used/
s/alwais/always/
s/statemet/statement/
> + (LEX::current_select_number) without storing and restoring a global
> + counter which was THD::select_number.
> +
> + TODO: make some unified statement representation (now SP has different)
> + to store such data like LEX::current_select_number.
> + */
> + LEX *stmt_lex;
> /*
> Points to the query associated with this statement. It's const, but
> we need to declare it char * because all table handlers are written
> diff --git a/sql/sql_lex.h b/sql/sql_lex.h
> index b57fba08b47..a2da6f672ca 100644
> --- a/sql/sql_lex.h
> +++ b/sql/sql_lex.h
> @@ -727,7 +727,13 @@ class st_select_lex: public st_select_lex_node
> Item *prep_having;/* saved HAVING clause for prepared statement processing
> */
> /* Saved values of the WHERE and HAVING clauses*/
> Item::cond_result cond_value, having_value;
> - /* point on lex in which it was created, used in view subquery detection */
> + /*
> + Point on lex in which it was created, used in view subquery detection.
"Points to the LEX...". Also please shift the whole comment 1 position to the
left.
> +
> + TODO: make also st_select_lex::parent_stmt_lex (see THD::stmt_lex)
> + and use st_select_lex::parent_lex & st_select_lex::parent_stmt_lex
> + instead of global (from THD) references where it is possible.
> + */
> LEX *parent_lex;
> enum olap_type olap;
> /* FROM clause - points to the beginning of the TABLE_LIST::next_local
> list. */
> @@ -2452,7 +2458,7 @@ struct LEX: public Query_tables_list
> /** SELECT of CREATE VIEW statement */
> LEX_STRING create_view_select;
>
> - uint number_of_selects; // valid only for view
> + uint current_select_number; // valid for statmet LEX (not view)
s/statmet/statement/
>
> /** Start of 'ON table', in trigger statements. */
> const char* raw_trg_on_table_name_begin;
> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> index 6084c59a257..8d3116cd8b3 100644
> --- a/sql/sql_parse.cc
> +++ b/sql/sql_parse.cc
> @@ -6903,7 +6903,12 @@ void THD::reset_for_next_command(bool do_clear_error)
> clear_error(1);
>
> thd->free_list= 0;
> - thd->select_number= 1;
> + /*
> + The same thd->stmt_lex made in lex_start, but during bootstrup this
"We also assign thd->stmt_lex in lex_start()"
s/bootstrup/bootstrap
> + code executed first
code is executed
> + */
> + thd->stmt_lex= &main_lex; thd->stmt_lex->current_select_number= 1;
> + DBUG_PRINT("info", ("Lex %p stmt_lex: %p", thd->lex, thd->stmt_lex));
> /*
> Those two lines below are theoretically unneeded as
> THD::cleanup_after_query() should take care of this already.
> diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> index b9fe8f3162a..7a6a028ee9c 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -2462,6 +2462,17 @@ void JOIN::save_explain_data(Explain_query *output,
> bool can_overwrite,
> bool need_tmp_table, bool need_order,
> bool distinct)
> {
> + /*
> + If there is SELECT in this statemet with the same number it must be the
> + same SELECT
> + */
> + DBUG_ASSERT(select_lex->select_number == UINT_MAX ||
> + select_lex->select_number == INT_MAX ||
> + !output ||
Is it really possible that output==NULL? If yes, that's probably a bug.
> + !output->get_select(select_lex->select_number) ||
> + output->get_select(select_lex->select_number)->select_lex ==
> + select_lex);
> +
> if (select_lex->select_number != UINT_MAX &&
> select_lex->select_number != INT_MAX /* this is not a UNION's "fake
> select */ &&
> have_query_plan != JOIN::QEP_NOT_PRESENT_YET &&
> @@ -24601,6 +24612,11 @@ int JOIN::save_explain_data_intern(Explain_query
> *output, bool need_tmp_table,
> {
> explain= new (output->mem_root) Explain_select(output->mem_root,
> thd->lex->analyze_stmt);
> + if (!explain)
> + DBUG_RETURN(1); // EoM
> +#ifndef DBUG_OFF
> + explain->select_lex= select_lex;
> +#endif
> join->select_lex->set_explain_type(true);
>
> explain->select_id= join->select_lex->select_number;
> diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc
> index 0b4978b2862..878cd5a3b57 100644
> --- a/sql/sql_trigger.cc
> +++ b/sql/sql_trigger.cc
> @@ -1378,7 +1378,7 @@ bool Table_triggers_list::check_n_load(THD *thd, const
> char *db,
> ulonglong save_sql_mode= thd->variables.sql_mode;
> LEX_STRING *on_table_name;
>
> - thd->lex= &lex;
> + thd->lex= thd->stmt_lex= &lex;
>
The same comment as before: we change thd->stmt_lex here.
The function has code below that restores the value of thd->lex:
// QQ: anything else ?
lex_end(&lex);
thd->lex= old_lex;
But the value of thd->stmt_lex is not restored?
> save_db.str= thd->db;
> save_db.length= thd->db_length;
BR
Sergei
--
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
