maria-developers team mailing list archive

[Vicențiu Ciorbaru <vicentiu@xxxxxxxxxxx>]

Hi Sachin!

Comments inline:

On Sun, 4 Dec 2016 at 08:36 <sachin.setiya@xxxxxxxxxxx> wrote:

> revision-id: 27af98805c27c8e0c41dc8c19bf80aefff4c6d3d
> (mariadb-galera-5.5.53-3-g27af988)
> parent(s): 72fd15f7c31aa3e3705ae1b005a3247a985c5bb3
> author: SachinSetiya
> committer: SachinSetiya
>

NIT: To get these to display correctly you should have a space between
Sachin and Setiya.


> timestamp: 2016-12-04 12:05:05 +0530
> message:
>
> MDEV-11479 Improved wsrep_dirty_reads
>
>     Tasks:-
>         Changes in wsrep_dirty_reads variable
>         1.) Global + Session scope (Current: session-only)
>         2.) Allow all commands that do not change data (besides SELECT)
>         3.) Allow prepared Statements that do not change data
>         4.) Works with wsrep_sync_wait enabled
>

You also seemed to have made the variable available as a command line
option. Please specify that as well.


> ---
>  .../suite/galera/r/galera_var_dirty_reads.result   | 29
> +++++++++++++++++++---
>  .../suite/galera/t/galera_var_dirty_reads.test     | 25
> ++++++++++++++++++-
>  .../sys_vars/r/wsrep_dirty_reads_basic.result      | 19 ++++++++++++--
>  .../suite/sys_vars/t/wsrep_dirty_reads_basic.test  | 13 ++++++++--
>  sql/sql_parse.cc                                   | 18 ++++++++++----
>  sql/sys_vars.cc                                    |  4 +--
>  sql/wsrep_mysqld.cc                                |  3 ++-
>  sql/wsrep_mysqld.h                                 |  1 +
>  8 files changed, 96 insertions(+), 16 deletions(-)
>
> diff --git a/mysql-test/suite/galera/r/galera_var_dirty_reads.result
> b/mysql-test/suite/galera/r/galera_var_dirty_reads.result
> index 6a2aa1e..d009c1b 100644
> --- a/mysql-test/suite/galera/r/galera_var_dirty_reads.result
> +++ b/mysql-test/suite/galera/r/galera_var_dirty_reads.result
> @@ -28,9 +28,32 @@ SELECT @@max_allowed_packet;
>  SELECT 2+2 from DUAL;
>  2+2
>  4
> -SELECT sysdate() from DUAL;
> -sysdate()
> -2016-10-28 23:13:06
> +SET @@session.wsrep_dirty_reads=ON;
> +SELECT * FROM t1;
> +i
> +1
> +prepare stmt_show from 'select 1';
> +prepare stmt_select from 'select * from t1';
> +prepare stmt_insert from 'insert into t1 values(1)';
> +execute stmt_show;
> +1
> +1
> +execute stmt_select;
> +i
> +1
> +execute stmt_insert;
> +ERROR 08S01: WSREP has not yet prepared node for application use
> +select * from t1;
> +i
> +1
> +execute stmt_show;
> +1
> +1
> +execute stmt_select;
> +i
> +1
> +execute stmt_insert;
> +ERROR 08S01: WSREP has not yet prepared node for application use
>  SELECT * FROM t1;
>  i
>  1
>

You're not really testing the global aspect of the variable here. You're
only testing the sesssion behaviour of it. Also, test with it both ON and
OFF (when setting it globally). Here's a sample that tests both session and
global behaviour:

--enable_connect_log

SELECT @@expensive_subquery_limit;
SET @@session.expensive_subquery_limit=300;
SELECT @@expensive_subquery_limit;

create user user1;
create user user2;
--connect (con1, localhost, user1,,)
SELECT @@expensive_subquery_limit;
SET @@session.expensive_subquery_limit=300;
SELECT @@expensive_subquery_limit;

connection default;

SELECT @@expensive_subquery_limit;
SET @@global.expensive_subquery_limit=400;
SELECT @@expensive_subquery_limit;

