Re: [Maria-developers] [Commits] 5190ebd: MDEV-11817: Altering a table with more rows than ..
Hi Nirbhay! On Tue, Jan 24, 2017 at 8:02 AM, Nirbhay Choubey <nirbhay@mariadb.com> 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.
--- .../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.
typedef bool Log_func(THD*, TABLE*, bool, const uchar*, const uchar*);
static int binlog_log_row(TABLE* table, @@ -5765,6 +5789,13 @@ static int binlog_log_row(TABLE* table, bool const has_trans= thd->lex->sql_command == SQLCOM_CREATE_TABLE || table->file->has_transactions(); error= (*log_func)(thd, table, has_trans, before_record, after_record); + + /* + Now that the record has been logged, increment wsrep_affected_rows and + also check whether its within the allowable limits (wsrep_max_ws_rows). + */ + if (error == 0) + error= check_wsrep_max_ws_rows(); } } return error ? HA_ERR_RBR_LOGGING_FAILED : 0; @@ -5875,30 +5906,6 @@ int handler::ha_reset() }
-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; -} - - int handler::ha_write_row(uchar *buf) { int error; @@ -5923,7 +5930,7 @@ int handler::ha_write_row(uchar *buf) DBUG_RETURN(error); /* purecov: inspected */
DEBUG_SYNC_C("ha_write_row_end"); - DBUG_RETURN(check_wsrep_max_ws_rows()); + DBUG_RETURN(0); }
@@ -5954,7 +5961,7 @@ int handler::ha_update_row(const uchar *old_data, uchar *new_data) rows_changed++; if (unlikely(error= binlog_log_row(table, old_data, new_data, log_func))) return error; - return check_wsrep_max_ws_rows(); + return 0; }
int handler::ha_delete_row(const uchar *buf) @@ -5981,7 +5988,7 @@ int handler::ha_delete_row(const uchar *buf) rows_changed++; if (unlikely(error= binlog_log_row(table, buf, 0, log_func))) return error; - return check_wsrep_max_ws_rows(); + return 0; }
_______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
-- Regards Sachin Setiya Software Engineer at MariaDB
Hi Sachin, On Wed, Jan 25, 2017 at 1:17 AM, Sachin Setiya <sachin.setiya@mariadb.com> wrote:
Hi Nirbhay!
On Tue, Jan 24, 2017 at 8:02 AM, Nirbhay Choubey <nirbhay@mariadb.com> 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
Hi Nirbhay! On Sun, Jan 29, 2017 at 11:57 PM, Nirbhay Choubey <nirbhay@mariadb.com> wrote:
Hi Sachin,
On Wed, Jan 25, 2017 at 1:17 AM, Sachin Setiya <sachin.setiya@mariadb.com> wrote:
Hi Nirbhay!
On Tue, Jan 24, 2017 at 8:02 AM, Nirbhay Choubey <nirbhay@mariadb.com> 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
participants (2)
-
Nirbhay Choubey -
Sachin Setiya