Re: [Maria-developers] 349283c5e7a: MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared

Hi! On Thu, Jan 27, 2022 at 9:15 PM Sergei Golubchik wrote:

Hi, Oleksandr,

...

commit 349283c5e7a Author: Oleksandr Byelkin 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

On Jan 27, Oleksandr Byelkin wrote: 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 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 security@mariadb.org