maria-developers team mailing list archive

[Sachin Setiya <sachin.setiya@xxxxxxxxxxx>]

Hi Nirbhay!

On Sun, Jan 29, 2017 at 11:57 PM, Nirbhay Choubey <nirbhay@xxxxxxxxxxx> wrote:
> 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
>
This , lookes okay to me.

Regards
sachin