Hi! On Thu, Jan 27, 2022 at 9:15 PM Sergei Golubchik <[email protected]> wrote:
> Hi, Oleksandr, > > On Jan 27, Oleksandr Byelkin wrote: > > commit 349283c5e7a > > Author: Oleksandr Byelkin <[email protected]> > > 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(×tamp, > > - > (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(×tamp, > > 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(×tamp, > > - > (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(×tamp, 2); > > } > > else > > Regards, > Sergei > VP of MariaDB Server Engineering > and [email protected] >
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