--connect (con2, localhost, user1,,)
SELECT @@expensive_subquery_limit;


connection default;
drop user user1;
drop user user2;

************************************

SELECT @@expensive_subquery_limit;
@@expensive_subquery_limit
100
SET @@session.expensive_subquery_limit=300;
SELECT @@expensive_subquery_limit;
@@expensive_subquery_limit
300
create user user1;
create user user2;
connect  con1, localhost, user1,,;
SELECT @@expensive_subquery_limit;
@@expensive_subquery_limit
100
SET @@session.expensive_subquery_limit=300;
SELECT @@expensive_subquery_limit;
@@expensive_subquery_limit
300
connection default;
SELECT @@expensive_subquery_limit;
@@expensive_subquery_limit
300
SET @@global.expensive_subquery_limit=400;
SELECT @@expensive_subquery_limit;
@@expensive_subquery_limit
300
connect  con2, localhost, user1,,;
SELECT @@expensive_subquery_limit;
@@expensive_subquery_limit
400
connection default;
drop user user1;
drop user user2;

Notice how the last connection has the default value 400 as that's the
global value. For your use case, you'd also have to execute the prepared
statements to see if it works (execute select_stmt).



> diff --git a/mysql-test/suite/galera/t/galera_var_dirty_reads.test
> b/mysql-test/suite/galera/t/galera_var_dirty_reads.test
> index dd7bc88..44931aa 100644
> --- a/mysql-test/suite/galera/t/galera_var_dirty_reads.test
> +++ b/mysql-test/suite/galera/t/galera_var_dirty_reads.test
> @@ -44,7 +44,30 @@ SET @@session.wsrep_dirty_reads=OFF;
>  SELECT 2;
>  SELECT @@max_allowed_packet;
>  SELECT 2+2 from DUAL;
> -SELECT sysdate() from DUAL;
> +
> +#Query which does not change data should be allowed MDEV-11479
> +SET @@session.wsrep_dirty_reads=ON;
> +SELECT * FROM t1;
> +
> +#Prepared statement creation should be allowed MDEV-11479
> +prepare stmt_show from 'select 1';
> +prepare stmt_select from 'select * from t1';
> +prepare stmt_insert from 'insert into t1 values(1)';
> +
> +#Only prepare statement which does not change data should be allowed
> +execute stmt_show;
> +execute stmt_select;
> +--error ER_UNKNOWN_COM_ERROR
> +execute stmt_insert;

+
> +#wsrep_dirty_read should work when wsrep_sync_wait is 1 or non zero
> +#because we already are disconnected , So It does not make any sense
> +#to wait for other nodes
>

I don't think you're testing this correctly. See my comment on the if
statement which is supposed to govern this.


> +select * from t1;
> +execute stmt_show;
> +execute stmt_select;
> +--error ER_UNKNOWN_COM_ERROR
> +execute stmt_insert;
>
>  --disable_query_log
>  --eval SET @@global.wsrep_cluster_address = '$wsrep_cluster_address_saved'
> diff --git a/mysql-test/suite/sys_vars/r/wsrep_dirty_reads_basic.result
> b/mysql-test/suite/sys_vars/r/wsrep_dirty_reads_basic.result
> index d2a62d6..649ac83 100644
> --- a/mysql-test/suite/sys_vars/r/wsrep_dirty_reads_basic.result
> +++ b/mysql-test/suite/sys_vars/r/wsrep_dirty_reads_basic.result
> @@ -5,12 +5,13 @@
>  SET @wsrep_dirty_reads_session_saved = @@session.wsrep_dirty_reads;
>  # default
>  SELECT @@global.wsrep_dirty_reads;
> -ERROR HY000: Variable 'wsrep_dirty_reads' is a SESSION variable
> +@@global.wsrep_dirty_reads
> +0
>  SELECT @@session.wsrep_dirty_reads;
>  @@session.wsrep_dirty_reads
>  0
>
> -# scope and valid values
> +# valid values for session
>  SET @@session.wsrep_dirty_reads=OFF;
>  SELECT @@session.wsrep_dirty_reads;
>  @@session.wsrep_dirty_reads
> @@ -24,6 +25,20 @@ SELECT @@session.wsrep_dirty_reads;
>  @@session.wsrep_dirty_reads
>  0
>
> +# valid values for global
> +SET @@global.wsrep_dirty_reads=OFF;
> +SELECT @@global.wsrep_dirty_reads;
> +@@global.wsrep_dirty_reads
> +0
> +SET @@global.wsrep_dirty_reads=ON;
> +SELECT @@global.wsrep_dirty_reads;
> +@@global.wsrep_dirty_reads
> +1
> +SET @@global.wsrep_dirty_reads=default;
> +SELECT @@global.wsrep_dirty_reads;
> +@@global.wsrep_dirty_reads
> +0
> +
>  # invalid values
>  SET @@session.wsrep_dirty_reads=NULL;
>  ERROR 42000: Variable 'wsrep_dirty_reads' can't be set to the value of
> 'NULL'
>

