maria-developers team mailing list archive

[Oleksandr Byelkin <sanja@xxxxxxxxxxx>]

Hi!

On Thu, Jan 27, 2022 at 9:15 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> 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 "盜". microseconds, not millisecons.
>

fixed


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

yes fixed, and all following like this


> >  # 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;
>

I changed everywhere to hr_


> >    /*
> >      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?
>

OK


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

OK I removed it. (not sure that it is 100% safe)


> > +
> >      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
>
> OK

> > +    // 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
>

fixed


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

fixed to ht_timestamp


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

comened and renamed


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

I do not fully understand what you mean.
As I remember there was a version which prevented creation of views in one
microsecond but it was removed (I can not find why).
I can add code to create_view.


>
> > +
> > +
> >  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;
>
fixed (hr_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
>

fixed (make_hr_time)


> > +{
> > +  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?
>

we always treat it as a string, so if it is old and created before server
restart it does not need to convert, it will not match new versions.


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

I added safe length 0 assignment in case of broken .frm it should be enough.


>
> > +  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()
>

they made for events, so I made  hr_time_to_time, hr_time_from_time


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