maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr,

On Jan 27, Oleksandr Byelkin wrote:
> commit 349283c5e7a
> Author: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> Date:   Wed Apr 17 15:50:59 2019 +0200
> 
>     MDEV-17124: mariadb 10.1.34, views and prepared statements:  ERROR 1615 (HY000): Prepared statement needs to be re-prepared
>     
>     The problem is that if table definition cache (TDC) is full of real tables
>     which are in tables cache, view definition can not stay there so will be
>     removed by its own underlying tables.
>     In situation above old mechanism of detection matching definition in PS
>     and current version always require reprepare and so prevent executing
>     the PS.
>     
>     One work around is to increase TDC, other - improve version check for
>     views/triggers (which is done here). Now in suspicious cases we check:
>      - timestamp (ms) of the view to be sure that version really have changed;
>      - time (ms) of creation of a trigger related to time (ms) of statement
>        preparation.

not "ms" but "us" or "µs". microseconds, not millisecons.

> diff --git a/mysql-test/r/ps_ddl.result b/mysql-test/r/ps_ddl.result
> index e69c6e06193..005c78a0319 100644
> --- a/mysql-test/r/ps_ddl.result
> +++ b/mysql-test/r/ps_ddl.result
> @@ -831,8 +831,8 @@ a	b	c
>  20	40	100
>  30	60	150
>  call p_verify_reprepare_count(1);
> -SUCCESS
> -
> +ERROR
> +Expected: 1, actual: 0

oops, forgot to update?

>  # Check that we properly handle ALTER VIEW statements.
>  execute stmt;
>  a	b	c
> @@ -882,9 +882,9 @@ a	b	c
>  10	50	60
>  20	100	120
>  30	150	180
> -call p_verify_reprepare_count(1);
> -SUCCESS
> -
> +call p_verify_reprepare_count(0);
> +ERROR
> +Expected: 0, actual: 1

oops, updated too much?

>  execute stmt;
>  a	b	c
>  10	50	60
> diff --git a/sql/sql_class.h b/sql/sql_class.h
> index 3f0fba8fc10..b9bb9c978fb 100644
> --- a/sql/sql_class.h
> +++ b/sql/sql_class.h
> @@ -1079,6 +1079,7 @@ class Statement: public ilink, public Query_arena
>  
>    LEX_STRING name; /* name for named prepared statements */
>    LEX *lex;                                     // parse tree descriptor
> +  ulonglong ms_prepare_time; // time of preparation in microseconds

"ms" here too :(
Better just say

    my_hrtime_t prepare_time;

>    /*
>      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_prepare.cc b/sql/sql_prepare.cc
> index 64e4cd30561..a4a24cc1a80 100644
> --- a/sql/sql_prepare.cc
> +++ b/sql/sql_prepare.cc
> @@ -4177,6 +4177,9 @@ bool Prepared_statement::prepare(const char *packet, uint packet_len)
>    Query_arena *old_stmt_arena;
>    DBUG_ENTER("Prepared_statement::prepare");
>    DBUG_ASSERT(m_sql_mode == thd->variables.sql_mode);
> +
> +  // The same format as for triggers to compare
> +  ms_prepare_time= my_hrtime().val;

do it at the end instead?

>    /*
>      If this is an SQLCOM_PREPARE, we also increase Com_prepare_sql.
>      However, it seems handy if com_stmt_prepare is increased always,
> @@ -4519,6 +4523,22 @@ Prepared_statement::execute_loop(String *expanded_query,
>      DBUG_ASSERT(thd->get_stmt_da()->sql_errno() == ER_NEED_REPREPARE);
>      thd->clear_error();
>  
> +    {
> +      /*
> +        Check if we too fast with reprepare:
> +        we can be so fast that:
> +          1) make change of a trigger,
> +          2) prepare,
> +          3) try to exacute and reprepare
> +        in 1 microsecond, so we will wait till
> +        next microsecond before last reprepare
> +      */
> +      while (first_prepared == my_hrtime().val)
> +      {
> +        pthread_yield();
> +      }
> +    }

