Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for more session variables #3731 #3761 #2702 #3754

Merged
merged 7 commits into from
Apr 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/mysql_connection.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class MySQL_Connection {
uint32_t var_hash[SQL_NAME_LAST_HIGH_WM];
// for now we store possibly missing variables in the lower range
// we may need to fix that, but this will cost performance
bool var_absent[SQL_NAME_LAST_LOW_WM] = {false};
bool var_absent[SQL_NAME_LAST_HIGH_WM] = {false};

std::vector<uint32_t> dynamic_variables_idx;
unsigned int reorder_dynamic_variables_idx();
Expand Down
8 changes: 8 additions & 0 deletions include/proxysql_structs.h
Original file line number Diff line number Diff line change
Expand Up @@ -166,13 +166,15 @@ enum mysql_variable_name {
SQL_COLLATION_CONNECTION,
SQL_WSREP_SYNC_WAIT,
SQL_NAME_LAST_LOW_WM,
SQL_AURORA_READ_REPLICA_READ_COMMITTED,
SQL_AUTO_INCREMENT_INCREMENT,
SQL_AUTO_INCREMENT_OFFSET,
SQL_BIG_TABLES,
SQL_DEFAULT_STORAGE_ENGINE,
SQL_DEFAULT_TMP_STORAGE_ENGINE,
SQL_FOREIGN_KEY_CHECKS,
SQL_GROUP_CONCAT_MAX_LEN,
SQL_GROUP_REPLICATION_CONSISTENCY,
SQL_GTID_NEXT,
SQL_INNODB_LOCK_WAIT_TIMEOUT,
SQL_INNODB_STRICT_MODE,
Expand All @@ -190,6 +192,7 @@ enum mysql_variable_name {
SQL_OPTIMIZER_SEARCH_DEPTH,
SQL_OPTIMIZER_SWITCH,
SQL_PROFILING,
SQL_QUERY_CACHE_TYPE,
SQL_SORT_BUFFER_SIZE,
SQL_SQL_AUTO_IS_NULL,
SQL_SQL_BIG_SELECTS,
Expand All @@ -201,6 +204,7 @@ enum mysql_variable_name {
SQL_TIMESTAMP,
SQL_TMP_TABLE_SIZE,
SQL_UNIQUE_CHECKS,
SQL_WSREP_OSU_METHOD,
SQL_NAME_LAST_HIGH_WM,
};

Expand Down Expand Up @@ -1103,13 +1107,15 @@ mysql_variable_st mysql_tracked_variables[] {
// { SQL_NET_WRITE_TIMEOUT, SETTING_VARIABLE, false, false, true, false, (char *)"net_write_timeout", (char *)"net_write_timeout", (char *)"60" , false} ,
{ SQL_WSREP_SYNC_WAIT, SETTING_VARIABLE, false, false, true, false, (char *)"wsrep_sync_wait", (char *)"wsrep_sync_wait", (char *)"0" , false} ,
{ SQL_NAME_LAST_LOW_WM, SETTING_VARIABLE, false, false, true, false, (char *)"placeholder", (char *)"placeholder", (char *)"0" , false} , // this is just a placeholder to separate the previous index from the next block
{ SQL_AURORA_READ_REPLICA_READ_COMMITTED, SETTING_VARIABLE, false, false, false, true, ( char *)"aurora_read_replica_read_committed", NULL, (char *)"" , false} ,
{ SQL_AUTO_INCREMENT_INCREMENT, SETTING_VARIABLE, false, false, true, false, (char *)"auto_increment_increment", NULL, (char *)"" , false} ,
{ SQL_AUTO_INCREMENT_OFFSET, SETTING_VARIABLE, false, false, true, false, (char *)"auto_increment_offset", NULL, (char *)"" , false} ,
{ SQL_BIG_TABLES, SETTING_VARIABLE, true, false, false, true, ( char *)"big_tables", NULL, (char *)"" , false} ,
{ SQL_DEFAULT_STORAGE_ENGINE, SETTING_VARIABLE, true, false, false, false, (char *)"default_storage_engine", NULL, (char *)"" , false} ,
{ SQL_DEFAULT_TMP_STORAGE_ENGINE, SETTING_VARIABLE, true, false, false, false, (char *)"default_tmp_storage_engine", NULL, (char *)"" , false} ,
{ SQL_FOREIGN_KEY_CHECKS, SETTING_VARIABLE, true, false, false, true, (char *)"foreign_key_checks", NULL, (char *)"" , false} ,
{ SQL_GROUP_CONCAT_MAX_LEN, SETTING_VARIABLE, false, false, true, false, (char *)"group_concat_max_len", NULL, (char *)"" , false} ,
{ SQL_GROUP_REPLICATION_CONSISTENCY, SETTING_VARIABLE, true, false, false, false, (char *)"group_replication_consistency", NULL, (char *)"" , false} ,
{ SQL_GTID_NEXT, SETTING_VARIABLE, true, false, false, false, (char *)"gtid_next", NULL, (char *)"" , true} ,
{ SQL_INNODB_LOCK_WAIT_TIMEOUT, SETTING_VARIABLE, false, false, true, false, (char *)"innodb_lock_wait_timeout", NULL, (char *)"" , false} ,
{ SQL_INNODB_STRICT_MODE, SETTING_VARIABLE, true, false, false, true, ( char *)"innodb_strict_mode", NULL, (char *)"" , false} ,
Expand All @@ -1127,6 +1133,7 @@ mysql_variable_st mysql_tracked_variables[] {
{ SQL_OPTIMIZER_SEARCH_DEPTH, SETTING_VARIABLE, false, false, true, false, (char *)"optimizer_search_depth", NULL, (char *)"" , false} ,
{ SQL_OPTIMIZER_SWITCH, SETTING_VARIABLE, true, false, false, false, (char *)"optimizer_switch", NULL, (char *)"" , false} ,
{ SQL_PROFILING, SETTING_VARIABLE, true, false, false, true, ( char *)"profiling", NULL, (char *)"" , false} ,
{ SQL_QUERY_CACHE_TYPE, SETTING_VARIABLE, false, false, true, true, ( char *)"query_cache_type", NULL, (char *)"" , false} , // note that this variable can act both as boolean AND a number. See https://dev.mysql.com/doc/refman/5.7/en/server-system-variables.html#sysvar_query_cache_type
{ SQL_SORT_BUFFER_SIZE, SETTING_VARIABLE, false, false, true, false, (char *)"sort_buffer_size", NULL, (char *)"18446744073709551615" , false} ,
{ SQL_SQL_AUTO_IS_NULL, SETTING_VARIABLE, true, false, false, true, (char *)"sql_auto_is_null", NULL, (char *)"OFF" , false} ,
{ SQL_SQL_BIG_SELECTS, SETTING_VARIABLE, true, false, false, true, (char *)"sql_big_selects", NULL, (char *)"OFF" , true} ,
Expand All @@ -1138,6 +1145,7 @@ mysql_variable_st mysql_tracked_variables[] {
{ SQL_TIMESTAMP, SETTING_VARIABLE, false, false, true, false, (char *)"timestamp", NULL, (char *)"" , false} ,
{ SQL_TMP_TABLE_SIZE, SETTING_VARIABLE, false, false, true, false, (char *)"tmp_table_size", NULL, (char *)"" , false} ,
{ SQL_UNIQUE_CHECKS, SETTING_VARIABLE, true, false, false, true, (char *)"unique_checks", NULL, (char *)"" , false} ,
{ SQL_WSREP_OSU_METHOD, SETTING_VARIABLE, true, false, false, false, (char *)"wsrep_osu_method", NULL, (char *)"" , false} ,

/*
variables that will need input validation:
Expand Down
58 changes: 41 additions & 17 deletions lib/MySQL_Session.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ static inline char is_normal_char(char c) {
}

static const std::set<std::string> mysql_variables_boolean = {
"aurora_read_replica_read_committed",
"foreign_key_checks",
"innodb_strict_mode",
"innodb_table_locks",
Expand All @@ -94,12 +95,22 @@ static const std::set<std::string> mysql_variables_numeric = {
"max_sort_length",
"optimizer_prune_level",
"optimizer_search_depth",
"query_cache_type",
"sort_buffer_size",
"sql_select_limit",
"timestamp",
"tmp_table_size",
"wsrep_sync_wait"
};
static const std::set<std::string> mysql_variables_strings = {
"default_storage_engine",
"default_tmp_storage_engine",
"group_replication_consistency",
"lc_messages",
"lc_time_names",
"optimizer_switch",
"wsrep_osu_method",
};

extern MARIADB_CHARSET_INFO * proxysql_find_charset_name(const char * const name);
extern MARIADB_CHARSET_INFO * proxysql_find_charset_collate_names(const char *csname, const char *collatename);
Expand Down Expand Up @@ -2430,12 +2441,15 @@ bool MySQL_Session::handler_again___status_SETTING_GENERIC_VARIABLE(int *_rc, co
return ret;
} else {
proxy_warning("Error while setting %s to \"%s\" on %s:%d hg %d : %d, %s\n", var_name, var_value, myconn->parent->address, myconn->parent->port, current_hostgroup, myerr, mysql_error(myconn->mysql));
if (myerr == 1193) { // variable is not found
// FIXME: here we work on absent variables
// FIXME: currently we use SQL_NAME_LAST_LOW_WM here
// FIXME: but we need to switch to SQL_NAME_LAST_HIGH_WM
int idx = SQL_NAME_LAST_LOW_WM; // FIXME: here we work on absent variables
for (int i=0; i<SQL_NAME_LAST_LOW_WM; i++) {
if (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use the errno macros here from mariadb at deps/mariadb-client/include/mysqld_error.h. So that it could read if (myerr == ER_PARSE_ERROR || myerr == ER_UNKNOWN_SYSTEM_VARIABLE || myerr == ER_QUERY_CACHE_DISABLED). These macros are safer against typos, and the names match those in the official mysql documentation for verification as well. Fewer lines of code, and self documenting.

(myerr == 1064) // You have an error in your SQL syntax
||
(myerr == 1193) // variable is not found
||
(myerr == 1651) // Query cache is disabled
) {
int idx = SQL_NAME_LAST_HIGH_WM;
for (int i=0; i<SQL_NAME_LAST_HIGH_WM; i++) {
if (strcasecmp(mysql_tracked_variables[i].set_variable_name, var_name) == 0) {
idx = i;
break;
Expand Down Expand Up @@ -4533,7 +4547,8 @@ int MySQL_Session::handler() {
auto server_hash = myconn->var_hash[i];
if (client_hash != server_hash) {
if(
//!myconn->var_absent[i] && // we currently ignore this because we assume all variables exist
!myconn->var_absent[i]
&&
mysql_variables.verify_variable(this, i)
) {
goto handler_again;
Expand Down Expand Up @@ -5660,13 +5675,7 @@ bool MySQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_C
}
proxy_debug(PROXY_DEBUG_MYSQL_COM, 8, "Changing connection SQL Mode to %s\n", value1.c_str());
}
} else if (
(var == "default_storage_engine")
|| (var == "default_tmp_storage_engine")
|| (var == "lc_messages")
|| (var == "lc_time_names")
|| (var == "optimizer_switch")
) {
} else if (mysql_variables_strings.find(var) != mysql_variables_strings.end()) {
std::string value1 = *values;
std::size_t found_at = value1.find("@");
if (found_at != std::string::npos) {
Expand All @@ -5691,7 +5700,6 @@ bool MySQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_C
}
}
}
//} else if (
} else if (mysql_variables_boolean.find(var) != mysql_variables_boolean.end()) {
int idx = SQL_NAME_LAST_HIGH_WM;
for (int i = 0 ; i < SQL_NAME_LAST_HIGH_WM ; i++) {
Expand All @@ -5718,8 +5726,24 @@ bool MySQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_C
}
}
if (idx != SQL_NAME_LAST_HIGH_WM) {
if (mysql_variables.parse_variable_number(this,idx, *values, lock_hostgroup)==false) {
return false;
if (var == "query_cache_type") {
// note that query_cache_type variable can act both as boolean AND a number , but also accept "DEMAND"
// See https://dev.mysql.com/doc/refman/5.7/en/server-system-variables.html#sysvar_query_cache_type
std::string value1 = *values;
if (strcasecmp(value1.c_str(),"off")==0 || strcasecmp(value1.c_str(),"false")==0) {
value1 = "0";
} else if (strcasecmp(value1.c_str(),"on")==0 || strcasecmp(value1.c_str(),"true")==0) {
value1 = "1";
} else if (strcasecmp(value1.c_str(),"demand")==0 || strcasecmp(value1.c_str(),"true")==0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second or condition is already handled in the previous else if. If strcasecmp(value1.c_str(),"true")==0 condition is true, then it would be handled in the second case, always resulting in value1 = "1"; Should be removed.

value1 = "2";
}
if (mysql_variables.parse_variable_number(this,idx, value1, lock_hostgroup)==false) {
return false;
}
} else {
if (mysql_variables.parse_variable_number(this,idx, *values, lock_hostgroup)==false) {
return false;
}
}
}
} else if (var == "autocommit") {
Expand Down
42 changes: 28 additions & 14 deletions test/tap/tests/generate_set_session_csv.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,19 @@ void add_value_j(std::string& j, const std::string& s, variable *v) {
}


void add_values_and_quotes(const std::string& name, const std::vector<std::string>& values) {
for (std::vector<std::string>::const_iterator it = values.begin(); it != values.end(); it++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If using iterators this way, could write it for (auto it = values.begin(); it != values.end(); it++) shorter. Since all values here are iterated, an even simpler ranged based for loop could be used for (auto& value : values) which might also save some keystrokes, in this instance, no de-referencing *it would be needed, could write value instead.

Consider renaming s to quoted_val or quoted_value for the variable name describing what the string would hold.

vars[name]->add(*it);
std::string s;
s = "\"" + *it + "\"";
vars[name]->add(s);
s = "'" + *it + "'";
vars[name]->add(s);
s = "`" + *it + "`";
vars[name]->add(s);
}
}

int main() {

srand(1);
Expand All @@ -132,6 +145,8 @@ int main() {
vars["innodb_strict_mode"]->add(bool_values);
vars["innodb_table_locks"] = new variable("innodb_table_locks", true, false, true);
vars["innodb_table_locks"]->add(bool_values);
vars["aurora_read_replica_read_committed"] = new variable("aurora_read_replica_read_committed", true, false, true);
vars["aurora_read_replica_read_committed"]->add(bool_values);

//vars[""] = new variable("");
//vars[""]->add(bool_values);
Expand All @@ -158,6 +173,12 @@ int main() {
*it = std::to_string(a);
}
}

vars["query_cache_type"] = new variable("query_cache_type", true, true, false);
vars["query_cache_type"]->add(bool_values);
vars["query_cache_type"]->add("2");
add_values_and_quotes("query_cache_type", {"DeMaNd"});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"demand" or "DEMAND" would be easier to read.


vars["lock_wait_timeout"] = new variable("lock_wait_timeout", true, true, false);
vars["lock_wait_timeout"]->add(int_values_small, 321);
vars["max_join_size"] = new variable("max_join_size", true, true, false);
Expand Down Expand Up @@ -240,22 +261,15 @@ int main() {


vars["default_storage_engine"] = new variable("default_storage_engine", true, false, false);
{
std::vector<std::string> engines = {"InnoDB", "MEMORY", "MyISAM", "BLACKHOLE"};
for (std::vector<std::string>::iterator it = engines.begin(); it != engines.end(); it++) {
vars["default_storage_engine"]->add(*it);
std::string s;
s = "\"" + *it + "\"";
vars["default_storage_engine"]->add(s);
s = "'" + *it + "'";
vars["default_storage_engine"]->add(s);
s = "`" + *it + "`";
vars["default_storage_engine"]->add(s);
}
}
add_values_and_quotes("default_storage_engine", {"InnoDB", "MEMORY", "MyISAM", "BLACKHOLE"});
vars["default_tmp_storage_engine"] = new variable("default_tmp_storage_engine", true, false, false);
vars["default_tmp_storage_engine"]->add(vars["default_storage_engine"]->values);

vars["group_replication_consistency"] = new variable("group_replication_consistency", true, false, false);
add_values_and_quotes("group_replication_consistency", {"EVENTUAL", "BEFORE_ON_PRIMARY_FAILOVER", "BEFORE", "AFTER", "BEFORE_AND_AFTER"});

vars["wsrep_osu_method"] = new variable("wsrep_osu_method", true, false, false);
add_values_and_quotes("wsrep_osu_method", {"TOI","RSU"});
/*
example:
"SET sql_mode='NO_ENGINE_SUBSTITUTION', sql_select_limit=3030, session_track_gtids=OWN_GTID; SET max_join_size=10000; ", "{'sql_mode':'NO_ENGINE_SUBSTITUTION','sql_select_limit':'3030', 'max_join_size':'10000', 'session_track_gtids':'OWN_GTID'}"
Expand All @@ -269,7 +283,7 @@ int main() {
i++;
}
}
for (int i=0; i<20000; i++) {
for (int i=0; i<40000; i++) {
int ne = rand()%4+1;
variable * va[ne];
for (int ine = 0; ine < ne; ) {
Expand Down
53 changes: 50 additions & 3 deletions test/tap/tests/set_testing-240-t.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,19 @@ std::vector<std::string> forgotten_vars {};

#include "set_testing.h"

class var_counter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a class like this, could write struct var_counter which would is equivalent to a public access class by default, so wouldn't have to write public:. For incremental only counters, unsigned (uint64_t?) maybe better. For the constructor, could write var_counter() : count(0), unknown(0) {} and perform direct initialization.

public:
int count;
int unknown;
var_counter() {
count=0;
unknown=0;
}
};

//std::unordered_map<std::string,int> unknown_var_counters;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused comment, should remove.


std::unordered_map<std::string,var_counter> vars_counters;

/* TODO
add support for variables with values out of range,
Expand Down Expand Up @@ -335,7 +348,9 @@ void * my_conn_thread(void *arg) {
}
}
}

if (std::find(possible_unknown_variables.begin(), possible_unknown_variables.end(), el.key()) != possible_unknown_variables.end()) {
vars_counters[el.key()].count++;
}
if (
(special_sqlmode == true && verified_special_sqlmode == false) ||
(k == mysql_vars.end()) ||
Expand All @@ -346,7 +361,14 @@ void * my_conn_thread(void *arg) {
(el.key() == "session_track_gtids" && !check_session_track_gtids(el.value(), s.value(), k.value()))
)
) {
if (el.key() == "wsrep_sync_wait" && k == mysql_vars.end() && (s.value() == el.value())) {
if ( k != mysql_vars.end() && s != proxysql_vars["conn"].end()) {
if (k.value() == UNKNOWNVAR) { // mysql doesn't recognize the variable
if (s.value() == el.value()) { // but proxysql and CSV are the same
variables_tested++;
vars_counters[el.key()].unknown++;
}
}
} else if (el.key() == "wsrep_sync_wait" && k == mysql_vars.end() && (s.value() == el.value())) {
variables_tested++;
} else {
__sync_fetch_and_add(&g_failed, 1);
Expand Down Expand Up @@ -399,11 +421,33 @@ int main(int argc, char *argv[]) {
host = cl.host;
port = cl.port;

diag("Loading test cases from file. This will take some time...");
if (!readTestCasesJSON(fileName2)) {
fprintf(stderr, "Cannot read %s\n", fileName2.c_str());
return exit_status();
}
queries = 2500;

MYSQL* proxysql_admin = mysql_init(NULL);

if (!mysql_real_connect(proxysql_admin, cl.host, cl.admin_username, cl.admin_password, NULL, cl.admin_port, NULL, 0)) {
fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql_admin));
return EXIT_FAILURE;
}
diag("Disabling admin-hash_passwords to be able to run test on MySQL 8");
MYSQL_QUERY(proxysql_admin, "SET admin-hash_passwords='false'");
MYSQL_QUERY(proxysql_admin, "LOAD ADMIN VARIABLES TO RUNTIME");
MYSQL_QUERY(proxysql_admin, "LOAD MYSQL USERS TO RUNTIME");

diag("Creating new hostgroup 101: DELETE FROM mysql_servers WHERE hostgroup_id = 101");
MYSQL_QUERY(proxysql_admin, "DELETE FROM mysql_servers WHERE hostgroup_id = 101");
diag("Creating new hostgroup 101: INSERT INTO mysql_servers (hostgroup_id, hostname, port, max_connections, max_replication_lag, comment) SELECT DISTINCT 101, hostname, port, 100, 0, comment FROM mysql_servers WHERE hostgroup_id IN (1,11,21,31)");
MYSQL_QUERY(proxysql_admin, "INSERT INTO mysql_servers (hostgroup_id, hostname, port, max_connections, max_replication_lag, comment) SELECT DISTINCT 101, hostname, port, 100, 0, comment FROM mysql_servers WHERE hostgroup_id IN (1,11,21,31)");
MYSQL_QUERY(proxysql_admin, "LOAD MYSQL SERVERS TO RUNTIME");
diag("Changing read traffic to hostgroup 101: UPDATE mysql_query_rules SET destination_hostgroup=101 WHERE destination_hostgroup=1");
MYSQL_QUERY(proxysql_admin, "UPDATE mysql_query_rules SET destination_hostgroup=101 WHERE destination_hostgroup=1");
MYSQL_QUERY(proxysql_admin, "LOAD MYSQL QUERY RULES TO RUNTIME");

queries = 3000;
//queries = testCases.size();
plan(queries * num_threads);

Expand All @@ -428,5 +472,8 @@ int main(int argc, char *argv[]) {
for (unsigned int i=0; i<num_threads; i++) {
pthread_join(thi[i], NULL);
}
for (std::unordered_map<std::string,var_counter>::iterator it = vars_counters.begin(); it!=vars_counters.end(); it++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto for iterator type

diag("Unknown variable %s:\t Count: %d , unknown: %d", it->first.c_str(), it->second.count, it->second.unknown);
}
return exit_status();
}
Loading