Re: 3aea3bd7422: MDEV-23294 Segfault or assertion upon MyISAM repair
Hi, Aleksey, On Oct 23, Aleksey Midenkov wrote:
revision-id: 3aea3bd7422 (mariadb-10.4.30-176-g3aea3bd7422) parent(s): 4fa6383c937 author: Aleksey Midenkov committer: Aleksey Midenkov timestamp: 2023-09-26 20:20:46 +0300 message:
MDEV-23294 Segfault or assertion upon MyISAM repair
When computing vcol expression some items use current_thd and that was not set in MyISAM repair thread. Since all the repair threads belong to one connection and items should not write into THD we can utilize table THD for that.
diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc index ddab8bcf741..27151548704 100644 --- a/storage/myisam/ha_myisam.cc +++ b/storage/myisam/ha_myisam.cc @@ -714,6 +714,8 @@ static int compute_vcols(MI_INFO *info, uchar *record, int keynum) /* This mutex is needed for parallel repair */ mysql_mutex_lock(&info->s->intern_lock); TABLE *table= (TABLE*)(info->external_ref); + if (!current_thd) + set_current_thd(table->in_use);
why not to do it in thr_find_all_keys(), unconditionally? you'd avoid an if() executed per row. Also, I'd suggest a comment, like /* To evaluate vcols we must have current_thd set. This will set current_thd in all threads to the same THD, but it's safe, because vcols are always evaluated under info->s->intern_lock. */ set_current_thd((TABLE*)(info->external_ref)->in_use);
table->move_fields(table->field, record, table->field[0]->record_ptr()); if (keynum == -1) // update all vcols {
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hi Sergei! On Mon, Oct 23, 2023 at 11:46 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey,
On Oct 23, Aleksey Midenkov wrote:
revision-id: 3aea3bd7422 (mariadb-10.4.30-176-g3aea3bd7422) parent(s): 4fa6383c937 author: Aleksey Midenkov committer: Aleksey Midenkov timestamp: 2023-09-26 20:20:46 +0300 message:
MDEV-23294 Segfault or assertion upon MyISAM repair
When computing vcol expression some items use current_thd and that was not set in MyISAM repair thread. Since all the repair threads belong to one connection and items should not write into THD we can utilize table THD for that.
diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc index ddab8bcf741..27151548704 100644 --- a/storage/myisam/ha_myisam.cc +++ b/storage/myisam/ha_myisam.cc @@ -714,6 +714,8 @@ static int compute_vcols(MI_INFO *info, uchar *record, int keynum) /* This mutex is needed for parallel repair */ mysql_mutex_lock(&info->s->intern_lock); TABLE *table= (TABLE*)(info->external_ref); + if (!current_thd) + set_current_thd(table->in_use);
why not to do it in thr_find_all_keys(), unconditionally? you'd avoid an if() executed per row.
It is done only for repair, isn't it, so what are we supposed to optimize by removing this if? I think cleaner code is more important in this case. Putting this to thr_find_all_keys() makes some mess: THD is not the part of MyISAM, thr_find_all_keys() doesn't know anything about virtual keys and should not care. Keeping the logic of SQL layer in one place of storage layer (or as much in one place as possible) would be a wise idea. Of course if we didn't decide that this impacts performance.
Also, I'd suggest a comment, like
/* To evaluate vcols we must have current_thd set. This will set current_thd in all threads to the same THD, but it's safe, because vcols are always evaluated under info->s->intern_lock. */
This is self-explanatory for the code in compute_vcols(). Another argument to keep that in compute_vcols().
set_current_thd((TABLE*)(info->external_ref)->in_use);
table->move_fields(table->field, record, table->field[0]->record_ptr()); if (keynum == -1) // update all vcols {
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
-- @midenok
Hi, Aleksey, On Oct 24, Aleksey Midenkov wrote:
diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc index ddab8bcf741..27151548704 100644 --- a/storage/myisam/ha_myisam.cc +++ b/storage/myisam/ha_myisam.cc @@ -714,6 +714,8 @@ static int compute_vcols(MI_INFO *info, uchar *record, int keynum) /* This mutex is needed for parallel repair */ mysql_mutex_lock(&info->s->intern_lock); TABLE *table= (TABLE*)(info->external_ref); + if (!current_thd) + set_current_thd(table->in_use);
why not to do it in thr_find_all_keys(), unconditionally? you'd avoid an if() executed per row.
It is done only for repair, isn't it, so what are we supposed to optimize by removing this if?
it's done for repair, for *every row* of the table, there can be many millions of rows. It's the condition that will be true for the first row and then it'll be always false. There is simply no reason to reevaluate it for every row.
Also, I'd suggest a comment, like
/* To evaluate vcols we must have current_thd set. This will set current_thd in all threads to the same THD, but it's safe, because vcols are always evaluated under info->s->intern_lock. */
This is self-explanatory for the code in compute_vcols(). Another argument to keep that in compute_vcols().
If it would've been self-explanatory, I wouldn't have asked. Meaning it wasn't self-explanatory to me. Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hi Sergei, On Fri, Nov 3, 2023 at 5:24 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey,
On Oct 24, Aleksey Midenkov wrote:
diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc index ddab8bcf741..27151548704 100644 --- a/storage/myisam/ha_myisam.cc +++ b/storage/myisam/ha_myisam.cc @@ -714,6 +714,8 @@ static int compute_vcols(MI_INFO *info, uchar *record, int keynum) /* This mutex is needed for parallel repair */ mysql_mutex_lock(&info->s->intern_lock); TABLE *table= (TABLE*)(info->external_ref); + if (!current_thd) + set_current_thd(table->in_use);
why not to do it in thr_find_all_keys(), unconditionally? you'd avoid an if() executed per row.
It is done only for repair, isn't it, so what are we supposed to optimize by removing this if?
it's done for repair, for *every row* of the table, there can be many millions of rows. It's the condition that will be true for the first row and then it'll be always false. There is simply no reason to reevaluate it for every row.
I understand this perfectly. There is just no good place for this set_current_thd(), so this was the choice between the two evils. I chosen code cleanliness instead of performance from the 3 facts: 1. This is only for repair operation that should happen less than once a week; 2. This is only for a table that contains virtual columns; 3. Specific weight of this condition is small against the execution of compute_vcols().
Also, I'd suggest a comment, like
/* To evaluate vcols we must have current_thd set. This will set current_thd in all threads to the same THD, but it's safe, because vcols are always evaluated under info->s->intern_lock. */
This is self-explanatory for the code in compute_vcols(). Another argument to keep that in compute_vcols().
If it would've been self-explanatory, I wouldn't have asked. Meaning it wasn't self-explanatory to me.
Ok.
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
-- @midenok
participants (2)
-
Aleksey Midenkov -
Sergei Golubchik