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