← Back to team overview

maria-developers team mailing list archive

Re: [Commits] 5190ebd: MDEV-11817: Altering a table with more rows than ..

 

Hi Sachin,

On Wed, Jan 25, 2017 at 1:17 AM, Sachin Setiya <sachin.setiya@xxxxxxxxxxx>
wrote:

> Hi Nirbhay!
>
> On Tue, Jan 24, 2017 at 8:02 AM, Nirbhay Choubey <nirbhay@xxxxxxxxxxx>
> wrote:
> >
> > revision-id: 5190ebde3aa142219e1c703df6ea1a78f171182c
> (mariadb-10.1.21-7-g5190ebd)
> > parent(s): 15f46d517435f3570e2c788349637a06d818a619
> > author: Nirbhay Choubey
> > committer: Nirbhay Choubey
> > timestamp: 2017-01-23 21:32:50 -0500
> > message:
> >
> > MDEV-11817: Altering a table with more rows than ..
> >
> > .. wsrep_max_ws_rows causes cluster to break when running
> > Galera cluster in TOI mode
> >
> > During ALTER TABLE, while copying records to temporary
> > table, since the records are not placed into the binary
> > log, wsrep_affected_rows must also not be incremented.
> >
> I think,  you should write a more detailed commit message, So that
> user/developer
> can link this commit to jira task body.
> May be add something like this
> Problem:-
> 1. When doing altering table in galera cluster if table has more
> records than wsrep_max_ws_rows
> it gives error.
> 2. Doing this also breaks replication.
>
> Solution:-
> 1. Your commit message.
> 2. Replication is broke because setting up  wsrep_max_ws_rows is not
> transferred to another cluster.
> And as ALTER command is passed in statement format and it is passed
> much earlier.
> here in Sql_cmd_alter_table::execute(THD *thd)   function.
>
> #ifdef WITH_WSREP
>           TABLE *find_temporary_table(THD *thd, const TABLE_LIST *tl);
>
>           if ((!thd->is_current_stmt_binlog_format_row() ||
>                !find_temporary_table(thd, first_table)))
>           {
>             WSREP_TO_ISOLATION_BEGIN(((lex->name.str) ? select_lex->db
> : NULL),
>                                      ((lex->name.str) ? lex->name.str
> : NULL),
>                                      first_table);
>           }
> this send alter command to another node , before it is executed on node.
> Galera assumes that if alter command is failed on one node it should
> also be failing on other nodes.
>
> Or may be this can be added in comment.
>

IMHO, the above explanation (though correct) seem too verbose for the
change that
the patch introduces. :) I have redone the commit message.


