Re: [Maria-developers] b1b38b64598: MDEV-29181 Potential corruption on FK update on a table with vcol index
Hi, Nikita, On Aug 30, Nikita Malyavin wrote:
revision-id: b1b38b64598 (mariadb-10.5.17-4-gb1b38b64598) parent(s): 3b656ac8c17 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2022-08-23 00:27:39 +0300 message:
MDEV-29181 Potential corruption on FK update on a table with vcol index
vc_templ->mysql_table concept is completely broken. This table pointer persists with a global access and can be overwritten by arbitrary thread. Like it wasn't enough, it also could become invalid after eviction from tc cache.
this is a bit misleading. I agree it's completely broken, though :) But not "this table pointer", because, of course it persists with a global access and can be overwritten and so on. This is by design, it's protected by mysql_table_query_id. The problem is that table (dict_table_t) argument of innodb_find_table_for_vc() points to a shared data structure, not thread local, as I believed. So vc_templ is shared too. I suggest to rewrite the comment like vc_templ->mysql_table concept is completely broken. table argument of innodb_find_table_for_vc() points to a shared data structure dict_table_t, and thus vc_templ is shared too and cannot be used for a thread-local cache.
This new solution simply does the following: * Sets up a referenced table list in TABLE instance (sql_base.cc) * Iterates through it along with dict_table_t::referenced_set (row_upd_check_references_constraints) * Passes corresponding prebuilt through a call chain to row_ins_foreign_check_on_constraint * Sets up newly created upd_node_t::prebuilt field and uses it accordingly is cascade update.
Is this upd_node_t::prebuilt used anywhere? As a prebuilt, I mean. I couldn't find it (it's a complex patch, so I couldn've missed it). As far as I can see, it's only used to store a pointer to TABLE. So it seems to me than a simpler fix for this bug could be: * remove vc_templ caching (mysql_table and mysql_table_query_id) * store TABLE* in upd_node_t. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hello, Sergei! On Thu, 1 Sept 2022 at 15:23, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita,
On Aug 30, Nikita Malyavin wrote:
revision-id: b1b38b64598 (mariadb-10.5.17-4-gb1b38b64598) parent(s): 3b656ac8c17 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2022-08-23 00:27:39 +0300 message:
MDEV-29181 Potential corruption on FK update on a table with vcol index
vc_templ->mysql_table concept is completely broken. This table pointer persists with a global access and can be overwritten by arbitrary thread. Like it wasn't enough, it also could become invalid after eviction from tc cache.
this is a bit misleading. I agree it's completely broken, though :)
But not "this table pointer", because, of course it persists with a global access and can be overwritten and so on. This is by design, it's protected by mysql_table_query_id.
The problem is that table (dict_table_t) argument of innodb_find_table_for_vc() points to a shared data structure, not thread local, as I believed. So vc_templ is shared too.
This new solution simply does the following: * Sets up a referenced table list in TABLE instance (sql_base.cc) * Iterates through it along with dict_table_t::referenced_set (row_upd_check_references_constraints) * Passes corresponding prebuilt through a call chain to row_ins_foreign_check_on_constraint * Sets up newly created upd_node_t::prebuilt field and uses it accordingly is cascade update.
Is this upd_node_t::prebuilt used anywhere? As a prebuilt, I mean. I couldn't find it (it's a complex patch, so I couldn've missed it). As far as I can see, it's only used to store a pointer to TABLE.
So it seems to me than a simpler fix for this bug could be: * remove vc_templ caching (mysql_table and mysql_table_query_id) * store TABLE* in upd_node_t.
Not sure about the lifetimes, so it's not necessarily simpler. prebuilt is not used in this patch, but it is used during online row logging to convert the row to [my]sql
Well, that just wasn't the only problem:) But yes, mentioning it in the commit message is worth it, too. I will think on how to rewrite it better. format. -- Yours truly, Nikita Malyavin
Sergei, I have reworded the commit message, please see it here: https://github.com/MariaDB/server/commit/3a3064e355bac20ed56ae807e790068e16d... On Thu, 1 Sept 2022 at 19:59, Nikita Malyavin <nikitamalyavin@gmail.com> wrote:
Hello, Sergei!
On Thu, 1 Sept 2022 at 15:23, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita,
On Aug 30, Nikita Malyavin wrote:
revision-id: b1b38b64598 (mariadb-10.5.17-4-gb1b38b64598) parent(s): 3b656ac8c17 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2022-08-23 00:27:39 +0300 message:
MDEV-29181 Potential corruption on FK update on a table with vcol index
vc_templ->mysql_table concept is completely broken. This table pointer persists with a global access and can be overwritten by arbitrary thread. Like it wasn't enough, it also could become invalid after eviction from tc cache.
this is a bit misleading. I agree it's completely broken, though :)
But not "this table pointer", because, of course it persists with a global access and can be overwritten and so on. This is by design, it's protected by mysql_table_query_id.
The problem is that table (dict_table_t) argument of innodb_find_table_for_vc() points to a shared data structure, not thread local, as I believed. So vc_templ is shared too.
Well, that just wasn't the only problem:) But yes, mentioning it in the commit message is worth it, too. I will think on how to rewrite it better.
This new solution simply does the following: * Sets up a referenced table list in TABLE instance (sql_base.cc) * Iterates through it along with dict_table_t::referenced_set (row_upd_check_references_constraints) * Passes corresponding prebuilt through a call chain to row_ins_foreign_check_on_constraint * Sets up newly created upd_node_t::prebuilt field and uses it accordingly is cascade update.
Is this upd_node_t::prebuilt used anywhere? As a prebuilt, I mean. I couldn't find it (it's a complex patch, so I couldn've missed it). As far as I can see, it's only used to store a pointer to TABLE.
So it seems to me than a simpler fix for this bug could be: * remove vc_templ caching (mysql_table and mysql_table_query_id) * store TABLE* in upd_node_t.
Not sure about the lifetimes, so it's not necessarily simpler. prebuilt is not used in this patch, but it is used during online row logging to convert the row to [my]sql format.
-- Yours truly, Nikita Malyavin
-- Yours truly, Nikita Malyavin
Hi, Nikita, On Sep 27, Nikita Malyavin wrote:
Sergei, I have reworded the commit message, please see it here: https://github.com/MariaDB/server/commit/3a3064e355bac20ed56ae807e790068e16d...
Same thing. I still cannot understand from the comment what the problem was.
This new solution simply does the following: * Sets up a referenced table list in TABLE instance (sql_base.cc) * Iterates through it along with dict_table_t::referenced_set (row_upd_check_references_constraints) * Passes corresponding prebuilt through a call chain to row_ins_foreign_check_on_constraint * Sets up newly created upd_node_t::prebuilt field and uses it accordingly is cascade update.
Is this upd_node_t::prebuilt used anywhere? As a prebuilt, I mean. I couldn't find it (it's a complex patch, so I couldn've missed it). As far as I can see, it's only used to store a pointer to TABLE.
So it seems to me than a simpler fix for this bug could be: * remove vc_templ caching (mysql_table and mysql_table_query_id) * store TABLE* in upd_node_t.
Not sure about the lifetimes, so it's not necessarily simpler. prebuilt is not used in this patch, but it is used during online row logging to convert the row to [my]sql format.
It is. But it's used as node->prebuilt->m_mysql_table. And I write "store TABLE* in upd_node_t", so that'll work for online alter too. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Nikita, On Sep 01, Sergei Golubchik wrote:
This new solution simply does the following: * Sets up a referenced table list in TABLE instance (sql_base.cc) * Iterates through it along with dict_table_t::referenced_set (row_upd_check_references_constraints) * Passes corresponding prebuilt through a call chain to row_ins_foreign_check_on_constraint * Sets up newly created upd_node_t::prebuilt field and uses it accordingly is cascade update.
Is this upd_node_t::prebuilt used anywhere? As a prebuilt, I mean. I couldn't find it (it's a complex patch, so I couldn've missed it). As far as I can see, it's only used to store a pointer to TABLE.
So it seems to me than a simpler fix for this bug could be: * remove vc_templ caching (mysql_table and mysql_table_query_id) * store TABLE* in upd_node_t.
I've spent more time on this issue, and it looks like there is no structure in InnoDB with the life time till the end of the statement (or, at least, nothing easily usable). So I suggest to keep the mysql_table_query_id/mysql_table caching, but move it from dict_table_t to upd_node_t. The life time will be till the end of the connection, over many statements, that's why mysql_table_query_id will be still needed. Even better solution would be to put mysql_table_query_id/mysql_table pair into ib_vcol_row and store ib_vcol_row in upd_node_t, instead of creating it again for every row, saving on thousands of mallocs. But I'm not sure it'll work for online alter in 10.11. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Sergei, On Sun, Oct 9, 2022 at 2:03 PM Sergei Golubchik <serg@mariadb.org> wrote:
I've spent more time on this issue, and it looks like there is no structure in InnoDB with the life time till the end of the statement (or, at least, nothing easily usable).
Would ha_innobase::m_prebuilt fill that purpose? Its scope is not exactly a statement, because ha_innobase::reset_template() will be invoked not only by ha_innobase::reset() at the end of a statement. As I can tell, it could be close enough. Marko -- Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation
Hi, Marko, On Oct 10, Marko Mäkelä wrote:
On Sun, Oct 9, 2022 at 2:03 PM Sergei Golubchik <serg@mariadb.org> wrote:
I've spent more time on this issue, and it looks like there is no structure in InnoDB with the life time till the end of the statement (or, at least, nothing easily usable).
Would ha_innobase::m_prebuilt fill that purpose? Its scope is not exactly a statement, because ha_innobase::reset_template() will be invoked not only by ha_innobase::reset() at the end of a statement. As I can tell, it could be close enough.
prebuilt is only minimally reset, I thought you wouldn't want anything complex there. And anyway, Nikita's problem is with cascading updates, they don't have their own ha_innobase::m_prebuilt. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (3)
-
Marko Mäkelä -
Nikita Malyavin -
Sergei Golubchik