Test invalid values for global scope as well. It should not change
obviously but since we're writing tests, might as well do that too.


> diff --git a/mysql-test/suite/sys_vars/t/wsrep_dirty_reads_basic.test
> b/mysql-test/suite/sys_vars/t/wsrep_dirty_reads_basic.test
> index a47524f..000c4c4 100644
> --- a/mysql-test/suite/sys_vars/t/wsrep_dirty_reads_basic.test
> +++ b/mysql-test/suite/sys_vars/t/wsrep_dirty_reads_basic.test
> @@ -8,12 +8,12 @@
>  SET @wsrep_dirty_reads_session_saved = @@session.wsrep_dirty_reads;
>
>  --echo # default
> ---error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +
>  SELECT @@global.wsrep_dirty_reads;
>  SELECT @@session.wsrep_dirty_reads;
>
>  --echo
> ---echo # scope and valid values
> +--echo # valid values for session
>  SET @@session.wsrep_dirty_reads=OFF;
>  SELECT @@session.wsrep_dirty_reads;
>  SET @@session.wsrep_dirty_reads=ON;
> @@ -22,6 +22,15 @@ SET @@session.wsrep_dirty_reads=default;
>  SELECT @@session.wsrep_dirty_reads;
>
>  --echo
> +--echo # valid values for global
> +SET @@global.wsrep_dirty_reads=OFF;
> +SELECT @@global.wsrep_dirty_reads;
> +SET @@global.wsrep_dirty_reads=ON;
> +SELECT @@global.wsrep_dirty_reads;
> +SET @@global.wsrep_dirty_reads=default;
> +SELECT @@global.wsrep_dirty_reads;
> +
> +--echo
>  --echo # invalid values
>  --error ER_WRONG_VALUE_FOR_VAR
>  SET @@session.wsrep_dirty_reads=NULL;
> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> index eb4f714..a56d694 100644
> --- a/sql/sql_parse.cc
> +++ b/sql/sql_parse.cc
> @@ -2393,10 +2393,11 @@ mysql_execute_command(THD *thd)
>      */
>      if (thd->variables.wsrep_on && !thd->wsrep_applier && !wsrep_ready &&
>          lex->sql_command != SQLCOM_SET_OPTION &&
> -        !(thd->variables.wsrep_dirty_reads &&
> -          lex->sql_command == SQLCOM_SELECT) &&
> -        !(lex->sql_command == SQLCOM_SELECT &&
> -          !all_tables)                      &&
> +        !(thd->variables.wsrep_dirty_reads    &&
> +        (sql_command_flags[lex->sql_command] &
> +        CF_CHANGES_DATA) == 0)                &&
>

Put this line on one line only, the single & is confusing, or better, turn
it into a small inline function, such as command_changes_data()


> +        !(lex->sql_command == SQLCOM_SELECT   &&
> +          !all_tables)                        &&
>
Can't tell for certain but this condition might not be necessary if you
have the previous one. What happens when you have a stored procedure that
inserts into a table? Does the CF_CHANGES_DATA flag filter out the query or
not?
ex:
select function_with_side_efect(1);
This seems like a good test case that I'm not sure is tested for this
variable anyway. Please try it.



>          !wsrep_is_show_query(lex->sql_command))
>      {
>  #if DIRTY_HACK
> @@ -2512,7 +2513,14 @@ mysql_execute_command(THD *thd)
>    case SQLCOM_SHOW_INDEX_STATS:
>    case SQLCOM_SELECT:
>  #ifdef WITH_WSREP
> -    if (WSREP_CLIENT(thd) && wsrep_sync_wait(thd)) goto error;
> +    /*
> +      wsrep_dirty_read should work when wsrep_sync_wait is 1 or  non-zero
> +      because we already are disconnected. So, it does not make any sense
> +      to wait for other nodes
> +    */
>
>  This here is not correct. This will not execute the select if
wsrep_dirty_reads is true and wsrep_ready is true and wsrep_sync_wait(thd)
is non-zero.
if (WSREP_CLIENT(thd) && thd->variables.wsrep_dirty_reads(wsrep_ready

> +    if (WSREP_CLIENT(thd) && (wsrep_ready ||
> !thd->variables.wsrep_dirty_reads)
> +        && wsrep_sync_wait(thd))
> +        goto error

   case SQLCOM_SHOW_VARIABLES:
>    case SQLCOM_SHOW_CHARSETS:
>    case SQLCOM_SHOW_COLLATIONS:
> diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc
> index bdcedff..0d864f4 100644
> --- a/sql/sys_vars.cc
> +++ b/sql/sys_vars.cc
> @@ -3967,8 +3967,8 @@ static Sys_var_mybool Sys_wsrep_restart_slave(
>
>  static Sys_var_mybool Sys_wsrep_dirty_reads(
>         "wsrep_dirty_reads", "Do not reject SELECT queries even when the
> node "
> -       "is not ready.", SESSION_ONLY(wsrep_dirty_reads), NO_CMD_LINE,
> -       DEFAULT(FALSE), NO_MUTEX_GUARD, NOT_IN_BINLOG);
> +       "is not ready.", SESSION_VAR(wsrep_dirty_reads),

+       CMD_LINE(OPT_ARG), DEFAULT(FALSE), NO_MUTEX_GUARD, NOT_IN_BINLOG);
>

We might need a guard here since global variables can be changed from
multiple places. Not 100% sure about this but that's my intuition. Need to
look this up.


>  #endif /* WITH_WSREP */
>
> diff --git a/sql/wsrep_mysqld.cc b/sql/wsrep_mysqld.cc
> index 48114fc..f21ff92 100644
> --- a/sql/wsrep_mysqld.cc
> +++ b/sql/wsrep_mysqld.cc
> @@ -65,7 +65,8 @@ my_bool wsrep_restart_slave_activated  = 0; // node has
> dropped, and slave
>                                              // restart will be needed
>  my_bool wsrep_slave_UK_checks          = 0; // slave thread does UK checks
>  my_bool wsrep_slave_FK_checks          = 0; // slave thread does FK checks
> -
> +bool wsrep_dirty_reads                 = 0; // is dirty reads allowed for
> +                                            // disconnectd/non major node
>

Typo: disconnectd -> disconnected.
I really hate how the whole file is set up with TRUE/FALSE/0 for either
bool or my_bool. It's so inconsistent. I'd prefer 'true' here to complicate
stuff further. :) To be "consistent" I'd make it a my_bool, but we should
clean up all of this mess at one point.


>  /*
>    Set during the creation of first wsrep applier and rollback threads.
>    Since these threads are critical, abort if the thread creation fails.
> diff --git a/sql/wsrep_mysqld.h b/sql/wsrep_mysqld.h
> index 3fb5189..9b9179a 100644
> --- a/sql/wsrep_mysqld.h
> +++ b/sql/wsrep_mysqld.h
> @@ -85,6 +85,7 @@ extern ulong       wsrep_max_ws_size;
>  extern ulong       wsrep_max_ws_rows;
>  extern const char* wsrep_notify_cmd;
>  extern my_bool     wsrep_certify_nonPK;
> +extern bool        wsrep_dirty_reads;
>

Remember to update the type here too.


>  extern long        wsrep_max_protocol_version;
>  extern long        wsrep_protocol_version;
>  extern ulong       wsrep_forced_binlog_format;