Hi, Sergei!
On Tue, Sep 27, 2022 at 2:36 PM Sergei Golubchik <s...@mariadb.org> 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 secur...@mariadb.org >
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp