maria-developers team mailing list archive

[Nirbhay Choubey <nirbhay@xxxxxxxxxxx>]

Hi Sachin,

My comments below.

On Sun, Jan 22, 2017 at 7:18 AM, SachinSetiya <sachin.setiya@xxxxxxxxxxx>
wrote:

> revision-id: d1124c98cbede6366e4b4d27a23dcf5b9d207cf0
> (mariadb-10.1.21-2-gd1124c9)
> parent(s): a14638581b4c8ef175e68dccff07967d819b3b7e
> author: Sachin Setiya
> committer: Sachin Setiya
> timestamp: 2017-01-22 17:47:29 +0530
> message:
>
> MDEV-10812 WSREP causes responses being sent to protocol commands that
> must not send a response
>
> Problem:- When using wsrep (w/ galera) and issuing commands that can cause
> deadlocks, deadlock exception errors are sent in responses to commands
> such as close prepared statement () which, by spec, must not send a
>

s/close prepared statement ()/close prepared statement and close connection/

response.
>
> Solution:- We will not jump to dispatch_end: when there is deadlock and
> query is either COM_QUIT or COM_STMT_CLOSE.
>

Or,  I would say, in dispatch_command, we handle COM_QUIT and COM_STMT_CLOSE
commands even in case of error.


>
> Patch Credit:- Jaka Močnik
>
> ---
>  .../galera/r/galera_prepared_statement.result      | 18 ++++++++++++++++
>  .../suite/galera/t/galera_prepared_statement.test  | 25
> ++++++++++++++++++++++
>  sql/sql_parse.cc                                   |  6 +++++-
>  3 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/mysql-test/suite/galera/r/galera_prepared_statement.result
> b/mysql-test/suite/galera/r/galera_prepared_statement.result
> index de5ac9c..3425648 100644
> --- a/mysql-test/suite/galera/r/galera_prepared_statement.result
> +++ b/mysql-test/suite/galera/r/galera_prepared_statement.result
> @@ -27,7 +27,25 @@ ALTER TABLE t1 ADD COLUMN f2 INTEGER;
>  ALTER TABLE t1 DROP COLUMN f1;
>  EXECUTE st1;
>  ERROR 22007: Incorrect integer value: 'abc' for column 'f2' at row 1
> +SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
> +CREATE TABLE t5(a int primary key);
> +INSERT INTO t5 VALUES(1),(2),(3);
> +START TRANSACTION ;
> +PREPARE st5 FROM 'SELECT * FROM t5';
> +EXECUTE st5;
> +a
> +1
> +2
> +3
> +update t5 set a=a+100;
> +EXECUTE st5;
> +a
> +101
> +102
> +103
> +update t5 set a=a+100;
>  DROP TABLE t1;
>  DROP TABLE t2;
>  DROP TABLE t3;
>  DROP TABLE t4;
> +DROP TABLE t5;

diff --git a/mysql-test/suite/galera/t/galera_prepared_statement.test
> b/mysql-test/suite/galera/t/galera_prepared_statement.test
> index 3bee097..debc00e 100644
> --- a/mysql-test/suite/galera/t/galera_prepared_statement.test
> +++ b/mysql-test/suite/galera/t/galera_prepared_statement.test
> @@ -38,8 +38,33 @@ ALTER TABLE t1 DROP COLUMN f1;
>  --error ER_TRUNCATED_WRONG_VALUE_FOR_FIELD
>  EXECUTE st1;
>
> +#  MDEV-10812
> +#  On Closing Prepare Stamemnt, When wsrep_conflict_state is ABORTED
> +#  It causes wrong response to be sent on Clients.
> +
> +#  First Create a Deadlock
>  --connection node_1
> +SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
> +CREATE TABLE t5(a int primary key);
> +INSERT INTO t5 VALUES(1),(2),(3);
> +START TRANSACTION ;
> +PREPARE st5 FROM 'SELECT * FROM t5';
> +EXECUTE st5;
> +update t5 set a=a+100;
> +EXECUTE st5;
> +
> +--sleep 2
> +--connection node_2
> +update t5 set a=a+100;
> +
> +--sleep 2
> +--connection node_1
> +# here we have deadlock
> +--disconnect node_1
> +
> +--connection node_2
>  DROP TABLE t1;
>  DROP TABLE t2;
>  DROP TABLE t3;
>  DROP TABLE t4;
> +DROP TABLE t5;
>


The test always hits command != COM_QUIT assert if I revert :

+    if (thd->wsrep_conflict_state == ABORTED &&
+        command != COM_STMT_CLOSE && command != COM_QUIT)

So, I modified your test a bit (simplified) to not use PS.

--echo #
--echo # MDEV-10812: On closing prepare stamemnt, when wsrep_conflict_state
--echo # is ABORTED, it causes wrong response to be sent to the client
--echo #

#  First create a deadlock
--connect node_1a, 127.0.0.1, root, , test, $NODE_MYPORT_1
SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
CREATE TABLE t1(a INT PRIMARY KEY);
INSERT INTO t1 VALUES(1),(2),(3);
START TRANSACTION ;
UPDATE t1 SET a=a+100;

--sleep 2
--connection node_2
UPDATE t1 SET a=a+100;

--sleep 2
--connection node_1a
# here we get deadlock error
--disconnect node_1a

--connection node_2
DROP TABLE t1;

I think it should be ok.



> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> index 033e88a..d45b196 100644
> --- a/sql/sql_parse.cc
> +++ b/sql/sql_parse.cc
> @@ -1265,7 +1265,9 @@ bool dispatch_command(enum enum_server_command
> command, THD *thd,
>      {
>        wsrep_client_rollback(thd);
>      }
> -    if (thd->wsrep_conflict_state== ABORTED)
> +    /* we let COM_QUIT and STMT_CLOSE execute even if wsrep aborted */
>

 We let COM_QUIT and COM_STMT_CLOSE to execute even if wsrep aborted.

+    if (thd->wsrep_conflict_state == ABORTED &&
> +        command != COM_STMT_CLOSE && command != COM_QUIT)
>      {
>        my_error(ER_LOCK_DEADLOCK, MYF(0), "wsrep aborted transaction");
>        WSREP_DEBUG("Deadlock error for: %s", thd->query());
> @@ -1918,6 +1920,8 @@ bool dispatch_command(enum enum_server_command
> command, THD *thd,
>
>    if (WSREP(thd))
>    {
> +    DBUG_ASSERT((command != COM_QUIT && command != COM_STMT_CLOSE)
> +                || thd->get_stmt_da()->is_disabled());
>

Please add a comment explaining this assert.

Thanks,
Nirbhay


>      /* wsrep BF abort in query exec phase */
>      mysql_mutex_lock(&thd->LOCK_wsrep_thd);
>      do_end_of_statement= thd->wsrep_conflict_state != REPLAYING &&
> _______________________________________________
> commits mailing list
> commits@xxxxxxxxxxx
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits