maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

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