maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr,

Few minor comments, see below

On Sep 27, Oleksandr Byelkin wrote:
> revision-id: 5265f7001c6 (mariadb-10.3.36-60-g5265f7001c6)
> parent(s): 86953d3df0a
> author: Oleksandr Byelkin
> committer: Oleksandr Byelkin
> timestamp: 2022-09-27 10:23:31 +0200
> message:
> 
> 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 (microseconds) of the view to be sure that version really
>    have changed;
>  - time (microseconds) of creation of a trigger related to time
>    (microseconds) of statement preparation.
 
> diff --git a/sql/table.cc b/sql/table.cc
> index 506195127b2..f289c2052cb 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -8861,6 +8861,63 @@ bool TABLE_LIST::is_with_table()
>    return derived && derived->with_element;
>  }
>  
> +
> +/**
> +  Check if the definition are the same.
> +
> +  If versions do not match it check definitions (with checking and setting
> +  trigger definition versions (times)
> +
> +  @sa check_and_update_table_version()
> +*/
> +
> +bool TABLE_LIST::is_the_same_definition(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 res == true, why do you need to check tabledef_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)
> +      {
> +        my_hrtime_t hr_stmt_prepare= thd->hr_prepare_time;
> +        if (hr_stmt_prepare.val)
> +          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 (hr_stmt_prepare.val <= tr->hr_create_time.val)
> +                {
> +                  set_tabledef_version(s);
> +                  return FALSE;
> +                }
> +            }
> +      }
> +      set_table_id(s);
> +      return TRUE;
> +    }
> +    else
> +      tabledef_version.length= 0;
> +    return res;
> +  }
> +  else
> +    set_tabledef_version(s);

why not set_table_id() ?

> +  return FALSE;
> +}
> +
> +
>  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 ae3d1738b16..f7a2ccf2abc 100644
> --- a/sql/sql_trigger.h
> +++ b/sql/sql_trigger.h
> @@ -198,7 +198,7 @@ class Table_triggers_list: public Sql_alloc
>    */
>    List<ulonglong> definition_modes_list;
>    /** Create times for triggers */
> -  List<ulonglong> create_times;
> +  List<my_hrtime_t> hr_create_times;

I suspect it might be UB. You use FILE_OPTIONS_ULLLIST for hr_create_times,
perhaps it's safer to use List<ulonglong> here.

>  
>    List<LEX_CSTRING>  definers_list;
>  
> @@ -328,4 +328,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 my_hrtime_t make_hr_time(my_time_t time, ulong time_sec_part)
> +{
> +  return my_hrtime_t({((ulonglong) time)*1000000 + time_sec_part});
> +}

better put it next to hrtime_to_time/etc
and, please, always 'static inline'

> +
>  #endif /* SQL_TRIGGER_INCLUDED */
> diff --git a/sql/sql_view.cc b/sql/sql_view.cc
> index 17dea643144..177d01a312e 100644
> --- a/sql/sql_view.cc
> +++ b/sql/sql_view.cc
> @@ -1154,7 +1163,32 @@ 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

Eh. I don't understad this comment at all. First, I thought you made two
typos, s/is/if/ and s/misspatch/mismatch/. But this function doesn't check if
something matches, if gets the view version, as the name says, the name's
good. But the comment seems to be from is_the_same_definition()

> +*/
> +
> +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;
> +  share->tabledef_version.length= 0; // safety if the drfinition file is brocken
>  
> +  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);
> +  return FALSE;
> +}
>  
>  /**
>    read VIEW .frm and create structures
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx