Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
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 wrote:
>
> Sergei,
>
> On Thu, Jun 3, 2021 at 12:27 PM Aleksey Midenkov wrote:
> >
> > Sergei,
> >
> > On Thu, Jun 3, 2021 at 11:30 AM Sergei Golubchik wrote:
> > >
> > > Hi, Aleksey!
> > >
> > > On Jun 03, Aleksey Midenkov wrote:
> > > > On Wed, Jun 2, 2021 at 11:37 PM Sergei Golubchik
> > > > wrote:
> > > > > On Jun 02, Aleksey Midenkov wrote:
> > > > > > On Wed, Jun 2, 2021 at 9:06 PM Sergei Golubchik
> > > > > > 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.
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Sergei,
On Thu, Jun 3, 2021 at 12:27 PM Aleksey Midenkov wrote:
>
> Sergei,
>
> On Thu, Jun 3, 2021 at 11:30 AM Sergei Golubchik wrote:
> >
> > Hi, Aleksey!
> >
> > On Jun 03, Aleksey Midenkov wrote:
> > > On Wed, Jun 2, 2021 at 11:37 PM Sergei Golubchik wrote:
> > > > On Jun 02, Aleksey Midenkov wrote:
> > > > > On Wed, Jun 2, 2021 at 9:06 PM Sergei Golubchik
> > > > > 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.
>
> >
> > > > > 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?
>
> >
> > > > > > > > > > > > > + 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.
> >
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Sergei,
On Thu, Jun 3, 2021 at 11:30 AM Sergei Golubchik wrote:
>
> Hi, Aleksey!
>
> On Jun 03, Aleksey Midenkov wrote:
> > On Wed, Jun 2, 2021 at 11:37 PM Sergei Golubchik wrote:
> > > On Jun 02, Aleksey Midenkov wrote:
> > > > On Wed, Jun 2, 2021 at 9:06 PM Sergei Golubchik
> > > > 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.
>
> > > > 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?
>
> > > > > > > > > > > > + 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
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Hi, Aleksey!
On Jun 03, Aleksey Midenkov wrote:
> On Wed, Jun 2, 2021 at 11:37 PM Sergei Golubchik wrote:
> > On Jun 02, Aleksey Midenkov wrote:
> > > On Wed, Jun 2, 2021 at 9:06 PM Sergei Golubchik 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?
> > > 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.
> > > > > > > > > > > + 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.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Sergei,
On Wed, Jun 2, 2021 at 11:37 PM Sergei Golubchik wrote:
>
> Hi, Aleksey!
>
> On Jun 02, Aleksey Midenkov wrote:
> > On Wed, Jun 2, 2021 at 9:06 PM Sergei Golubchik 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;
> > > > > > > > >
> > > > > > Sorry, I meant the moment when MDEV-23639 calls mysql_lock_merge().
> > > > > > Yes, that is the part of the bug fix.
> > > > >
> > > > > What did it fix and how?
> > > > >
> > > > That's not about changing the method, that's about merging locks. When
> > > > I merged my locks with thd there were already thd-allocated locks.
> > >
> > > And? How did allocating locks on THD fix anything?
> >
> > LTM_PRELOCKED had locks on thd, I keep merged locks on thd. Isn't that
> > enough for you?
>
> I'm just trying to understand what the bug was. And I still cannot.
This was not a bug, this was a new change in recover_from_failed_open().
>
> > 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.
>
> > > > > > > > > > + 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.
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and secur...@mariadb.org
--
All the best,
Aleksey Midenkov
@midenok
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Hi, Aleksey!
On Jun 02, Aleksey Midenkov wrote:
> On Wed, Jun 2, 2021 at 9:06 PM Sergei Golubchik 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;
> > > > > > > >
> > > > > Sorry, I meant the moment when MDEV-23639 calls mysql_lock_merge().
> > > > > Yes, that is the part of the bug fix.
> > > >
> > > > What did it fix and how?
> > > >
> > > That's not about changing the method, that's about merging locks. When
> > > I merged my locks with thd there were already thd-allocated locks.
> >
> > And? How did allocating locks on THD fix anything?
>
> LTM_PRELOCKED had locks on thd, I keep merged locks on thd. Isn't that
> enough for you?
I'm just trying to understand what the bug was. And I still cannot.
> 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.
> > > > > > > > > + 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.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Sergei,
On Wed, Jun 2, 2021 at 9:06 PM Sergei Golubchik wrote:
>
> Hi, Aleksey!
>
> 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;
> > > > > > >
> > > > Sorry, I meant the moment when MDEV-23639 calls mysql_lock_merge().
> > > > Yes, that is the part of the bug fix.
> > >
> > > What did it fix and how?
> > >
> > That's not about changing the method, that's about merging locks. When
> > I merged my locks with thd there were already thd-allocated locks.
>
> And? How did allocating locks on THD fix anything?
LTM_PRELOCKED had locks on thd, I keep merged locks on thd. Isn't that
enough for you? Also freeing was impossible for locks on thd.
>
> > > > > > > > + /*
> > > > > > > > + NOTE: The semantics of vers_set_hist_part() is double:
> > > > > > >
> > > > > > > twofold
> > > > > >
> > > > Do you really believe in that butter butterish? :) I mean do we
> > > > need to discuss all sorts of butter? That level of white noise
> > > > should be ignored.
> > >
> > > I'm just trying to avoid incorrect usage of a language.
> >
> > I wish that threshold of incorrectness would be more relaxed.
>
> Sure. If any authoritative source will show that both words are correct
> in this context, I'll readily admit that I'm wrong and will remember it
> for the future. I didn't comment on a whim, it wasn't a matter of taste,
> it was as far as I know an actual incorrect usage of words, an error.
Go ahead and play that game again.
>
> > > > > > > > + 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.
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and secur...@mariadb.org
--
All the best,
Aleksey Midenkov
@midenok
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Hi, Aleksey!
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;
> > > > > >
> > > Sorry, I meant the moment when MDEV-23639 calls mysql_lock_merge().
> > > Yes, that is the part of the bug fix.
> >
> > What did it fix and how?
> >
> That's not about changing the method, that's about merging locks. When
> I merged my locks with thd there were already thd-allocated locks.
And? How did allocating locks on THD fix anything?
> > > > > > > + /*
> > > > > > > + NOTE: The semantics of vers_set_hist_part() is double:
> > > > > >
> > > > > > twofold
> > > > >
> > > Do you really believe in that butter butterish? :) I mean do we
> > > need to discuss all sorts of butter? That level of white noise
> > > should be ignored.
> >
> > I'm just trying to avoid incorrect usage of a language.
>
> I wish that threshold of incorrectness would be more relaxed.
Sure. If any authoritative source will show that both words are correct
in this context, I'll readily admit that I'm wrong and will remember it
for the future. I didn't comment on a whim, it wasn't a matter of taste,
it was as far as I know an actual incorrect usage of words, 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.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Hi, Aleksey!
On Jun 02, Aleksey Midenkov wrote:
> > > > > diff --git a/mysql-test/suite/versioning/t/partition.test
> > > > > b/mysql-test/suite/versioning/t/partition.test
> > > > > --- a/mysql-test/suite/versioning/t/partition.test
> > > > > +++ b/mysql-test/suite/versioning/t/partition.test
> > > > > @@ -1068,13 +1078,456 @@ alter table t1 add partition partitions 2;
> > > > > --replace_result $default_engine DEFAULT_ENGINE
> > > > > show create table t1;
> > > > > alter table t1 add partition partitions 8;
> > > > > +drop table t1;
> > > > > +
> > > > > +--echo #
> > > > > +--echo # End of 10.5 tests
> > > > > +--echo #
> > > > > +
> > > > > +--echo #
> > > > > +--echo # MDEV-17554 Auto-create new partition for system versioned
> > > > > tables with history partitioned by INTERVAL/LIMIT
> > > > > +--echo #
> > > > > +create or replace table t1 (x int) with system versioning
> > > > > +partition by system_time limit 1 auto;
> > > > > +--replace_result $default_engine DEFAULT_ENGINE
> > > > > +show create table t1;
> > > > > +
> > > >
> > > > in the previous version of this patch you had clarifying comments here,
> > > > like
> > > >
> > > > --echo # INSERT, INSERT .. SELECT don't auto-create partitions
> > > >
> > > > or
> > > >
> > > > --echo # Number of partitions goes from 3 to 5
> > > >
> > > > I kind of miss them now, they were helpful
> > >
> > > Yes, they were helpful as is. I removed them like I promised in
> > > the previous thread. Though, Sergei, you are free to add your own
> > > comments in the form you like. I'm really sorry, but nobody won't
> > > tell me what words to use unless I am not able to be clear and
> > > bear the common sense.
> >
> > okay
What about my suggested comments above? Look clear enough?
> > > > > diff --git a/sql/ha_partition.h b/sql/ha_partition.h
> > > > > --- a/sql/ha_partition.h
> > > > > +++ b/sql/ha_partition.h
> > > > > @@ -1607,7 +1607,7 @@ class ha_partition :public handler
> > > > > for (; part_id < part_id_end; ++part_id)
> > > > > {
> > > > >handler *file= m_file[part_id];
> > > > > - DBUG_ASSERT(bitmap_is_set(&(m_part_info->read_partitions),
> > > > > part_id));
> > > > > + bitmap_set_bit(&(m_part_info->read_partitions), part_id);
> > > >
> > > > would be good to have a comment before the method that it's
> > > > only used in vers_set_hist_part(), if (vers_info->limit).
> > > >
> > > > because if it's a generic method to return the number of rows in a
> > > > partition
> > > > (incl. subpartitions) then it's totally not clear why it modifies
> > > > read_partitions bitmap.
> > > >
> > > > or may be you'd want to rename the method to make it look more
> > > > specific for LIMIT partitions in vers_set_hist_part() ?
> > >
> > > Added a comment if it will matter to anyone. No, I prefer generic and
> > > easy names instead of long and capacious ones. I believe the latter
> > > ones are non-productive.
> >
> > I also prefer generic names, but only if the method is generic.
> >
> > I feel that modifying the read_partitions bitmap as a side effect
> > makes this function rather specialized.
>
> Generally there are a lot of complications and exceptions everywhere
> in the world. That is the nature of life. Or that is just a limitation
> of us to not see the higher level of order in the observed entropy. If
> we always mention and signify that, we'll spend too much energy in
> vain. That is called tediousness. But I made a good enough comment for
> the whole function, so I hope that will satisfy both of us.
ok, let's try that.
generally, as far as a big code base is concerned, I'd rather prefer
tediousness over the excitement of multi-hour debugging and discovering
an unexpected side effect of a generic function.
> > > > > diff --git a/sql/lock.cc b/sql/lock.cc
> > > > > --- a/sql/lock.cc
> > > > > +++ b/sql/lock.cc
> > > > > @@ -662,16 +662,28 @@ MYSQL_LOCK *mysql_lock_merge(MYSQL_LOCK
> > > > > *a,MYSQL_LOCK *b)
> > > > >DBUG_PRINT("enter", ("a->lock_count: %u b->lock_count: %u",
> > > > > a->lock_count, b->lock_count));
> > > > >
> > > > > - 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;
> > > >
> > This doesn't answer my question. Was it part of the MDEV-23639 bug
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Sergei,
On Wed, Jun 2, 2021 at 12:06 AM Sergei Golubchik wrote:
>
> Hi, Aleksey!
>
> On Jun 01, Aleksey Midenkov wrote:
> > > > +# Don't auto-create new partition on DELETE HISTORY:
> > > > +set timestamp= unix_timestamp('2000-01-01 00:00:00');
> > > > +create or replace table t (a int) with system versioning
> > > > +partition by system_time interval 1 hour auto;
> > > > +set timestamp= unix_timestamp('2000-01-01 10:00:00');
> > >
> > > perhaps, make it only 02:00:00 to avoid any chance that
> > > a partition won't be created because of max-auto-create limit.
> > > it's possible we put the limit at 10, but 2 is very unlikely.
> > >
> > > or does max-auto-create limit always result in a warning?
> >
> > What is max-auto-create limit and how do we make it 10? Did you mean
> > that condition:
> >
> > if (*create_count == MAX_PARTITIONS - 2)
> > {
> > my_error(ER_TOO_MANY_PARTITIONS_ERROR, MYF(0));
> > return true;
> > }
>
> Yes, that's what I meant. Ok, there will be an error, good.
>
> > > > diff --git a/mysql-test/suite/versioning/r/partition.result
> > > > b/mysql-test/suite/versioning/r/partition.result
> > > > --- a/mysql-test/suite/versioning/r/partition.result
> > > > +++ b/mysql-test/suite/versioning/r/partition.result
> > > > @@ -1236,27 +1270,826 @@ t1 CREATE TABLE `t1` (
> ...
> > > > +Warnings:
> > > > +Warning 4114Versioned table `test`.`t1`: last HISTORY
> > > > partition (`p0`) is out of INTERVAL, need more HISTORY partitions
> > > > +select * from t1 for system_time as of '2000-01-01 01:00:00';
> > > > +x
> > > > +select * from t1 partition (p0) order by x;
> > > > +x
> > > > +440
> > > > +441
> > > > +alter table t1 add partition partitions 3;
> > > > +select * from t1 for system_time as of '2000-01-01 01:00:00';
> > > > +x
> > > > +441
> > > > +select * from t1 partition (p0) order by x;
> > > > +x
> > > > +440
> > >
> > > would be good to have EXPLAINs after the overflow and after the manual
> > > fix.
> > > to show how ALTER changes what partition will be pruned
>
> What about this? Agree/disagree?
Agree.
>
> > > > diff --git a/mysql-test/suite/versioning/t/partition.test
> > > > b/mysql-test/suite/versioning/t/partition.test
> > > > --- a/mysql-test/suite/versioning/t/partition.test
> > > > +++ b/mysql-test/suite/versioning/t/partition.test
> > > > @@ -1068,13 +1078,456 @@ alter table t1 add partition partitions 2;
> > > > --replace_result $default_engine DEFAULT_ENGINE
> > > > show create table t1;
> > > > alter table t1 add partition partitions 8;
> > > > +drop table t1;
> > > > +
> > > > +--echo #
> > > > +--echo # End of 10.5 tests
> > > > +--echo #
> > > > +
> > > > +--echo #
> > > > +--echo # MDEV-17554 Auto-create new partition for system versioned
> > > > tables with history partitioned by INTERVAL/LIMIT
> > > > +--echo #
> > > > +create or replace table t1 (x int) with system versioning
> > > > +partition by system_time limit 1 auto;
> > > > +--replace_result $default_engine DEFAULT_ENGINE
> > > > +show create table t1;
> > > > +
> > >
> > > in the previous version of this patch you had clarifying comments here,
> > > like
> > >
> > > --echo # INSERT, INSERT .. SELECT don't auto-create partitions
> > >
> > > or
> > >
> > > --echo # Number of partitions goes from 3 to 5
> > >
> > > I kind of miss them now, they were helpful
> >
> > Yes, they were helpful as is. I removed them like I promised in the
> > previous thread. Though, Sergei, you are free to add your own comments
> > in the form you like. I'm really sorry, but nobody won't tell me what
> > words to use unless I am not able to be clear and bear the common
> > sense.
>
> okay
>
> > > > diff --git a/sql/ha_partition.h b/sql/ha_partition.h
> > > > --- a/sql/ha_partition.h
> > > > +++ b/sql/ha_partition.h
> > > > @@ -1607,7 +1607,7 @@ class ha_partition :public handler
> > > > for (; part_id < part_id_end; ++part_id)
> > > > {
> > > >handler *file= m_file[part_id];
> > > > - DBUG_ASSERT(bitmap_is_set(&(m_part_info->read_partitions),
> > > > part_id));
> > > > + bitmap_set_bit(&(m_part_info->read_partitions), part_id);
> > >
> > > would be good to have a comment before the method that it's
> > > only used in vers_set_hist_part(), if (vers_info->limit).
> > >
> > > because if it's a generic method to return the number of rows in a
> > > partition
> > > (incl. subpartitions) then it's totally not clear why it modifies
> > > read_partitions bitmap.
> > >
> > > or may be you'd want to rename the method to make it look more
> > > specific for LIMIT partitions in vers_set_hist_part() ?
> >
> > Added a comment if it will matter to anyone. No, I prefer generic and
> > easy names instead of long and capacious ones. I believe the latter
> > ones are non-productive.
>
> I also prefer generic names, but only if the method is generic.
>
> I feel that modifying the read_partitions bitmap as a side effect
> makes this function rather
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Hi, Aleksey!
On Jun 01, Aleksey Midenkov wrote:
> > > +# Don't auto-create new partition on DELETE HISTORY:
> > > +set timestamp= unix_timestamp('2000-01-01 00:00:00');
> > > +create or replace table t (a int) with system versioning
> > > +partition by system_time interval 1 hour auto;
> > > +set timestamp= unix_timestamp('2000-01-01 10:00:00');
> >
> > perhaps, make it only 02:00:00 to avoid any chance that
> > a partition won't be created because of max-auto-create limit.
> > it's possible we put the limit at 10, but 2 is very unlikely.
> >
> > or does max-auto-create limit always result in a warning?
>
> What is max-auto-create limit and how do we make it 10? Did you mean
> that condition:
>
> if (*create_count == MAX_PARTITIONS - 2)
> {
> my_error(ER_TOO_MANY_PARTITIONS_ERROR, MYF(0));
> return true;
> }
Yes, that's what I meant. Ok, there will be an error, good.
> > > diff --git a/mysql-test/suite/versioning/r/partition.result
> > > b/mysql-test/suite/versioning/r/partition.result
> > > --- a/mysql-test/suite/versioning/r/partition.result
> > > +++ b/mysql-test/suite/versioning/r/partition.result
> > > @@ -1236,27 +1270,826 @@ t1 CREATE TABLE `t1` (
...
> > > +Warnings:
> > > +Warning 4114Versioned table `test`.`t1`: last HISTORY partition
> > > (`p0`) is out of INTERVAL, need more HISTORY partitions
> > > +select * from t1 for system_time as of '2000-01-01 01:00:00';
> > > +x
> > > +select * from t1 partition (p0) order by x;
> > > +x
> > > +440
> > > +441
> > > +alter table t1 add partition partitions 3;
> > > +select * from t1 for system_time as of '2000-01-01 01:00:00';
> > > +x
> > > +441
> > > +select * from t1 partition (p0) order by x;
> > > +x
> > > +440
> >
> > would be good to have EXPLAINs after the overflow and after the manual fix.
> > to show how ALTER changes what partition will be pruned
What about this? Agree/disagree?
> > > diff --git a/mysql-test/suite/versioning/t/partition.test
> > > b/mysql-test/suite/versioning/t/partition.test
> > > --- a/mysql-test/suite/versioning/t/partition.test
> > > +++ b/mysql-test/suite/versioning/t/partition.test
> > > @@ -1068,13 +1078,456 @@ alter table t1 add partition partitions 2;
> > > --replace_result $default_engine DEFAULT_ENGINE
> > > show create table t1;
> > > alter table t1 add partition partitions 8;
> > > +drop table t1;
> > > +
> > > +--echo #
> > > +--echo # End of 10.5 tests
> > > +--echo #
> > > +
> > > +--echo #
> > > +--echo # MDEV-17554 Auto-create new partition for system versioned
> > > tables with history partitioned by INTERVAL/LIMIT
> > > +--echo #
> > > +create or replace table t1 (x int) with system versioning
> > > +partition by system_time limit 1 auto;
> > > +--replace_result $default_engine DEFAULT_ENGINE
> > > +show create table t1;
> > > +
> >
> > in the previous version of this patch you had clarifying comments here, like
> >
> > --echo # INSERT, INSERT .. SELECT don't auto-create partitions
> >
> > or
> >
> > --echo # Number of partitions goes from 3 to 5
> >
> > I kind of miss them now, they were helpful
>
> Yes, they were helpful as is. I removed them like I promised in the
> previous thread. Though, Sergei, you are free to add your own comments
> in the form you like. I'm really sorry, but nobody won't tell me what
> words to use unless I am not able to be clear and bear the common
> sense.
okay
> > > diff --git a/sql/ha_partition.h b/sql/ha_partition.h
> > > --- a/sql/ha_partition.h
> > > +++ b/sql/ha_partition.h
> > > @@ -1607,7 +1607,7 @@ class ha_partition :public handler
> > > for (; part_id < part_id_end; ++part_id)
> > > {
> > >handler *file= m_file[part_id];
> > > - DBUG_ASSERT(bitmap_is_set(&(m_part_info->read_partitions),
> > > part_id));
> > > + bitmap_set_bit(&(m_part_info->read_partitions), part_id);
> >
> > would be good to have a comment before the method that it's
> > only used in vers_set_hist_part(), if (vers_info->limit).
> >
> > because if it's a generic method to return the number of rows in a partition
> > (incl. subpartitions) then it's totally not clear why it modifies
> > read_partitions bitmap.
> >
> > or may be you'd want to rename the method to make it look more
> > specific for LIMIT partitions in vers_set_hist_part() ?
>
> Added a comment if it will matter to anyone. No, I prefer generic and
> easy names instead of long and capacious ones. I believe the latter
> ones are non-productive.
I also prefer generic names, but only if the method is generic.
I feel that modifying the read_partitions bitmap as a side effect
makes this function rather specialized.
> > > diff --git a/sql/lock.cc b/sql/lock.cc
> > > --- a/sql/lock.cc
> > > +++ b/sql/lock.cc
> > > @@ -662,16 +662,28 @@ MYSQL_LOCK *mysql_lock_merge(MYSQL_LOCK
> > > *a,MYSQL_LOCK *b)
> > >DBUG_PRINT("enter", ("a->lock_count: %u b->lock_count: %u",
> > > a->lock_count, b->lock_count));
> >
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Hi Sergei!
I'm sending the reply but still didn't push the changes. Please expect
the changes pushed when I reassign MDEV-17554 to you.
On Sun, May 30, 2021 at 11:41 PM Sergei Golubchik wrote:
>
> Hi, Aleksey!
>
> Like last time, it's a review of the diff bc04ded2353 22158b3c05a,
> so not only commit 38a888da0f1.
>
> The main thing below - I didn't understand a couple of changes and asked
> for clarifications.
>
> On May 30, Aleksey Midenkov wrote:
> > revision-id: 38a888da0f1 (mariadb-10.5.2-653-g38a888da0f1)
> > parent(s): bc04ded2353
> > author: Aleksey Midenkov
> > committer: Aleksey Midenkov
> > timestamp: 2021-04-22 23:35:06 +0300
> > message:
> >
> > MDEV-17554 Auto-create new partition for system versioned tables with
> > history partitioned by INTERVAL/LIMIT
>
>
> > diff --git a/mysql-test/suite/versioning/r/delete_history.result
> > b/mysql-test/suite/versioning/r/delete_history.result
> > --- a/mysql-test/suite/versioning/r/delete_history.result
> > +++ b/mysql-test/suite/versioning/r/delete_history.result
> > @@ -154,3 +152,21 @@ select * from t1;
> > a
> > 1
> > drop table t1;
> > +#
> > +# MDEV-17554 Auto-create new partition for system versioned tables with
> > history partitioned by INTERVAL/LIMIT
> > +#
> > +# Don't auto-create new partition on DELETE HISTORY:
> > +set timestamp= unix_timestamp('2000-01-01 00:00:00');
> > +create or replace table t (a int) with system versioning
> > +partition by system_time interval 1 hour auto;
> > +set timestamp= unix_timestamp('2000-01-01 10:00:00');
>
> perhaps, make it only 02:00:00 to avoid any chance that
> a partition won't be created because of max-auto-create limit.
> it's possible we put the limit at 10, but 2 is very unlikely.
>
> or does max-auto-create limit always result in a warning?
>
What is max-auto-create limit and how do we make it 10? Did you mean
that condition:
if (*create_count == MAX_PARTITIONS - 2)
{
my_error(ER_TOO_MANY_PARTITIONS_ERROR, MYF(0));
return true;
}
> > +delete history from t;
> > +set timestamp= default;
> > +show create table t;
> > +TableCreate Table
> > +tCREATE TABLE `t` (
> > + `a` int(11) DEFAULT NULL
> > +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING
> > + PARTITION BY SYSTEM_TIME INTERVAL 1 HOUR STARTS TIMESTAMP'2000-01-01
> > 00:00:00' AUTO
> > +PARTITIONS 2
> > +drop table t;
> > diff --git a/mysql-test/suite/versioning/r/partition.result
> > b/mysql-test/suite/versioning/r/partition.result
> > --- a/mysql-test/suite/versioning/r/partition.result
> > +++ b/mysql-test/suite/versioning/r/partition.result
> > @@ -1236,27 +1270,826 @@ t1 CREATE TABLE `t1` (
> > PARTITION `p5` HISTORY ENGINE = DEFAULT_ENGINE,
> > PARTITION `pn` CURRENT ENGINE = DEFAULT_ENGINE)
> > alter table t1 add partition partitions 8;
> > +drop table t1;
> > +#
> > +# End of 10.5 tests
> > +#
> > +#
> > +# MDEV-17554 Auto-create new partition for system versioned tables with
> > history partitioned by INTERVAL/LIMIT
> > +#
> > +create or replace table t1 (x int) with system versioning
> > +partition by system_time limit 1 auto;
> > show create table t1;
> > TableCreate Table
> > t1 CREATE TABLE `t1` (
> >`x` int(11) DEFAULT NULL
> > ) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING
> > - PARTITION BY SYSTEM_TIME LIMIT 1
> > -(PARTITION `p2` HISTORY ENGINE = DEFAULT_ENGINE,
> > - PARTITION `p8` HISTORY ENGINE = DEFAULT_ENGINE,
> > + PARTITION BY SYSTEM_TIME LIMIT 1 AUTO
> > +PARTITIONS 2
> > +insert into t1 values (1);
> > +create or replace table t2 (y int);
> > +insert into t2 values (2);
> > +insert into t1 select * from t2;
> > +insert into t2 select * from t1;
> > +show create table t1;
> > +TableCreate Table
> > +t1 CREATE TABLE `t1` (
> > + `x` int(11) DEFAULT NULL
> > +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING
> > + PARTITION BY SYSTEM_TIME LIMIT 1 AUTO
> > +PARTITIONS 2
> > +# Too many partitions error
> > +set timestamp= unix_timestamp('2000-01-01 00:00:00');
> > +create or replace table t1 (x int) with system versioning
> > +partition by system_time interval 1 hour auto;
> > +set timestamp= unix_timestamp('2001-01-01 00:01:00');
> > +update t1 set x= x + 1;
> > +ERROR HY000: Too many partitions (including subpartitions) were defined
> > +# Partition overflow error and manual fix
>
> seems to be a wrong comment? no manual fix here.
Fixed.
>
> > +set timestamp= unix_timestamp('2000-01-01 00:00:00');
> > +create or replace table t1 (x int) with system versioning
> > +partition by system_time interval 1 hour;
> > +insert into t1 values (333);
> > +set timestamp= unix_timestamp('2000-01-01 01:00:00');
> > +update t1 set x= x + 1;
> > +Warnings:
> > +Warning 4114Versioned table `test`.`t1`: last HISTORY partition
> > (`p0`) is out of INTERVAL, need more HISTORY partitions
> > +drop table t1;
> > +# Partition overflow error and
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Hi, Aleksey!
Like last time, it's a review of the diff bc04ded2353 22158b3c05a,
so not only commit 38a888da0f1.
The main thing below - I didn't understand a couple of changes and asked
for clarifications.
On May 30, Aleksey Midenkov wrote:
> revision-id: 38a888da0f1 (mariadb-10.5.2-653-g38a888da0f1)
> parent(s): bc04ded2353
> author: Aleksey Midenkov
> committer: Aleksey Midenkov
> timestamp: 2021-04-22 23:35:06 +0300
> message:
>
> MDEV-17554 Auto-create new partition for system versioned tables with history
> partitioned by INTERVAL/LIMIT
> diff --git a/mysql-test/suite/versioning/r/delete_history.result
> b/mysql-test/suite/versioning/r/delete_history.result
> --- a/mysql-test/suite/versioning/r/delete_history.result
> +++ b/mysql-test/suite/versioning/r/delete_history.result
> @@ -154,3 +152,21 @@ select * from t1;
> a
> 1
> drop table t1;
> +#
> +# MDEV-17554 Auto-create new partition for system versioned tables with
> history partitioned by INTERVAL/LIMIT
> +#
> +# Don't auto-create new partition on DELETE HISTORY:
> +set timestamp= unix_timestamp('2000-01-01 00:00:00');
> +create or replace table t (a int) with system versioning
> +partition by system_time interval 1 hour auto;
> +set timestamp= unix_timestamp('2000-01-01 10:00:00');
perhaps, make it only 02:00:00 to avoid any chance that
a partition won't be created because of max-auto-create limit.
it's possible we put the limit at 10, but 2 is very unlikely.
or does max-auto-create limit always result in a warning?
> +delete history from t;
> +set timestamp= default;
> +show create table t;
> +TableCreate Table
> +tCREATE TABLE `t` (
> + `a` int(11) DEFAULT NULL
> +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING
> + PARTITION BY SYSTEM_TIME INTERVAL 1 HOUR STARTS TIMESTAMP'2000-01-01
> 00:00:00' AUTO
> +PARTITIONS 2
> +drop table t;
> diff --git a/mysql-test/suite/versioning/r/partition.result
> b/mysql-test/suite/versioning/r/partition.result
> --- a/mysql-test/suite/versioning/r/partition.result
> +++ b/mysql-test/suite/versioning/r/partition.result
> @@ -1236,27 +1270,826 @@ t1 CREATE TABLE `t1` (
> PARTITION `p5` HISTORY ENGINE = DEFAULT_ENGINE,
> PARTITION `pn` CURRENT ENGINE = DEFAULT_ENGINE)
> alter table t1 add partition partitions 8;
> +drop table t1;
> +#
> +# End of 10.5 tests
> +#
> +#
> +# MDEV-17554 Auto-create new partition for system versioned tables with
> history partitioned by INTERVAL/LIMIT
> +#
> +create or replace table t1 (x int) with system versioning
> +partition by system_time limit 1 auto;
> show create table t1;
> TableCreate Table
> t1 CREATE TABLE `t1` (
>`x` int(11) DEFAULT NULL
> ) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING
> - PARTITION BY SYSTEM_TIME LIMIT 1
> -(PARTITION `p2` HISTORY ENGINE = DEFAULT_ENGINE,
> - PARTITION `p8` HISTORY ENGINE = DEFAULT_ENGINE,
> + PARTITION BY SYSTEM_TIME LIMIT 1 AUTO
> +PARTITIONS 2
> +insert into t1 values (1);
> +create or replace table t2 (y int);
> +insert into t2 values (2);
> +insert into t1 select * from t2;
> +insert into t2 select * from t1;
> +show create table t1;
> +TableCreate Table
> +t1 CREATE TABLE `t1` (
> + `x` int(11) DEFAULT NULL
> +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING
> + PARTITION BY SYSTEM_TIME LIMIT 1 AUTO
> +PARTITIONS 2
> +# Too many partitions error
> +set timestamp= unix_timestamp('2000-01-01 00:00:00');
> +create or replace table t1 (x int) with system versioning
> +partition by system_time interval 1 hour auto;
> +set timestamp= unix_timestamp('2001-01-01 00:01:00');
> +update t1 set x= x + 1;
> +ERROR HY000: Too many partitions (including subpartitions) were defined
> +# Partition overflow error and manual fix
seems to be a wrong comment? no manual fix here.
> +set timestamp= unix_timestamp('2000-01-01 00:00:00');
> +create or replace table t1 (x int) with system versioning
> +partition by system_time interval 1 hour;
> +insert into t1 values (333);
> +set timestamp= unix_timestamp('2000-01-01 01:00:00');
> +update t1 set x= x + 1;
> +Warnings:
> +Warning 4114Versioned table `test`.`t1`: last HISTORY partition
> (`p0`) is out of INTERVAL, need more HISTORY partitions
> +drop table t1;
> +# Partition overflow error and manual fix
> +set timestamp= unix_timestamp('2000-01-01 00:00:00');
> +create or replace table t1 (x int) with system versioning
> +partition by system_time interval 1 hour;
> +insert into t1 values (440);
> +set timestamp= unix_timestamp('2000-01-01 00:10:00');
> +update t1 set x= x + 1;
> +# Check how pruning boundaries work
> +explain partitions select * from t1 for system_time as of '2000-01-01
> 00:59:58';
> +id select_type table partitions typepossible_keys key
> key_len ref rowsExtra
> +1SIMPLE t1 p0,pn # NULLNULLNULLNULL#
> #
> +explain partitions select * from t1 for system_time as of
