Hi Sergei! The rebased patches are in
bb-10.7-midenok-MDEV-17554 Do we have open questions on review? On Thu, Jun 3, 2021 at 1:52 PM Aleksey Midenkov <[email protected]> wrote: > > Sergei, > > On Thu, Jun 3, 2021 at 12:27 PM Aleksey Midenkov <[email protected]> wrote: > > > > Sergei, > > > > On Thu, Jun 3, 2021 at 11:30 AM Sergei Golubchik <[email protected]> wrote: > > > > > > Hi, Aleksey! > > > > > > On Jun 03, Aleksey Midenkov wrote: > > > > On Wed, Jun 2, 2021 at 11:37 PM Sergei Golubchik <[email protected]> > > > > wrote: > > > > > On Jun 02, Aleksey Midenkov wrote: > > > > > > On Wed, Jun 2, 2021 at 9:06 PM Sergei Golubchik <[email protected]> > > > > > > wrote: > > > > > > > > > > > > > > On Jun 02, Aleksey Midenkov wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > - if (!(sql_lock= (MYSQL_LOCK*) > > > > > > > > > > > > > > - my_malloc(key_memory_MYSQL_LOCK, > > > > > > > > > > > > > > sizeof(*sql_lock) + > > > > > > > > > > > > > > - > > > > > > > > > > > > > > sizeof(THR_LOCK_DATA*)*((a->lock_count+b->lock_count)*2) > > > > > > > > > > > > > > + > > > > > > > > > > > > > > - > > > > > > > > > > > > > > sizeof(TABLE*)*(a->table_count+b->table_count),MYF(MY_WME)))) > > > > > > > > > > > > > > - DBUG_RETURN(0); // > > > > > > > > > > > > > > Fatal error > > > > > > > > > > > > > > + const size_t lock_size= sizeof(*sql_lock) + > > > > > > > > > > > > > > + sizeof(THR_LOCK_DATA *) * ((a->lock_count + > > > > > > > > > > > > > > b->lock_count) * 2) + > > > > > > > > > > > > > > + sizeof(TABLE *) * (a->table_count + > > > > > > > > > > > > > > b->table_count); > > > > > > > > > > > > > > + if (thd) > > > > > > > > > > > > > > + { > > > > > > > > > > > > > > + sql_lock= (MYSQL_LOCK *) thd->alloc(lock_size); > > > > > > > > > > > > > > + if (!sql_lock) > > > > > > > > > > > > > > + DBUG_RETURN(0); > > > > > > > > > > > > > > + sql_lock->flags= GET_LOCK_ON_THD; > > > > > > > > > > > > > > > > > > > > > > > Yes, that is the part of the bug fix. > > > > This was not a bug, this was a new change in recover_from_failed_open(). > > > > > > Hmm, so was it part of the bug fix or not? > > > > The bug was it doesn't work with LTM_PRELOCKED. It was a part of the > > bug fix. There were no bug with lock merging -- it was a new change to > > satisfy that bug fix. > > If you are reviewing MDEV-23639, that part is not yet finished and > fully pushed. Maybe there was a misunderstanding. Please wait until I > reassign that ticket to you. I'll do that along with MDEV-17554 review > changes. That part was finished and the ticket is on you. > > > > > > > > > > > > Also freeing was impossible for locks on thd. > > > > > > > > > > Yes, this change I understand, no questions about freeing. > > > > > > > > > > > > > > > > > > > + /* > > > > > > > > > > > > > > + NOTE: The semantics of vers_set_hist_part() > > > > > > > > > > > > > > is double: > > > > > > > > > > > > > > > > > > > > > > > > > > twofold > > > > > > > > > > Please, fix the language to be proper English. > > > > > > > > Don't you want to ask Ian now? Please look for "double semantics" > > > > collocation in Google query (quotes are important). There are quite a > > > > number of examples including scientific books and IETF drafts. > > > > > > No, I don't. > > > "double semantics" looks good, if you want to change the comment to > > > > > > Note the double semantics of vers_set_hist_part() ... > > > > > > this is fine. > > > > And what's wrong with "is" construct? > > Note the red apple. > > Note the apple is red. > > > > The second one emphasizes the apple IS red. Is "double" some special > > adjective that cannot be used in the second form? > > I changed to "twofold" but please see: https://english.stackexchange.com/questions/570448/ So "is double" was not an error. > > > > > > > > > > > > > > > > > + table_list->vers_skip_create= false; > > > > > > > > > > > > > > + ot_ctx->vers_create_count= 0; > > > > > > > > > > > > > > + action= Open_table_context::OT_REOPEN_TABLES; > > > > > > > > > > > > > > + table_arg= NULL; > > > > > > > > > > > > > > + } > > > > > > > > > > > > > > > > > > > > > > > > > > I'm afraid I don't understand. All this business with > > > > > > > > > > > > > vers_skip_create and vers_skip_auto_create, it wasn't > > > > > > > > > > > > > in > > > > > > > > > > > > > the previous version of the patch. So, I believe it > > > > > > > > > > > > > was > > > > > > > > > > > > > a fix for one of the MDEV bugs reported and fixed > > > > > > > > > > > > > meanwhile. > > > > > > > > > > > > > > > > > > > > > > > > No, that was the multi-threaded case which worked good > > > > > > > > > > > > for > > > > > > > > > > > > me, but suddenly I discovered it fails on some buildbot. > > > > > > > > > > > > > > > > > > > > > > Could you elaborate on what the problem was? Two threads > > > > > > > > > > > trying to add the partition in parallel? I'd expect > > > > > > > > > > > MDL_EXCLUSIVE to prevent that. > > > > > > > > > > > > > > > > > > > > MDL_EXCLUSIVE prevents parallel execution of > > > > > > > > > > repair_from_failed_open(), but not sequential. So it can add > > > > > > > > > > several partitions instead of 1, one after another. > > > > > > > > > > > > > > > > > > What's the sequence of events? One thread decides to add a > > > > > > > > > partition, takes an MDL_EXCLUSIVE, the other thread decides to > > > > > > > > > add a partition, waits for MDL_EXCLUSIVE, the first one > > > > > > > > > finishes > > > > > > > > > adding a partition, releases the lock, the second grabs it and > > > > > > > > > adds a second partition? > > > > > > > > > > > > > > > > Right. > > > > > > > > > > > > > > Okay. Then, why a thread didn't check the number of partitions > > > > > > > after acquiring MDL_EXCLUSIVE? Like with mutexes, if there's a > > > > > > > mutex that protects a shared variable, you first acquire the > > > > > > > mutex, then read the variable's value. Not the other way around. > > > > > > > > > > > > Number of partitions is not a shared variable. part_info is kept in > > > > > > TABLE instance. To get new value it must reacquire share, reparse > > > > > > part_sql string. Then comparing with what? After releasing > > > > > > MDL_SHARED_WRITE everything is gone: TABLE, TABLE_SHARE. You must > > > > > > store somewhere old value, presumably in Open_table_context. > > > > > > > > > > I thought that after acquiring MDL_EXCLUSIVE, just as the thread is > > > > > trying to add a new partition, it could check the conditions if the > > > > > new partition, indeed, needs to be added. > > > > > > > > If it were so easy as it sounds I'd make it. > > > > > > Then, please, help me to understand why it's not easy. > > > > I already said, you will need to reacquire share and reparse part_sql > > string. > > Sorry, I really don't know how to help you more until we pass these > complexities. It is just irrational to do part of open_table() inside > recover_from_failed_open() just to check the new value of num_parts. > -- All the best, Aleksey Midenkov @midenok _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