I wouldn't bother. A more likely case is when a system time goes back,
e.g. if there's an ntpd running and your loop doesn't help there.

I suggest not to do anything special in this case. And if an ::execute()
will open a trigger or a view with a timestamp >= than ms_prepare_time,
it will cause reprepare every time. Which is exactly the behavior before
your bugfix - opening of a trigger or a view causes reprepare - and
it is not a problem unless one has a very small table definition cache.

> +
>      error= reprepare();
>  
>      if (! error)                                /* Success */
> diff --git a/sql/parse_file.cc b/sql/parse_file.cc
> index 999f53bd681..92e7d82af1c 100644
> --- a/sql/parse_file.cc
> +++ b/sql/parse_file.cc
> @@ -174,11 +174,11 @@ write_parameter(IO_CACHE *file, uchar* base, File_option *parameter)
>    {
>      /* string have to be allocated already */
>      LEX_STRING *val_s= (LEX_STRING *)(base + parameter->offset);
> -    time_t tm= my_time(0);
> -
> -    get_date(val_s->str, GETDATE_DATE_TIME|GETDATE_GMT|GETDATE_FIXEDLENGTH,
> -	     tm);
> -    val_s->length= PARSE_FILE_TIMESTAMPLENGTH;
> +    ulonglong tm= my_hrtime().val;

add a comment here, "number of microseconds since Epoch, timezone-independent"
or something like that

> +    // Paded to 19 characters for compatibility
> +    val_s->length= snprintf(val_s->str, MICROSECOND_TIMESTAMP_BUFFER_SIZE,
> +                            "%019lld", tm);
> +    DBUG_ASSERT(val_s->length == MICROSECOND_TIMESTAMP_BUFFER_SIZE-1);
>      if (my_b_write(file, (const uchar *)val_s->str,
>                      PARSE_FILE_TIMESTAMPLENGTH))
>        DBUG_RETURN(TRUE);
> @@ -835,15 +835,15 @@ File_parser::parse(uchar* base, MEM_ROOT *mem_root,
>  	  /* string have to be allocated already */
>  	  LEX_STRING *val= (LEX_STRING *)(base + parameter->offset);
>  	  /* yyyy-mm-dd HH:MM:SS = 19(PARSE_FILE_TIMESTAMPLENGTH) characters */

a comment is now wrong

> -	  if (ptr[PARSE_FILE_TIMESTAMPLENGTH] != '\n')
> +	  if (ptr[MICROSECOND_TIMESTAMP_BUFFER_SIZE-1] != '\n')
>  	  {
>  	    my_error(ER_FPARSER_ERROR_IN_PARAMETER, MYF(0),
>                       parameter->name.str, line);
>  	    DBUG_RETURN(TRUE);
>  	  }
> -	  memcpy(val->str, ptr, PARSE_FILE_TIMESTAMPLENGTH);
> -	  val->str[val->length= PARSE_FILE_TIMESTAMPLENGTH]= '\0';
> -	  ptr+= (PARSE_FILE_TIMESTAMPLENGTH+1);
> +	  memcpy(val->str, ptr, MICROSECOND_TIMESTAMP_BUFFER_SIZE-1);
> +	  val->str[val->length= MICROSECOND_TIMESTAMP_BUFFER_SIZE-1]= '\0';
> +	  ptr+= MICROSECOND_TIMESTAMP_BUFFER_SIZE;
>  	  break;
>  	}
>  	case FILE_OPTIONS_STRLIST:
> diff --git a/sql/table.h b/sql/table.h
> index 14d6b787b4e..82508388f84 100644
> --- a/sql/table.h
> +++ b/sql/table.h
> @@ -2159,7 +2175,7 @@ struct TABLE_LIST
>    LEX_STRING	source;			/* source of CREATE VIEW */
>    LEX_STRING	view_db;		/* saved view database */
>    LEX_STRING	view_name;		/* saved view name */
> -  LEX_STRING	timestamp;		/* GMT time stamp of last operation */
> +  LEX_STRING	ms_timestamp;		/* GMT time stamp of last operation */

better keep "timestamp" name. Or "hrtimestamp" if you'd like

>    st_lex_user   definer;                /* definer of view */
>    ulonglong	file_version;		/* version of file's field set */
>    ulonglong	mariadb_version;	/* version of server on creation */
> diff --git a/sql/table.cc b/sql/table.cc
> index ca6ce02e4f2..24412966b36 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -8480,6 +8480,53 @@ bool TABLE_LIST::is_with_table()
>  }
>  
>  
> +bool TABLE_LIST::is_table_ref_id_equal(THD* thd, TABLE_SHARE *s)
> +{
> +  enum enum_table_ref_type tp= s->get_table_ref_type();
> +  if (m_table_ref_type == tp)
> +  {
> +    bool res= m_table_ref_version == s->get_table_ref_version();
> +
> +    /*
> +      If definition is different check content version
> +    */
> +    if (tabledef_version.length &&
> +        tabledef_version.length == s->tabledef_version.length &&
> +        memcmp(tabledef_version.str, s->tabledef_version.str,
> +               tabledef_version.length) == 0)
> +    {
> +      if (table && table->triggers)
> +      {
> +
> +        ulonglong ms_stmt_prepare= thd->ms_prepare_time;
> +        if (ms_stmt_prepare)
> +          for(uint i= 0; i < TRG_EVENT_MAX; i++)
> +            for (uint j= 0; j < TRG_ACTION_MAX; j++)
> +            {
> +              Trigger *tr=
> +                table->triggers->get_trigger((trg_event_type)i,
> +                                             (trg_action_time_type)j);
> +              if (tr)
> +                if (ms_stmt_prepare <= tr->ms_create_time)
> +                {
> +                  set_tabledef_version(s);
> +                  return FALSE;
> +                }
> +            }
> +      }
> +      set_table_id(s);
> +      return TRUE;
> +    }
> +    else
> +      tabledef_version.length= 0;
> +    return res;
> +  }
> +  else
> +    set_tabledef_version(s);
> +  return FALSE;
> +}

