maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

Sergei, I have reworded the commit message, please see it here:
https://github.com/MariaDB/server/commit/3a3064e355bac20ed56ae807e790068e16dd16f3

On Thu, 1 Sept 2022 at 19:59, Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
wrote:

> Hello, Sergei!
>
> On Thu, 1 Sept 2022 at 15:23, Sergei Golubchik <serg@xxxxxxxxxxx> 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