maria-developers team mailing list archive

[Oleksandr Byelkin <sanja@xxxxxxxxxxx>]

Hi, Sergei!


On Tue, Sep 27, 2022 at 2:36 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

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

You are right, fixed:
--- a/sql/table.cc
+++ b/sql/table.cc
@@ -8881,10 +8881,11 @@ bool TABLE_LIST::is_the_same_definition(THD* thd,
TABLE_SHARE *s)
     /*
       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 (res ||
+        (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)
       {


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

It is a case when type does not patch, and so type and version will be
assigned on reopening.


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

Yes, probably it can be undefined behaviour, so I changed it to ulonglong.


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

OK, moved to the .h and changed it to make it compilable.


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

fixed


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