Re: [Maria-developers] 5265f7001c6: MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared
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 security@mariadb.org
Hi, Sergei! On Tue, Sep 27, 2022 at 2:36 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Oleksandr,
Few minor comments, see below
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
On Sep 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 (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@mariadb.org
Hi, Oleksandr, On Sep 28, Oleksandr Byelkin wrote:
+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))
I still don't understand. I'd expect something like if (!res && (tabledef_version.length && ... that is, if m_table_ref_version matches, then you don't need to do any further checks, the table is fine. If m_table_ref_version differs, meaning, the table was reopened, then you check tabledef_version and triggers. Perhaps the simplest fix would be to remove res and instead do if (m_table_ref_version == s->get_table_ref_version()) return TRUE; if (tabledef_version.length && ...
{ if (table && table->triggers) {
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
The check of triggers (if present) need only if definition did not changed. Before the check need all that if would be just "return true". Sergei Golubchik <serg@mariadb.org> schrieb am Mi., 28. Sept. 2022, 19:15:
Hi, Oleksandr,
On Sep 28, Oleksandr Byelkin wrote:
+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))
I still don't understand. I'd expect something like
if (!res && (tabledef_version.length && ...
that is, if m_table_ref_version matches, then you don't need to do any further checks, the table is fine. If m_table_ref_version differs, meaning, the table was reopened, then you check tabledef_version and triggers.
Perhaps the simplest fix would be to remove res and instead do
if (m_table_ref_version == s->get_table_ref_version()) return TRUE;
if (tabledef_version.length && ...
{ if (table && table->triggers) {
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Oleksandr Byelkin -
Sergei Golubchik