this is now a very confusing function. it doesn't only check
is_table_ref_id_equal, it also sets some of them (tabledef or both
tabledef and ref_id), and under unclear conditions.
Could you please comment it?

And you don't check that view's timestamp is in the past. If it's not,
it can be changed still within the same microsecond.

> +
> +
>  uint TABLE_SHARE::actual_n_key_parts(THD *thd)
>  {
>    return use_ext_keys &&
> diff --git a/sql/sql_trigger.h b/sql/sql_trigger.h
> index e2e6e1b7acc..70f246d4e0d 100644
> --- a/sql/sql_trigger.h
> +++ b/sql/sql_trigger.h
> @@ -112,7 +112,7 @@ class Trigger :public Sql_alloc
>    GRANT_INFO subject_table_grants;
>    sql_mode_t sql_mode;
>    /* Store create time. Can't be mysql_time_t as this holds also sub seconds */
> -  ulonglong create_time;
> +  ulonglong ms_create_time; // Create time timestamp in microseconds

 my_hrtime_t create_time;

>    trg_event_type event;
>    trg_action_time_type action_time;
>    uint action_order;
> @@ -195,7 +195,7 @@ class Table_triggers_list: public Sql_alloc
>    */
>    List<ulonglong> definition_modes_list;
>    /** Create times for triggers */
> -  List<ulonglong> create_times;
> +  List<ulonglong> ms_create_times;

ditto

>  
>    List<LEX_STRING>  definers_list;
>  
> @@ -331,4 +331,14 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create);
>  extern const char * const TRG_EXT;
>  extern const char * const TRN_EXT;
>  
> +
> +/**
> +  Make time compatible with MySQL 5.7 trigger time.
> +*/
> +
> +inline ulonglong make_ms_time(my_time_t time, ulong time_sec_part)

