Re: [Maria-developers] 3cbe15bd78c: Fixed core dump in alter table if ADD PARTITION fails
Hi!
On Thu, Apr 16, 2020 at 1:17 PM Sergei Golubchik wrote:
>
> Hi, Michael!
>
> On Apr 13, Michael Widenius wrote:
> > revision-id: 3cbe15bd78c (mariadb-10.5.2-120-g3cbe15bd78c)
> > parent(s): d4d332d196d
> > author: Michael Widenius
> > committer: Michael Widenius
> > timestamp: 2020-04-09 01:37:01 +0300
> > message:
> >
> > Fixed core dump in alter table if ADD PARTITION fails
> >
> > I didn't add a test case as to reproduce this we need to
> > have a failed write in on of the engines. Bug found and bug fix
> > verified while debugging S3 and partitions.
> >
> > ---
> > sql/sql_partition.cc | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc
> > index ef8ef5114a8..4e984fa775d 100644
> > --- a/sql/sql_partition.cc
> > +++ b/sql/sql_partition.cc
> > @@ -6817,7 +6817,8 @@ static void
> > handle_alter_part_error(ALTER_PARTITION_PARAM_TYPE *lpt,
> >DBUG_ENTER("handle_alter_part_error");
> >DBUG_ASSERT(table->m_needs_reopen);
> >
> > - if (close_table)
> > + /* The table may not be open if ha_partition::change_partitions() failed
> > */
> > + if (close_table && !table->file->is_open())
>
> This looks *very* confusing, like you close the table only if it's *not*
> open. But then the whole if() is very confusing, as it closes in both
> branches, so the meaning of "close_table" is totally not clear.
Agree it looks confusing but I am reasonable sure it did stop a crash.
I can't verify that as it was very hard to repeat.
> May be instead of this your fix you'd better cherry-pick
> 9c02b7d6670b069866 from MySQL? It removes close_table completely,
> so I expect it to work for S3 just as you wanted.
I ported the above to MariaDB and I agree it looks this is a better solution.
Regards,
Monty
___
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] 3cbe15bd78c: Fixed core dump in alter table if ADD PARTITION fails
Hi, Michael!
On Apr 13, Michael Widenius wrote:
> revision-id: 3cbe15bd78c (mariadb-10.5.2-120-g3cbe15bd78c)
> parent(s): d4d332d196d
> author: Michael Widenius
> committer: Michael Widenius
> timestamp: 2020-04-09 01:37:01 +0300
> message:
>
> Fixed core dump in alter table if ADD PARTITION fails
>
> I didn't add a test case as to reproduce this we need to
> have a failed write in on of the engines. Bug found and bug fix
> verified while debugging S3 and partitions.
>
> ---
> sql/sql_partition.cc | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc
> index ef8ef5114a8..4e984fa775d 100644
> --- a/sql/sql_partition.cc
> +++ b/sql/sql_partition.cc
> @@ -6817,7 +6817,8 @@ static void
> handle_alter_part_error(ALTER_PARTITION_PARAM_TYPE *lpt,
>DBUG_ENTER("handle_alter_part_error");
>DBUG_ASSERT(table->m_needs_reopen);
>
> - if (close_table)
> + /* The table may not be open if ha_partition::change_partitions() failed */
> + if (close_table && !table->file->is_open())
This looks *very* confusing, like you close the table only if it's *not*
open. But then the whole if() is very confusing, as it closes in both
branches, so the meaning of "close_table" is totally not clear.
May be instead of this your fix you'd better cherry-pick
9c02b7d6670b069866 from MySQL? It removes close_table completely,
so I expect it to work for S3 just as you wanted.
>{
> /*
>All instances of this table needs to be closed.
>
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