> >
> > ---
> >  .../suite/galera/r/galera_var_max_ws_rows.result   | 23 ++++++++
> >  .../suite/galera/t/galera_var_max_ws_rows.test     | 28 +++++++++-
> >  sql/handler.cc                                     | 61
> ++++++++++++----------
> >  3 files changed, 83 insertions(+), 29 deletions(-)
> >
> > diff --git a/mysql-test/suite/galera/r/galera_var_max_ws_rows.result
> b/mysql-test/suite/galera/r/galera_var_max_ws_rows.result
> > index 6e239c7..555bd1a 100644
> > --- a/mysql-test/suite/galera/r/galera_var_max_ws_rows.result
> > +++ b/mysql-test/suite/galera/r/galera_var_max_ws_rows.result
> > @@ -113,3 +113,26 @@ INSERT INTO t1 (f2) VALUES (2);
> >  ERROR HY000: wsrep_max_ws_rows exceeded
> >  DROP TABLE t1;
> >  DROP TABLE ten;
> > +#
> > +# MDEV-11817: Altering a table with more rows than
> > +# wsrep_max_ws_rows causes cluster to break when running
> > +# Galera cluster in TOI mode
> > +#
> > +CREATE TABLE t1(c1 INT)ENGINE = INNODB;
> > +SET GLOBAL wsrep_max_ws_rows= DEFAULT;
> > +INSERT INTO t1 VALUES(1);
> > +INSERT INTO t1 SELECT * FROM t1;
> > +INSERT INTO t1 SELECT * FROM t1;
> > +INSERT INTO t1 SELECT * FROM t1;
> > +INSERT INTO t1 SELECT * FROM t1;
> > +SET GLOBAL wsrep_max_ws_rows= 10;
> > +ALTER TABLE t1 CHANGE COLUMN c1 c1 BIGINT;
> > +SHOW CREATE TABLE t1;
> > +Table  Create Table
> > +t1     CREATE TABLE `t1` (
> > +  `c1` bigint(20) DEFAULT NULL
> > +) ENGINE=InnoDB DEFAULT CHARSET=latin1
> > +SELECT COUNT(*) FROM t1;
> > +COUNT(*)
> > +16
> > +DROP TABLE t1;
> > diff --git a/mysql-test/suite/galera/t/galera_var_max_ws_rows.test
> b/mysql-test/suite/galera/t/galera_var_max_ws_rows.test
> > index 944238b..4b2ba60 100644
> > --- a/mysql-test/suite/galera/t/galera_var_max_ws_rows.test
> > +++ b/mysql-test/suite/galera/t/galera_var_max_ws_rows.test
> > @@ -146,10 +146,34 @@ INSERT INTO t1 (f2) VALUES (1);
> >  --error ER_ERROR_DURING_COMMIT
> >  INSERT INTO t1 (f2) VALUES (2);
> >
> > +DROP TABLE t1;
> > +DROP TABLE ten;
> > +
> > +--echo #
> > +--echo # MDEV-11817: Altering a table with more rows than
> > +--echo # wsrep_max_ws_rows causes cluster to break when running
> > +--echo # Galera cluster in TOI mode
> > +--echo #
> > +--connection node_1
> > +CREATE TABLE t1(c1 INT)ENGINE = INNODB;
> > +SET GLOBAL wsrep_max_ws_rows= DEFAULT;
> > +INSERT INTO t1 VALUES(1);
> > +INSERT INTO t1 SELECT * FROM t1;
> > +INSERT INTO t1 SELECT * FROM t1;
> > +INSERT INTO t1 SELECT * FROM t1;
> > +INSERT INTO t1 SELECT * FROM t1;
> > +SET GLOBAL wsrep_max_ws_rows= 10;
> > +ALTER TABLE t1 CHANGE COLUMN c1 c1 BIGINT;
> > +
> > +--connection node_2
> > +SHOW CREATE TABLE t1;
> > +SELECT COUNT(*) FROM t1;
> > +DROP TABLE t1;
> > +
> > +--connection node_1
> >
> >  --disable_query_log
> >  --eval SET GLOBAL wsrep_max_ws_rows = $wsrep_max_ws_rows_orig
> >  --enable_query_log
> >
> > -DROP TABLE t1;
> > -DROP TABLE ten;
> > +--source include/galera_end.inc
> > diff --git a/sql/handler.cc b/sql/handler.cc
> > index af06427..48946de 100644
> > --- a/sql/handler.cc
> > +++ b/sql/handler.cc
> > @@ -5724,6 +5724,30 @@ static int write_locked_table_maps(THD *thd)
> >  }
> >
> >
> > +static int check_wsrep_max_ws_rows()
> > +{
> > +#ifdef WITH_WSREP
> > +  if (wsrep_max_ws_rows)
> > +  {
> > +    THD *thd= current_thd;
> > +
> > +    if (!WSREP(thd))
> > +      return 0;
> > +
> > +    thd->wsrep_affected_rows++;
> > +    if (thd->wsrep_exec_mode != REPL_RECV &&
> > +        thd->wsrep_affected_rows > wsrep_max_ws_rows)
> > +    {
> > +      trans_rollback_stmt(thd) || trans_rollback(thd);
> > +      my_message(ER_ERROR_DURING_COMMIT, "wsrep_max_ws_rows exceeded",
> MYF(0));
> > +      return ER_ERROR_DURING_COMMIT;
> > +    }
> > +  }
> > +#endif /* WITH_WSREP */
> > +  return 0;
> > +}
> > +
> > +
> I think  declaring check_wsrep_max_ws_rows() on top would be a much
> better option.
>

Done.

http://lists.askmonty.org/pipermail/commits/2017-January/010555.html

Best,
Nirbhay

Follow ups

References