developers

Download

developers@lists.mariadb.org

December 2019

8 participants
12 discussions

Re: [Maria-developers] [Commits] 3a11b49bb5b: MDEV-20900: IN predicate to IN subquery conversion causes performance regression
by Sergey Petrunia 03 Dec '19

03 Dec '19

Hi Varun, As far as I understand, the intent of the patch is that this check: + for (uint i=0; i < n; i++) + { + if (item1->element_index(i)->cmp_type() != + item2->element_index(i)->cmp_type()) matches this check in subquery_types_allow_materialization() if (!inner->type_handler()->subquery_type_allows_materialization(inner, outer)) which has implementations like so: bool Type_handler_real_result:: subquery_type_allows_materialization(const Item *inner, const Item *outer) const { DBUG_ASSERT(inner->cmp_type() == REAL_RESULT); return outer->cmp_type() == REAL_RESULT; } // .. and the same for other datatypes ... I would like to see comments in both places (cmp_row_types and subquery_types_allow_materialization()) explaining that. Note that some Type_handler::subquery_types_allow_materialization() have a more complex implementation, for example Type_handler_string_result::subquery_types_allow_materialization() compares collations. Is there any reasons why cmp_row_types() doesn't use the same call as subquery_types_allow_materialization does: if (!inner->type_handler()->subquery_type_allows_materialization(inner, outer)) ? On Mon, Nov 04, 2019 at 04:38:02PM +0530, Varun wrote: > revision-id: 3a11b49bb5b716538f98c6a212bbbfa6fc9b7a88 (mariadb-10.3.17-145-g3a11b49bb5b) > parent(s): 162f475c4be81dfbceed093ad03d114b4c69a3c0 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2019-11-04 16:28:25 +0530 > message: > > MDEV-20900: IN predicate to IN subquery conversion causes performance regression > > Disable the IN predicate to IN subquery conversion when the types on the left and > right hand side of the IN predicate are not of comparable type. > > --- > mysql-test/main/opt_tvc.result | 53 ++++++++++++++++++++++++++++++++++++++---- > mysql-test/main/opt_tvc.test | 31 ++++++++++++++++++++++++ > sql/sql_tvc.cc | 27 ++++++++++++++++++++- > 3 files changed, 106 insertions(+), 5 deletions(-) > > diff --git a/mysql-test/main/opt_tvc.result b/mysql-test/main/opt_tvc.result > index 5329a9f64be..a68e70e8a25 100644 > --- a/mysql-test/main/opt_tvc.result > +++ b/mysql-test/main/opt_tvc.result > @@ -629,11 +629,9 @@ SELECT * FROM t1 WHERE i IN (NULL, NULL, NULL, NULL, NULL); > i > EXPLAIN EXTENDED SELECT * FROM t1 WHERE i IN (NULL, NULL, NULL, NULL, NULL); > id select_type table type possible_keys key key_len ref rows filtered Extra > -1 PRIMARY t1 ALL NULL NULL NULL NULL 3 100.00 > -1 PRIMARY <derived3> ALL NULL NULL NULL NULL 5 100.00 Using where; FirstMatch(t1); Using join buffer (flat, BNL join) > -3 DERIVED NULL NULL NULL NULL NULL NULL NULL NULL No tables used > +1 SIMPLE t1 ALL NULL NULL NULL NULL 3 100.00 Using where > Warnings: > -Note 1003 /* select#1 */ select `test`.`t1`.`i` AS `i` from `test`.`t1` semi join ((values (NULL),(NULL),(NULL),(NULL),(NULL)) `tvc_0`) where `test`.`t1`.`i` = `tvc_0`.`_col_1` > +Note 1003 select `test`.`t1`.`i` AS `i` from `test`.`t1` where `test`.`t1`.`i` in (NULL,NULL,NULL,NULL,NULL) > SET in_predicate_conversion_threshold= default; > DROP TABLE t1; > # > @@ -687,3 +685,50 @@ f1 f2 > 1 1 > DROP TABLE t1,t2,t3; > SET @@in_predicate_conversion_threshold= default; > +# > +# MDEV-20900: IN predicate to IN subquery conversion causes performance regression > +# > +create table t1(a int, b int); > +insert into t1 select seq-1, seq-1 from seq_1_to_10; > +set in_predicate_conversion_threshold=2; > +explain select * from t1 where t1.a IN ("1","2","3","4"); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 10 Using where > +select * from t1 where t1.a IN ("1","2","3","4"); > +a b > +1 1 > +2 2 > +3 3 > +4 4 > +set in_predicate_conversion_threshold=0; > +explain select * from t1 where t1.a IN ("1","2","3","4"); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 10 Using where > +select * from t1 where t1.a IN ("1","2","3","4"); > +a b > +1 1 > +2 2 > +3 3 > +4 4 > +set in_predicate_conversion_threshold=2; > +explain select * from t1 where (t1.a,t1.b) in (("1","1"),(2,2),(3,3),(4,4)); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 10 Using where > +select * from t1 where (t1.a,t1.b) in (("1","1"),(2,2),(3,3),(4,4)); > +a b > +1 1 > +2 2 > +3 3 > +4 4 > +set in_predicate_conversion_threshold=0; > +explain select * from t1 where (t1.a,t1.b) in (("1","1"),(2,2),(3,3),(4,4)); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 10 Using where > +select * from t1 where (t1.a,t1.b) in (("1","1"),(2,2),(3,3),(4,4)); > +a b > +1 1 > +2 2 > +3 3 > +4 4 > +drop table t1; > +SET @@in_predicate_conversion_threshold= default; > diff --git a/mysql-test/main/opt_tvc.test b/mysql-test/main/opt_tvc.test > index 7319dbdc9e8..e4e8c6d7919 100644 > --- a/mysql-test/main/opt_tvc.test > +++ b/mysql-test/main/opt_tvc.test > @@ -3,6 +3,7 @@ > # > source include/have_debug.inc; > source include/default_optimizer_switch.inc; > +source include/have_sequence.inc; > > create table t1 (a int, b int); > > @@ -397,3 +398,33 @@ SELECT * FROM t3 WHERE (f1,f2) IN ((2, 2), (1, 2), (3, 5), (1, 1)); > DROP TABLE t1,t2,t3; > > SET @@in_predicate_conversion_threshold= default; > + > +--echo # > +--echo # MDEV-20900: IN predicate to IN subquery conversion causes performance regression > +--echo # > + > +create table t1(a int, b int); > +insert into t1 select seq-1, seq-1 from seq_1_to_10; > + > +set in_predicate_conversion_threshold=2; > + > +let $query= select * from t1 where t1.a IN ("1","2","3","4"); > +eval explain $query; > +eval $query; > + > +set in_predicate_conversion_threshold=0; > +eval explain $query; > +eval $query; > + > +set in_predicate_conversion_threshold=2; > +let $query= select * from t1 where (t1.a,t1.b) in (("1","1"),(2,2),(3,3),(4,4)); > +eval explain $query; > +eval $query; > + > +set in_predicate_conversion_threshold=0; > +eval explain $query; > +eval $query; > + > +drop table t1; > +SET @@in_predicate_conversion_threshold= default; > + > diff --git a/sql/sql_tvc.cc b/sql/sql_tvc.cc > index 816c6fe1089..78c7c34a81a 100644 > --- a/sql/sql_tvc.cc > +++ b/sql/sql_tvc.cc > @@ -796,6 +796,31 @@ bool Item_subselect::wrap_tvc_into_select(THD *thd, st_select_lex *tvc_sl) > } > > > +/* > + @brief > + Check whether the items are of comparable type or not > + > + @retval > + 0 comparable > + 1 not comparable > +*/ > + > +static bool cmp_row_types(Item* item1, Item* item2) > +{ > + uint n= item1->cols(); > + if (item2->check_cols(n)) > + return true; > + > + for (uint i=0; i < n; i++) > + { > + if (item1->element_index(i)->cmp_type() != > + item2->element_index(i)->cmp_type()) > + return true; > + } > + return false; > +} > + > + > /** > @brief > Transform IN predicate into IN subquery > @@ -843,7 +868,7 @@ Item *Item_func_in::in_predicate_to_in_subs_transformer(THD *thd, > > for (uint i=1; i < arg_count; i++) > { > - if (!args[i]->const_item()) > + if (!args[i]->const_item() || cmp_row_types(args[0], args[i])) > return this; > } > > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 387839e44cc: Plugin for Hashicorp Vault Key Management System
by Sergei Golubchik 02 Dec '19

02 Dec '19

Hi, Julius! Looks good! A couple of minor comments, see below On Dec 02, Julius Goryavsky wrote: > revision-id: 387839e44cc (mariadb-10.4.10-179-g387839e44cc) > parent(s): 8115a02283d > author: Julius Goryavsky <julius.goryavsky(a)mariadb.com> > committer: Julius Goryavsky <julius.goryavsky(a)mariadb.com> > timestamp: 2019-11-15 13:37:35 +0100 > message: > > Plugin for Hashicorp Vault Key Management System > > diff --git a/plugin/hashicorp_key_management/CMakeLists.txt b/plugin/hashicorp_key_management/CMakeLists.txt > new file mode 100644 > index 00000000000..4b5f79afa66 > --- /dev/null > +++ b/plugin/hashicorp_key_management/CMakeLists.txt > @@ -0,0 +1,11 @@ > +INCLUDE(FindCURL) > +IF(NOT CURL_FOUND) > + # Can't build plugin > + RETURN() > +ENDIF() > + > +INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/sql ${CURL_INCLUDE_DIR}) > + > +MYSQL_ADD_PLUGIN(HASHICORP_KEY_MANAGEMENT > + hashicorp_key_management_plugin.cc > + LINK_LIBRARIES ${CURL_LIBRARIES}) > diff --git a/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc b/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc > new file mode 100644 > index 00000000000..26ee7b7cd3d > --- /dev/null > +++ b/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc > @@ -0,0 +1,325 @@ > +/* Copyright (C) 2019 MariaDB Corporation > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; version 2 of the License. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program; if not, write to the Free Software > + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1335 USA */ > + > +#include <mysql/plugin_encryption.h> > +#include <mysqld_error.h> > +#include <string.h> > +#include <stdlib.h> > +#include <limits.h> > +#include <errno.h> > +#include <string> > +#include <sstream> > +#include <curl/curl.h> > + > +static char* vault_url; > +static char* token; > +static char* vault_ca; > + > +static MYSQL_SYSVAR_STR(vault_ca, vault_ca, > + PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_READONLY, > + "Path to Certificate Authority (CA) bundle (is a file " > + "that contains root and intermediate certificates)", > + NULL, NULL, ""); > + > +static MYSQL_SYSVAR_STR(vault_url, vault_url, > + PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_READONLY, > + "HTTP[s] URL that is used to connect to the Hashicorp Vault server", > + NULL, NULL, "https://127.0.0.1:8200/v1/secret"); > + > +static MYSQL_SYSVAR_STR(token, token, > + PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_READONLY | PLUGIN_VAR_NOSYSVAR, > + "Authentication token that passed to the Hashicorp Vault " > + "in the request header", > + NULL, NULL, ""); > + > +static struct st_mysql_sys_var* settings[] = { > + MYSQL_SYSVAR(vault_url), > + MYSQL_SYSVAR(token), > + MYSQL_SYSVAR(vault_ca), > + NULL > +}; > + > +static std::string get_error_from_curl (CURLcode curl_code, char *curl_errbuf) > +{ > + size_t len = strlen(curl_errbuf); > + std::ostringstream stream; > + if (curl_code != CURLE_OK) > + { > + stream << "CURL returned this error code: " << curl_code; > + stream << " with error message : "; > + if (len) > + stream << curl_errbuf; > + else > + stream << curl_easy_strerror(curl_code); > + } > + return stream.str(); > +} > + > +#define max_response_size 65536 > + > +static size_t write_response_memory (void *contents, size_t size, size_t nmemb, > + void *userp) > +{ > + size_t realsize = size * nmemb; > + if (size != 0 && realsize / size != nmemb) > + return 0; // overflow detected > + std::ostringstream *read_data = static_cast<std::ostringstream*>(userp); > + size_t current_length = read_data->tellp(); > + if (current_length + realsize > max_response_size) > + return 0; // response size limit exceeded > + read_data->write(static_cast<char*>(contents), realsize); > + if (!read_data->good()) > + return 0; > + return realsize; > +} > + > +#define timeout 300 > + > +static bool curl_run (char *url, std::string *response) > +{ > + char curl_errbuf [CURL_ERROR_SIZE]; > + struct curl_slist *list = NULL; > + std::ostringstream read_data_stream; > + CURLcode curl_res = CURLE_OK; > + long http_code = 0; > + CURL *curl = curl_easy_init(); > + if (curl == NULL) > + { > + my_printf_error(ER_UNKNOWN_ERROR, > + "Cannot initialize curl session", > + ME_ERROR_LOG_ONLY); > + return true; > + } > + std::string token_header = std::string("X-Vault-Token:") + > + std::string(token); > + curl_errbuf[0] = '\0'; > + if ((list= curl_slist_append(list, token_header.c_str())) == NULL || > + (curl_res= curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, curl_errbuf)) != > + CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, > + write_response_memory)) != CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_WRITEDATA, > + &read_data_stream)) != > + CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_HTTPHEADER, list)) != > + CURLE_OK || > + /* > + The options CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST are > + set explicitly to withstand possible future changes in curl defaults: > + */ > + (curl_res= curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 1)) != > + CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 2L)) != > + CURLE_OK || > + (vault_ca != NULL && strlen(vault_ca) != 0 && vault_ca still cannot be NULL :) > + (curl_res= curl_easy_setopt(curl, CURLOPT_CAINFO, vault_ca)) != > + CURLE_OK) || > + (curl_res= curl_easy_setopt(curl, CURLOPT_USE_SSL, CURLUSESSL_ALL)) != > + CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L)) != > + CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT, timeout)) != > + CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_TIMEOUT, timeout)) != > + CURLE_OK) > + { if you get an error here you won't free list, so it'll be a memory leak. In fact, I don't understand why you need two if() here, just combine them in one, the error message is the same anyway, and the second if() already has curl_slist_free_all(). > + my_printf_error(ER_UNKNOWN_ERROR, > + get_error_from_curl(curl_res, curl_errbuf).c_str(), > + ME_ERROR_LOG_ONLY); > + return true; > + } > + if ((curl_res = curl_easy_setopt(curl, CURLOPT_URL, url)) != CURLE_OK || > + (curl_res = curl_easy_perform(curl)) != CURLE_OK || > + (curl_res = curl_easy_getinfo (curl, CURLINFO_RESPONSE_CODE, > + &http_code)) != CURLE_OK) > + { > + curl_slist_free_all(list); > + my_printf_error(ER_UNKNOWN_ERROR, > + get_error_from_curl(curl_res, curl_errbuf).c_str(), > + ME_ERROR_LOG_ONLY); > + return true; > + } > + curl_slist_free_all(list); > + if (http_code == 404) > + { > + *response = std::string(""); > + return false; > + } > + *response = read_data_stream.str(); > + return http_code < 200 || http_code >= 300; > +} > + > +static unsigned int get_latest_version (uint key_id) > +{ > + char url[2048]; > + std::string response_str; > + const char *response; > + const char *js, *ver; > + int js_len, ver_len; > + size_t response_len; > + snprintf(url, sizeof(url), "%s/%u", vault_url, key_id); > + if (curl_run(url, &response_str)) { > + my_printf_error(ER_UNKNOWN_ERROR, > + "Unable to get key data", > + 0); > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + response = response_str.c_str(); > + response_len = response_str.size(); > + if (json_get_object_key(response, response + response_len, > + "metadata", &js, &js_len) != JSV_OBJECT) > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + if (json_get_object_key(js, js+js_len,"version", > + &ver, &ver_len) != JSV_NUMBER) > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + /* > + Here we may use atoi, but it has undefined overflow behavior > + and does not work with unsigned integers, so use strtoul: > + */ > + unsigned long version = strtoul(ver, NULL, 10); > + /* > + If the result of the conversion does not fit into an > + unsigned integer or if an error (for example, overflow) > + occurred during the conversion, then we need to return > + an error: > + */ > + if (version > UINT_MAX || errno) this might be an error with gcc, "condition is always true" double check this please, no need to fix anything if there's no error > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + return version; this might be an error with VS, because of the implicit ulong->uint cast. double check this please, no need to fix anything if there's no error > +} > + > +static unsigned int get_key_from_vault (unsigned int key_id, > + unsigned int key_version, > + unsigned char *dstbuf, > + unsigned int *buflen) > +{ > + char url[2048]; > + std::string response_str; > + const char *response; > + const char *js, *ver, *key; > + int js_len, key_len, ver_len; > + size_t response_len; > + if (key_version != ENCRYPTION_KEY_VERSION_INVALID) > + snprintf(url, sizeof(url), "%s/%u?%u", vault_url, key_id, key_version); > + else > + snprintf(url, sizeof(url), "%s/%u", vault_url, key_id); > + if (curl_run(url, &response_str)) > + { > + my_printf_error(ER_UNKNOWN_ERROR, > + "Unable to get key data", > + 0); > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + response = response_str.c_str(); > + response_len = response_str.size(); > +#ifndef NDEBUG > + /* > + An internal check that is needed only for debugging the plugin > + operation - in order to ensure that we get from the Hashicorp Vault > + server exactly the version of the key that is needed: > + */ > + if (key_version != ENCRYPTION_KEY_VERSION_INVALID) > + { > + if (json_get_object_key(response, response + response_len, > + "metadata", &js, &js_len) != JSV_OBJECT) > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + if (json_get_object_key(js, js+js_len,"version", > + &ver, &ver_len) != JSV_NUMBER) > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + /* > + Here we may use atoi, but it has undefined overflow behavior > + and does not work with unsigned integers, so use strtoul: > + */ > + unsigned long version = strtoul(ver, NULL, 10); > + /* > + If the result of the conversion does not fit into an > + unsigned integer or if an error (for example, overflow) > + occurred during the conversion, then we need to return > + an error: > + */ > + if (errno || key_version != version) > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + } > +#endif > + if (json_get_object_key(response, response + response_len, > + "data", &js, &js_len) != JSV_OBJECT) > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + if (json_get_object_key(js, js + js_len, "data", > + &key, &key_len) != JSV_STRING) > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + if (*buflen < (unsigned) key_len) { > + *buflen= key_len; > + return ENCRYPTION_KEY_BUFFER_TOO_SMALL; > + } > + *buflen= key_len; > + memcpy(dstbuf, key, key_len); > + return 0; > +} > + > +struct st_mariadb_encryption hashicorp_key_management_plugin= { > + MariaDB_ENCRYPTION_INTERFACE_VERSION, > + get_latest_version, > + get_key_from_vault, > + 0, 0, 0, 0, 0 > +}; > + > +static int hashicorp_key_management_plugin_init(void *p) > +{ > + curl_global_init(CURL_GLOBAL_ALL); > + return 0; > +} > + > +static int hashicorp_key_management_plugin_deinit(void *p) > +{ > + curl_global_cleanup(); > + return 0; > +} > + > +/* > + Plugin library descriptor > +*/ > +maria_declare_plugin(hashicorp_key_management) > +{ > + MariaDB_ENCRYPTION_PLUGIN, > + &hashicorp_key_management_plugin, > + "hashicorp_key_management", > + "MariaDB Corporation", > + "HashiCorp Vault key management plugin", > + PLUGIN_LICENSE_GPL, > + hashicorp_key_management_plugin_init, > + hashicorp_key_management_plugin_deinit, > + 0x0100 /* 1.0 */, > + NULL, /* status variables */ > + settings, > + "1.0", > + MariaDB_PLUGIN_MATURITY_ALPHA > +} > +maria_declare_plugin_end; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0