maria-developers team mailing list archive

[Sachin Setiya <sachin.setiya@xxxxxxxxxxx>]

Hi  Nirbhay!

On Wed, Jan 25, 2017 at 7:53 PM, Nirbhay Choubey <nirbhay@xxxxxxxxxxx> wrote:
> 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.
Changed.
>
>>
>>
>> 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.
>
>
Yes, also Because Initial prepared statement test was written because,
I thought we will hit
COM_STMT_CLOSE, but is simple not possible using mtr.
Test changed.
>>
>> 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 */
>
Changed.
>
>  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.
>
Added.
> 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
>
>

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

--
Regards
Sachin Setiya
Software Engineer at  MariaDB


-- 
Regards
Sachin Setiya
Software Engineer at  MariaDB