make_hrtime. and better put it together with hrtime_to_time,
hrtime_from_time, etc

> +{
> +  return ((ulonglong) time)*1000000 + time_sec_part;
> +}
> +
>  #endif /* SQL_TRIGGER_INCLUDED */
> diff --git a/sql/sql_view.cc b/sql/sql_view.cc
> index 020830483c5..3b51f6d2451 100644
> --- a/sql/sql_view.cc
> +++ b/sql/sql_view.cc
> @@ -1131,7 +1141,31 @@ static int mysql_register_view(THD *thd, TABLE_LIST *view,
>    DBUG_RETURN(error);
>  }
>  
> +/**
> +  Check is TABLE_LEST and SHARE match
> +  @param[in]  view                TABLE_LIST of the view
> +  @param[in]  share               Share object of view
> +
> +  @return false on error or misspatch
> +*/
>  
> +bool mariadb_view_version_get(TABLE_SHARE *share)
> +{
> +  DBUG_ASSERT(share->is_view);
> +
> +  if (!(share->tabledef_version.str=
> +        (uchar*) alloc_root(&share->mem_root,
> +                            MICROSECOND_TIMESTAMP_BUFFER_SIZE)))
> +    return TRUE;
> +
> +  DBUG_ASSERT(share->view_def != NULL);
> +  if (share->view_def->parse((uchar *) &share->tabledef_version, NULL,
> +                             view_timestamp_parameters, 1,
> +                             &file_parser_dummy_hook))
> +    return TRUE;
> +  DBUG_ASSERT(share->tabledef_version.length == MICROSECOND_TIMESTAMP_BUFFER_SIZE-1);

If you open an old view with YYYY-MM-DD... timestamp,
where do you convert it?

If you open a view without a timestamp at all, where do you handle it?

> +  return FALSE;
> +}
>  
>  /**
>    read VIEW .frm and create structures
> diff --git a/sql/sql_show.cc b/sql/sql_show.cc
> index 9483db9eff9..138fa4bc631 100644
> --- a/sql/sql_show.cc
> +++ b/sql/sql_show.cc
> @@ -6724,13 +6724,15 @@ static bool store_trigger(THD *thd, Trigger *trigger,
>    table->field[14]->store(STRING_WITH_LEN("OLD"), cs);
>    table->field[15]->store(STRING_WITH_LEN("NEW"), cs);
>  
> -  if (trigger->create_time)
> +  if (trigger->ms_create_time)
>    {
> +    /* timestamp is in microseconds */
>      table->field[16]->set_notnull();
>      thd->variables.time_zone->gmt_sec_to_TIME(&timestamp,
> -                                              (my_time_t)(trigger->create_time/100));
> -    /* timestamp is with 6 digits */
> -    timestamp.second_part= (trigger->create_time % 100) * 10000;
> +                                              (my_time_t)
> +                                              (trigger->ms_create_time/
> +                                               1000000));
> +    timestamp.second_part= trigger->ms_create_time % 1000000;

hrtime_to_time() and hrtime_sec_part()

>      ((Field_temporal_with_date*) table->field[16])->store_time_dec(&timestamp,
>                                                                     2);
>    }
> @@ -9810,12 +9812,14 @@ static bool show_create_trigger_impl(THD *thd, Trigger *trigger)
>             trigger->db_cl_name.length,
>             system_charset_info);
>  
> -  if (trigger->create_time)
> +  if (trigger->ms_create_time)
>    {
>      MYSQL_TIME timestamp;
>      thd->variables.time_zone->gmt_sec_to_TIME(&timestamp,
> -                                              (my_time_t)(trigger->create_time/100));
> -    timestamp.second_part= (trigger->create_time % 100) * 10000;
> +                                              (my_time_t)
> +                                              (trigger->ms_create_time/
> +                                               1000000));
> +    timestamp.second_part= trigger->ms_create_time % 1000000;

ditto

>      p->store(&timestamp, 2);
>    }
>    else

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx