December 2016 - developers - lists.mariadb.org

[Maria-developers] Windows line endings in MariaDB sources again (
by Otto Kekäläinen 24 Jul '19

24 Jul '19

Hello! While trying to release 10.0.26 for Debian and Ubuntu I noticed git chokes again on Windows line endings. As our .gitattributes states that by default git should clean up the line endings, why do we keep getting new Windows line endings in new code? mariadb-10.0$ cat .gitattributes # Set the default behavior, in case people don't have core.autocrlf set. * text=auto ... storage/connect/mysql-test/connect/std_data/*.txt -text mariadb-10.0.26$ find . | xargs file | grep CRLF ./storage/connect/mysql-test/connect/std_data/boyswin.txt: ASCII text, with CRLF line terminators ./storage/connect/mysql-test/connect/std_data/expenses.txt: ASCII text, with CRLF line terminators ./storage/connect/mysql-test/connect/std_data/emp.txt: ASCII text, with CRLF line terminators ./mysql-test/std_data/loaddata7.dat: ASCII text, with CRLF line terminators ./mysql-test/r/loadxml.result: ASCII text, with CRLF, LF line terminators ./mysql-test/r/perror-win.result: ASCII text, with CRLF, LF line terminators ./mysql-test/r/mysql_binary_mode.result: ASCII English text, with CRLF, LF line terminators ./mysql-test/r/func_regexp_pcre.result: UTF-8 Unicode C++ program text, with CRLF, CR, LF line terminators ./pcre/testdata/grepoutputN: ASCII text, with CRLF, CR, LF line terminators ./pcre/testdata/greppatN4: ASCII text, with CRLF line terminators This leads for me to: dpkg-source: info: building mariadb-10.0 using existing ./mariadb-10.0_10.0.26.orig.tar.gz dpkg-source: info: local changes detected, the modified files are: mariadb-10.0/storage/connect/mysql-test/connect/std_data/boyswin.txt mariadb-10.0/storage/connect/mysql-test/connect/std_data/emp.txt mariadb-10.0/storage/connect/mysql-test/connect/std_data/expenses.txt dpkg-source: error: aborting due to unexpected upstream changes, see /tmp/mariadb-10.0_10.0.26-1.diff.Yzwwhw Which means I have to do extra work with importing the upstream sources.. - Otto

2 4

[Maria-developers] Please review MDEV-11292 Item_func_sp::fix_length_and_dec() does not set collation derivation properly
by Alexander Barkov 20 Nov '17

20 Nov '17

Hello Alexey, Please review a patch for MDEV-11292 (for 10.3). Thanks.

2 2

Re: [Maria-developers] 0310b44: MDEV-10418 Assertion `m_extra_cache' failed in ha_partition::late_extra_cache(uint)
by Sergei Golubchik 21 Feb '17

21 Feb '17

Hi, Alexey! On Dec 20, Alexey Botchkov wrote: > revision-id: 0310b444dba1b9cbd6921963dd3bff689ca23a24 (mariadb-5.5.53-34-g0310b44) > parent(s): f23b41b9b8a30e0e54a1ec7a8923057b0e57e0f5 > committer: Alexey Botchkov > timestamp: 2016-12-20 00:24:20 +0400 > message: > > MDEV-10418 Assertion `m_extra_cache' failed in ha_partition::late_extra_cache(uint) > > The m_extra_prepare_for_update parameter should be set to FALSE > as the query ends. This seems fairly obvious, that it should. But why MySQL doesn't have this bug (despite not resetting m_extra_prepare_for_update)? > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index 15fa7d1..4374354 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -6667,6 +6667,7 @@ int ha_partition::reset(void) > DBUG_ENTER("ha_partition::reset"); > if (m_part_info) > bitmap_set_all(&m_part_info->used_partitions); > + m_extra_prepare_for_update= FALSE; > file= m_file; > do > { Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

[Maria-developers] MDEV-10142 - bb-10.2-compatibility
by jerome brauge 01 Feb '17

01 Feb '17

Hello, To build this branch on windows (Visual Studio Express 2015), I have to add "#pragma warning( disable : 4099 )" in sql_lex.h. But I don't know if it's the right solution. I have some questions: - I read the comment of Michael Widenius on subtask MDEV-10574. I think that it's important to change behavior of string functions whose returns a string (like rtrim, ltrim, substring, concat, ...) to return null instead of ''. Without this, we have to use UDF instead native functions and it's never good from a performance point of view. Have you made a decision ? - On Oracle, we can easily disable/enable triggers. I found an old request for this on jira : MDEV-7579. As you already manage order_action now, perhaps you can simply use negative orders to indicate disabled triggers. - do you have planning to implement operator || to concat strings ? Best regards, J. Brauge

3 16

[Maria-developers] Please review MDEV-11692 Comparison data type aggregation for pluggable data types
by Alexander Barkov 31 Jan '17

31 Jan '17

Hello Vicențiu, can you please review a patch for MDEV-11692. The patch is for bb-10.2-ext. Thanks!

2 1

[Maria-developers] Please review MDEV-11134 Assertion `fixed' failed in Item::const_charset_converter(THD*, CHARSET_INFO*, bool, const char*)
by Alexander Barkov 23 Jan '17

23 Jan '17

Hello Sergei, can you please review a fix for MDEV-11134. I made it the easiest way, just fixed the assert to cover this special case when Item_param::safe_charset_converter() is called from mysql_prepare_create_table(). But perhaps it can be done in different ways: 1. Do call fix_fields() - Fix mysql_prepare_create_table() to call fix_fields. - Fix Item_param::cleanup() not to set fixed to false. or 2. Sync Item_param::fixed with Item_param::basic_const_item() - Fix Item_param::set_xxx() to set both state=XXX_VALUE and fixed=true. - Fix Item_param::cleanup() not to set fixed to false (like in #1) Thanks!

2 6

[Maria-developers] Please review MDEV-9522 Split sql_select.cc:can_change_cond_ref_to_const into virtual methods in Type_handler
by Alexander Barkov 13 Jan '17

13 Jan '17

Hello Vicențiu, can you please review a patch for MDEV-9522 ? Thanks!

2 3

Re: [Maria-developers] Review for MDEV-10332
by Sergei Golubchik 29 Dec '16

29 Dec '16

Hi, Georg! First, I've looked a bit at EVP_CIPHER_CTX_new/etc. With the CRYPTO_set_mem_functions() one can overwrite malloc/realloc/free functions that OpenSSL is using. We absolutely should do it. At the very least, OpenSSL should use my_malloc/my_realloc/my_free. But a more interesting approach would be to allocate a buffer on the stack and let our openssl m/r/f helpers take the memory from there. And print a warning with a fallback to malloc if that buffer isn't enough. This way openssl won't need mallocs is most cases. Still, I'd suggest to do this in a separate commit. In your main "OpenSSL 1.1 support" commit, just use CRYPTO_set_mem_functions to put OpenSSL through my_malloc/my_realloc/my_free. On Nov 15, Georg Richter wrote: > From 6103b311c4c5740998d342afbba9ac70f8ec5731 Mon Sep 17 00:00:00 2001 > From: Georg Richter <georg(a)mariadb.com> > Date: Tue, 15 Nov 2016 18:39:54 +0100 > Subject: [PATCH] MDEV-10332: Added support for OpenSSL 1.1 > > diff --git a/mysql-test/r/openssl_6975,tlsv10.result b/mysql-test/r/openssl_6975,tlsv10.result > index a65167f..e216ec2 100644 > --- a/mysql-test/r/openssl_6975,tlsv10.result > +++ b/mysql-test/r/openssl_6975,tlsv10.result > @@ -1,11 +1,11 @@ > create user ssl_sslv3@localhost; > -grant select on test.* to ssl_sslv3@localhost require cipher "RC4-SHA"; > +grant select on test.* to ssl_sslv3@localhost require cipher "DES-CBC3-SHA"; did you verify that after your changes all tests also work with older openssl versions and with yassl? > create user ssl_tls12@localhost; > grant select on test.* to ssl_tls12@localhost require cipher "AES128-SHA256"; > TLS1.2 ciphers: user is ok with any cipher > ERROR 2026 (HY000): SSL connection error: sslv3 alert handshake failure > diff --git a/mysys_ssl/my_crypt.cc b/mysys_ssl/my_crypt.cc > index 49bd9af..d72bc72 100644 > --- a/mysys_ssl/my_crypt.cc > +++ b/mysys_ssl/my_crypt.cc > @@ -36,9 +36,20 @@ > class MyCTX > { > public: > - EVP_CIPHER_CTX ctx; > - MyCTX() { EVP_CIPHER_CTX_init(&ctx); } > - virtual ~MyCTX() { EVP_CIPHER_CTX_cleanup(&ctx); ERR_remove_state(0); } > + EVP_CIPHER_CTX *ctx; > +#if defined(HAVE_YASSL) > + MyCTX() { ctx= new EVP_CIPHER_CTX; EVP_CIPHER_CTX_init(ctx); } > + virtual ~MyCTX() { EVP_CIPHER_CTX_cleanup(ctx); delete ctx; ERR_remove_state(0); } > +#else > + MyCTX() {ctx= EVP_CIPHER_CTX_new(); EVP_CIPHER_CTX_init(ctx); } > + virtual ~MyCTX() { EVP_CIPHER_CTX_free(ctx); > +#if OPENSSL_VERSION_NUMBER < 0x10100000L > + ERR_remove_state(0); > +#else > + ERR_clear_error(); > +#endif > + } > +#endif No #ifdef's here, please. YaSSL-specific code from mysys_ssl/yassl.cc makes yassl to look sufficiently like openssl, so that mysys_ssl/my_crypt.cc wouldn't need any ifdefs. Please, keep it that way. And, certainly, don't put any #if's that check openssl version in the code, keep them in the headers or in the beginning of the file (there is, already #define ERR_remove_state(X) there) > virtual int init(const EVP_CIPHER *cipher, int encrypt, const uchar *key, > uint klen, const uchar *iv, uint ivlen) > @@ -72,7 +83,10 @@ class MyCTX_nopad : public MyCTX > { > public: > const uchar *key; > + const uchar *src; > int klen; > + uchar oiv[90]; 90 bytes for an IV? > + uint extra_bytes; > > MyCTX_nopad() : MyCTX() { } > ~MyCTX_nopad() { } > @@ -285,7 +310,11 @@ int my_random_bytes(uchar *buf, int num) > instead of whatever random engine is currently set in OpenSSL. That way > we are guaranteed to have a non-blocking random. > */ > +#if OPENSSL_VERSION_NUMBER < 0x10100000L > RAND_METHOD *rand = RAND_SSLeay(); > +#else > + RAND_METHOD *rand = RAND_OpenSSL(); > +#endif no #if's in the code, please > if (rand == NULL || rand->bytes(buf, num) != 1) > return MY_AES_OPENSSL_ERROR; > return MY_AES_OK; > diff --git a/mysys_ssl/my_md5.cc b/mysys_ssl/my_md5.cc > index 7139ea9..4885033 100644 > --- a/mysys_ssl/my_md5.cc > +++ b/mysys_ssl/my_md5.cc > @@ -58,10 +57,11 @@ static void md5_result(MD5_CONTEXT *context, uchar digest[MD5_HASH_SIZE]) > > #elif defined(HAVE_OPENSSL) > #include <openssl/evp.h> > -typedef EVP_MD_CTX MD5_CONTEXT; > +typedef void MD5_CONTEXT; > > -static void md5_init(MD5_CONTEXT *context) > +static void md5_init(MD5_CONTEXT *ctx) > { > + EVP_MD_CTX *context= (EVP_MD_CTX *)ctx; > EVP_MD_CTX_init(context); > #ifdef EVP_MD_CTX_FLAG_NON_FIPS_ALLOW > /* Ok to ignore FIPS: MD5 is not used for crypto here */ > @@ -123,21 +143,43 @@ void my_md5_multi(uchar *digest, ...) > { > va_list args; > va_start(args, digest); > - > - MD5_CONTEXT md5_context; > +#ifdef HAVE_YASSL > + MD5_CONTEXT *md5_context= new MD5_CONTEXT; > +#else > +#if OPENSSL_VERSION_NUMBER < 0x10100000L > + EVP_MD_CTX *md5_context= (EVP_MD_CTX *)alloca(sizeof(EVP_MD_CTX)); > +#else > + EVP_MD_CTX *md5_context= EVP_MD_CTX_new(); > +#endif > +#endif > const uchar *str; > > - md5_init_fast(&md5_context); > + md5_init_fast(md5_context); > for (str= va_arg(args, const uchar*); str; str= va_arg(args, const uchar*)) > - md5_input(&md5_context, str, va_arg(args, size_t)); > + md5_input(md5_context, str, va_arg(args, size_t)); > > - md5_result(&md5_context, digest); > + md5_result(md5_context, digest); > va_end(args); > +#if defined(HAVE_YASSL) > + delete md5_context; > +#else > +#if OPENSSL_VERSION_NUMBER >= 0x10100000L > + EVP_MD_CTX_free(md5_context); > +#endif > +#endif > } > > size_t my_md5_context_size() > { > +#if defined(HAVE_YASSL) > return sizeof(MD5_CONTEXT); > +#elif OPENSSL_VERSION_NUMBER < 0x10100000L > + return sizeof(EVP_MD_CTX); > +#else > + /* Since OpenSSL 1.1 we can't determine the length of EVP_MD_CTX anymore, > + so we need to return a number (48 bytes + 16 bytes reserved) */ > + return 64; How can you be sure that it's enough? I'd just return sizeof(void*) and in md5_init stored a return value of EVP_MD_CTX_new() there. Until we do that optimization that I mentioned in the beginning of the email. And get rid of #if's please. > +#endif > } Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] TRUE LOCK=NONE for slave
by Sachin Setiya 29 Dec '16

29 Dec '16

Hello everyone! I am thinking of implementing true lock=none for slaves. Currently Alter command is only executed on slave when master commits it. I(thanks marko for suggesting this idea) am thinking of writing BEGIN_DDL_EVENT in binlog after master hits ha_prepare_inplace_alter_table. Then master will write QUERY_EVENT on binlog with actual alter query . On commit/rollback master will write COMMIT_DDL_EVENT/ROLLBACK_DDL_EVENT. On slave there will be pool of threads(configurable global variable), which will apply these DDLs. On receiving BEGIN_DDL_EVENT slave thread will pass the QUERY_EVENT to one of the worker thread. Worker thread will execute until ha_inplace_alter_table. Actual commit_inplace_alter will be called by sql thread. If sql thread recieve some kind of rollback event , then it will somehow signal worker thread to stop executing alter. If none of the worker threads are available then event will be en-queued, then If we received rollback event the we will simply discard event from queue, If we received commit event then SQL thread will synchronously process DDL event. Please let me know what you think. Credits for idea Marko and Lixun. Regards sachin

3 3

Re: [Maria-developers] [Commits] d158f55659c: MDEV-11679 Remove redundant function fsp_header_get_crypt_offset()
by Jan Lindström 29 Dec '16

29 Dec '16

On Wed, Dec 28, 2016 at 5:16 PM, <marko.makela(a)mariadb.com> wrote: > > diff --git a/storage/innobase/fil/fil0crypt.cc b/storage/innobase/fil/ > fil0crypt.cc > index 75efcdfdab0..095b9f706c0 100644 > --- a/storage/innobase/fil/fil0crypt.cc > +++ b/storage/innobase/fil/fil0crypt.cc > @@ -1179,10 +1179,11 @@ fil_crypt_start_encrypting_space( > > /* 3 - compute location to store crypt data */ > byte* frame = buf_block_get_frame(block); > - ulint maxsize = 0; > ut_ad(crypt_data); > - crypt_data->page0_offset = > - fsp_header_get_crypt_offset(page_size, &maxsize); > + crypt_data->page0_offset = FSP_HEADER_OFFSET > + + fsp_header_get_encryption_offset(page_size); > + const ulint maxsize = page_size.logical() > + - crypt_data->page0_offset - FIL_PAGE_DATA_END; > > This I do not like, why fsp_header_get_encryption_offset does not calculate the actual offset so that there is no need for adjustments? Now every place where you call this you need to add FSP_HEADER_OFFSET to returned offset. Similarly for maxsize actually meaning free space on the page. This is naturally technically correct, but is it maintainable? R: Jan

1 0

Re: [Maria-developers] e687577: MDEV-10340: support COM_RESET_CONNECTION
by Sergei Golubchik 28 Dec '16

28 Dec '16

Hi, Oleksandr! On Dec 12, Oleksandr Byelkin wrote: > revision-id: e6875772b0d65a6341456a6f4aca3353bbdae3e4 (mariadb-10.2.2-131-ge687577) > parent(s): 7ca1e2abad42a7436e6b668b4568d6fadc2ca165 > committer: Oleksandr Byelkin > timestamp: 2016-12-12 14:41:45 +0100 > message: > > MDEV-10340: support COM_RESET_CONNECTION > > draft to check with client > > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index a2a4ed2..b760aeb 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -1679,6 +1680,13 @@ bool dispatch_command(enum enum_server_command command, THD *thd, > break; > } > #endif > + case COM_RESET_CONNECTION: > + { > + thd->status_var.com_other++; > + thd->change_user(); > + my_ok(thd, 0, 0, 0); > + break; > + } Looks ok, but I suspect that, just like for COM_CHANGE_USER, you might need here: thd->clear_error(); // if errors from rollback ok to push with the change as above. But please make sure that C/C part is pushed too Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 33b01257190: MDEV-11584: GRANT inside an SP does not work well on 2nd execution
by Sergei Golubchik 28 Dec '16

28 Dec '16

Hi, Oleksandr! On Dec 21, Oleksandr Byelkin wrote: > revision-id: 33b01257190cef414a4b0f00bf36ee6a3ceda0a7 (mariadb-10.1.20-6-g33b01257190) > parent(s): 9e032d6150b39cf36a22e7c1503e06fae9c4016d > committer: Oleksandr Byelkin > timestamp: 2016-12-21 20:11:14 +0100 > message: > > MDEV-11584: GRANT inside an SP does not work well on 2nd execution > > Allocate password hash in statment memory > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 3f26bc2e08a..b44b120cf04 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -1067,7 +1067,14 @@ static bool fix_lex_user(THD *thd, LEX_USER *user) > make_scramble= my_make_scrambled_password; > } > > - char *buff= (char *) thd->alloc(scramble_length + 1); > + char *buff; > + { > + Query_arena *arena, backup; > + arena= thd->activate_stmt_arena_if_needed(&backup); > + buff= (char *) thd->alloc(scramble_length + 1); > + if (arena) > + thd->restore_active_arena(arena, &backup); > + } Ok to push, just remove the unnecessary pair of curly braces and the indentation. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] Fwd: mixing of user-defined data types with other data types
by Vicențiu Ciorbaru 28 Dec '16

28 Dec '16

Hi Alexander! Comments inline. Most are stylistic comments. I think the patch is great. The Item_type_holder is really bugging me though. It feels poorly designed in the current place. > diff --git a/sql/field.cc b/sql/field.cc > index 6f273e6..7876a6f 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -84,9 +84,13 @@ const char field_separator=','; > */ > #define FIELDTYPE_TEAR_FROM (MYSQL_TYPE_BIT + 1) > #define FIELDTYPE_TEAR_TO (MYSQL_TYPE_NEWDECIMAL - 1) > -#define FIELDTYPE_NUM (FIELDTYPE_TEAR_FROM + (255 - FIELDTYPE_TEAR_TO)) > +#define FIELDTYPE_LAST 254 > +#define FIELDTYPE_NUM (FIELDTYPE_TEAR_FROM + (FIELDTYPE_LAST - FIELDTYPE_TEAR_TO)) > These defines here look sketchy. I know it's not from your patch but can we not turn them to const ints instead? Also, the naming is horibly inconsistent, one uses 2 and others use to. I vote for _to_. We can have the full function definition as: const int FIELDTYPE_TEAR_FROM= MYSQL_TYPE_BIT + 1; const int FIELDTYPE_TEAR_TO= MYSQL_TYPE_NEWDECIMAL - 1; const int FIELDTYPE_LAST= 254; const int FIELDTYPE_NUM= FIELDTYPE_TEAR_FROM + (FIELDTYPE_LAST - FIELDTYPE_TEAR_TO); static inline int field_type_to_index (enum_field_types field_type) { DBUG_ASSERT(real_type_to_type(field_type) < FIELDTYPE_TEAR_FROM || real_type_to_type(field_type) > FIELDTYPE_TEAR_TO); DBUG_ASSERT(field_type <= FIELDTYPE_LAST); field_type= real_type_to_type(field_type); if (field_type < FIELDTYPE_TEAR_FROM) return field_type; return FIELDTYPE_TEAR_FROM + (field_type - FIELDTYPE_TEAR_TO) - 1; } The ternary operator provides no benefit in this case, let's replace it with regular if statement. Same number of lines but the code is cleaner I'd say. > > static inline int field_type2index (enum_field_types field_type) > { > + DBUG_ASSERT(real_type_to_type(field_type) < FIELDTYPE_TEAR_FROM || > + real_type_to_type(field_type) > FIELDTYPE_TEAR_TO); > + DBUG_ASSERT(field_type <= FIELDTYPE_LAST); > field_type= real_type_to_type(field_type); > return (field_type < FIELDTYPE_TEAR_FROM ? > field_type : > @@ -94,6 +98,12 @@ static inline int field_type2index (enum_field_types field_type) > } > > > +/** > + Implements data type merge rules for the built-in traditional data types. > + - UNION > + - CASE and its abbreviations COALESCE, IF, IFNULL > + - LEAST/GREATEST > +*/ > I don't like this comment here as it's not complete. I'd much rather have it also explain how to interpret this table. I had to look up a calling spot to figure out exactly how it's done. I'd also have it say: Ex: Given Fields A and B of real_types a and b, we find the result type of COALESCE(A, B) by querying: field_types_merge_rules[field_type_to_index(a)][field_type_to_index(b)]. > > > static enum_field_types field_types_merge_rules [FIELDTYPE_NUM][FIELDTYPE_NUM]= > { > /* MYSQL_TYPE_DECIMAL -> */ > @@ -948,46 +927,18 @@ static enum_field_types field_types_merge_rules [FIELDTYPE_NUM][FIELDTYPE_NUM]= > enum_field_types Field::field_type_merge(enum_field_types a, > enum_field_types b) > { > - DBUG_ASSERT(real_type_to_type(a) < FIELDTYPE_TEAR_FROM || > - real_type_to_type(a) > FIELDTYPE_TEAR_TO); > - DBUG_ASSERT(real_type_to_type(b) < FIELDTYPE_TEAR_FROM || > - real_type_to_type(b) > FIELDTYPE_TEAR_TO); > return field_types_merge_rules[field_type2index(a)] > [field_type2index(b)]; > } > > - > -static Item_result field_types_result_type [FIELDTYPE_NUM]= > +const Type_handler * > +Type_handler::aggregate_for_result_traditional(const Type_handler *a, > + const Type_handler *b) > { > - //MYSQL_TYPE_DECIMAL MYSQL_TYPE_TINY > - DECIMAL_RESULT, INT_RESULT, > - //MYSQL_TYPE_SHORT MYSQL_TYPE_LONG > - INT_RESULT, INT_RESULT, > - //MYSQL_TYPE_FLOAT MYSQL_TYPE_DOUBLE > - REAL_RESULT, REAL_RESULT, > - //MYSQL_TYPE_NULL MYSQL_TYPE_TIMESTAMP > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_LONGLONG MYSQL_TYPE_INT24 > - INT_RESULT, INT_RESULT, > - //MYSQL_TYPE_DATE MYSQL_TYPE_TIME > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_DATETIME MYSQL_TYPE_YEAR > - STRING_RESULT, INT_RESULT, > - //MYSQL_TYPE_NEWDATE MYSQL_TYPE_VARCHAR > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_BIT <16>-<245> > - STRING_RESULT, > - //MYSQL_TYPE_NEWDECIMAL MYSQL_TYPE_ENUM > - DECIMAL_RESULT, STRING_RESULT, > - //MYSQL_TYPE_SET MYSQL_TYPE_TINY_BLOB > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_MEDIUM_BLOB MYSQL_TYPE_LONG_BLOB > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_BLOB MYSQL_TYPE_VAR_STRING > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_STRING MYSQL_TYPE_GEOMETRY > - STRING_RESULT, STRING_RESULT > -}; > + enum_field_types ta= a->real_field_type(); > + enum_field_types tb= b->real_field_type(); > + return Type_handler::get_handler_by_real_type(Field::field_type_merge(ta, tb)); Nit: this line is over 80 characters. Do we care? Most code tends to comply to it. It adds one extra line to split so it might not be worth it to adhere to a full 100% 80 character limit. > +} > > > /* > diff --git a/sql/field.h b/sql/field.h > index 541da5a..fa84e7d 100644 > --- a/sql/field.h > +++ b/sql/field.h > @@ -835,7 +835,6 @@ class Field: public Value_source > virtual Item_result cmp_type () const { return result_type(); } > static bool type_can_have_key_part(enum_field_types); > The field_type_merge function breaks all the other naming patterns. We have result_type, cmp_type real_type and now field_type_merge. Wouldn't it be better to name it field_merge_type, to be consistent? Or since this is a lookup kind of operation, perhaps name it (get|lookup)_merge_field_type? I know it's not _exactly_ like result_type and compare_type, but it generally is used in a simillar context. > > static enum_field_types field_type_merge(enum_field_types, enum_field_types); > - static Item_result result_merge_type(enum_field_types); > virtual bool eq(Field *field) > { > return (ptr == field->ptr && null_ptr == field->null_ptr && > diff --git a/sql/item.cc b/sql/item.cc > index c97f41f..c9b6155 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -9657,20 +9657,8 @@ Item_type_holder::Item_type_holder(THD *thd, Item *item) > maybe_null= item->maybe_null; > collation.set(item->collation); > get_full_info(item); > - /** > - Field::result_merge_type(real_field_type()) should be equal to > - result_type(), with one exception when "this" is a Item_field for > - a BIT field: > - - Field_bit::result_type() returns INT_RESULT, so does its Item_field. > - - Field::result_merge_type(MYSQL_TYPE_BIT) returns STRING_RESULT. > - Perhaps we need a new method in Type_handler to cover these type > - merging rules for UNION. > - */ > - DBUG_ASSERT(real_field_type() == MYSQL_TYPE_BIT || > - Item_type_holder::result_type() == > - Field::result_merge_type(Item_type_holder::real_field_type())); > /* fix variable decimals which always is NOT_FIXED_DEC */ > - if (Field::result_merge_type(real_field_type()) == INT_RESULT) > Alright so this seems to be fixed here, looking at Type_handler_bit, inheriting from Type_handler_int_result. Do we test this somewhere though? I couldn't find it in the test case, perhaps you can point it out to me. > > + if (Item_type_holder::result_type() == INT_RESULT) > decimals= 0; > prev_decimal_int_part= item->decimal_int_part(); > #ifdef HAVE_SPATIAL > @@ -9785,12 +9773,21 @@ bool Item_type_holder::join_types(THD *thd, Item *item) > DBUG_PRINT("info:", ("in type %d len %d, dec %d", > get_real_type(item), > item->max_length, item->decimals)); > - set_handler_by_real_type(Field::field_type_merge(real_field_type(), > - get_real_type(item))); > + const Type_handler *item_type_handler= > + Type_handler::get_handler_by_real_type(get_real_type(item)); > + if (aggregate_for_result(item_type_handler)) > + { > + my_error(ER_CANT_AGGREGATE_2TYPES, MYF(0), > + Item_type_holder::type_handler()->name().ptr(), > + item_type_handler->name().ptr(), > + "UNION"); > + DBUG_RETURN(true); > + } > + > { > uint item_decimals= item->decimals; > /* fix variable decimals which always is NOT_FIXED_DEC */ > - if (Field::result_merge_type(real_field_type()) == INT_RESULT) > + if (Item_type_holder::result_type() == INT_RESULT) > Ok, we're avoiding a virtual function call here. What I don't like is that this Item_type_holder class basically "abuses" the Item interface to such a degree that it even has to directly declare functions such as: double Item_type_holder::val_real() { DBUG_ASSERT(0); // should never be called return 0.0; } Let's discuss about cleaning this up later. To me it feels like this Item does not really belong in the Item class and should be factored out. Probably a whole project on its own :) > > > item_decimals= 0; > decimals= MY_MAX(decimals, item_decimals); > } > diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc > index 98b179b..e5e366e 100644 > --- a/sql/item_cmpfunc.cc > +++ b/sql/item_cmpfunc.cc > @@ -180,32 +179,40 @@ static int agg_cmp_type(Item_result *type, Item **items, uint nitems) > @return aggregated field type. > */ > > -enum_field_types agg_field_type(Item **items, uint nitems, > - bool treat_bit_as_number) > Why is this function in item_cmpfunc.cc and not in sql_type.cc? > +bool > +Type_handler_hybrid_field_type::aggregate_for_result(const char *funcname, > + Item **items, uint nitems, > + bool treat_bit_as_number) > { I would move uint i to be a local for variable. This is a C-style loop. Is there a compiler that doesn't support this in one of our builders? Also, how about size_t instead of uint? (Probably not necessary but wlad made a point of prefering to use that for iterators and such). > uint i; > if (!nitems || items[0]->result_type() == ROW_RESULT) > { > DBUG_ASSERT(0); > - return MYSQL_TYPE_NULL; > + set_handler(&type_handler_null); > + return true; > } > - enum_field_types res= items[0]->field_type(); > + set_handler(items[0]->type_handler()); > uint unsigned_count= items[0]->unsigned_flag; > for (i= 1 ; i < nitems ; i++) > { > - enum_field_types cur= items[i]->field_type(); > + const Type_handler *cur= items[i]->type_handler(); > if (treat_bit_as_number && > - ((res == MYSQL_TYPE_BIT) ^ (cur == MYSQL_TYPE_BIT))) > + ((type_handler() == &type_handler_bit) ^ (cur == &type_handler_bit))) > { > - if (res == MYSQL_TYPE_BIT) > - res= MYSQL_TYPE_LONGLONG; // BIT + non-BIT > + if (type_handler() == &type_handler_bit) > + set_handler(&type_handler_longlong); // BIT + non-BIT > else > - cur= MYSQL_TYPE_LONGLONG; // non-BIT + BIT > + cur= &type_handler_longlong; // non-BIT + BIT > + } > + if (aggregate_for_result(cur)) > + { > + my_error(ER_CANT_AGGREGATE_2TYPES, MYF(0), > + type_handler()->name().ptr(), cur->name().ptr(), funcname); > + return true; > } > - res= Field::field_type_merge(res, cur); > unsigned_count+= items[i]->unsigned_flag; > } > - switch (res) { > + switch (field_type()) { > case MYSQL_TYPE_TINY: > case MYSQL_TYPE_SHORT: > case MYSQL_TYPE_LONG: > diff --git a/sql/sql_type.cc b/sql/sql_type.cc > index 8746595..397b5cf 100644 > --- a/sql/sql_type.cc > +++ b/sql/sql_type.cc > @@ -54,6 +51,41 @@ static Type_handler_set type_handler_set; > Type_handler_null type_handler_null; > Type_handler_row type_handler_row; > Type_handler_varchar type_handler_varchar; > +Type_handler_newdecimal type_handler_newdecimal; > +Type_handler_longlong type_handler_longlong; > +Type_handler_bit type_handler_bit; > + > + I'm sure there's a better way to write this so that it gets initialized at compile time instead of at runtime (before main). Perhaps we can define the Type_aggregator differently. I need to look this Up. For now it will work. Standard offers no guarantees regarding the order, but it shouldn't matter for us as the address shouldn't change for global objects during initialization. "It is implementation-defined whether or not the dynamic initialization (8.5, 9.4, 12.1, 12.6.1) of an object of namespace scope is done before the first statement of main. If the initialization is deferred to some point in time after the first statement of main, it shall occur before the first use of any function or object defined in the same translation unit as the object to be initialized." > +Type_aggregator type_aggregator_for_result; > + > + > +class Static_data_initializer > +{ > +public: > + static Static_data_initializer m_singleton; > + Static_data_initializer() > + { > +#ifdef HAVE_SPATIAL > + type_aggregator_for_result.add(&type_handler_geometry, > + &type_handler_null, > + &type_handler_geometry); > + type_aggregator_for_result.add(&type_handler_geometry, > + &type_handler_geometry, > + &type_handler_geometry); > + type_aggregator_for_result.add(&type_handler_geometry, > + &type_handler_blob, > + &type_handler_long_blob); > + type_aggregator_for_result.add(&type_handler_geometry, > + &type_handler_varchar, > + &type_handler_long_blob); > + type_aggregator_for_result.add(&type_handler_geometry, > + &type_handler_string, > + &type_handler_long_blob); > +#endif > + } > +}; > + > +Static_data_initializer Static_data_initializer::m_singleton; > > > /** > diff --git a/sql/sql_type.h b/sql/sql_type.h > index 92dee61..4bc5542 100644 > --- a/sql/sql_type.h > +++ b/sql/sql_type.h > @@ -214,6 +214,31 @@ class Type_std_attributes > }; > > I don't like this class too much. One can easily break it by either changing LEX_CSTRING::str or LEX_CSTRING::length without changing the other one. I suggest we make the inheritance private so that the only way to access the members is through the methods available. A suggestion I have is to create a generic "String" class that has this same behaviour, without calling it Name. Afterwards typedefing it to Name. > > +class Name: public LEX_CSTRING > +{ > +public: > + Name(const char *str_arg, uint length_arg) > + { > + LEX_CSTRING::str= str_arg; > + LEX_CSTRING::length= length_arg; > + } Also, these ought to be removed if they are commented out. > + /* > + Name() > + { > + LEX_CSTRING::str= NULL; > + LEX_CSTRING::length= 0; > + } > + Name(const LEX_STRING &name) > + { > + LEX_CSTRING::str= name.str; > + LEX_CSTRING::length= name.length; > + } > + */ > + const char *ptr() const { return LEX_CSTRING::str; } > + uint length() const { return LEX_CSTRING::length; } > +}; > + > + > class Type_handler > { > protected: > @@ -235,6 +260,11 @@ class Type_handler > DBUG_ASSERT(type != TIME_RESULT); > return get_handler_by_cmp_type(type); > } > + static const > + Type_handler *aggregate_for_result_traditional(const Type_handler *h1, > + const Type_handler *h2); > + This function can return a const reference from a const static object within the class's namespace. Why create an object every time this is called? Compiler might optimize it, but why risk it? This goes for all the implementations. > + virtual const Name name() const= 0; > virtual enum_field_types field_type() const= 0; > virtual enum_field_types real_field_type() const { return field_type(); } > virtual Item_result result_type() const= 0; > @@ -884,5 +958,63 @@ class Type_handler_hybrid_real_field_type: > extern Type_handler_row type_handler_row; > extern Type_handler_null type_handler_null; > extern Type_handler_varchar type_handler_varchar; > +extern Type_handler_newdecimal type_handler_newdecimal; > +extern Type_handler_longlong type_handler_longlong; > +extern Type_handler_bit type_handler_bit; > + > +class Type_aggregator > +{ I'm not sure if Element is a good name for this. Aggregate_result_pair maybe? > + class Element > + { > + public: > + const Type_handler *m_handler1; > + const Type_handler *m_handler2; > + const Type_handler *m_result; > + Element() { } > + Element(const Type_handler *handler1, > + const Type_handler *handler2, > + const Type_handler *result) > + :m_handler1(handler1), m_handler2(handler2), m_result(result) > + { } > + bool eq(const Type_handler *handler1, const Type_handler *handler2) const > + { > + return m_handler1 == handler1 && m_handler2 == handler2; > + } > + }; So, we've discussed this before to change to a dynamic array. I prefer the template Dynamic_array over DYNAMIC_ARRAY. > + Element m_element_array[256]; > + uint m_element_count; > + const Element *element(uint i) const { return &m_element_array[i]; } > +public: > + Type_aggregator() > + :m_element_count(0) > + { } > + bool add(const Type_handler *handler1, > + const Type_handler *handler2, > + const Type_handler *result) > + { > + m_element_array[m_element_count]= Element(handler1, handler2, result); > + m_element_count++; > + return false; > + } > + const Element *find_element(const Type_handler *handler1, > + const Type_handler *handler2) const > + { > + for (uint i= 0; i < m_element_count; i++) > + { > + const Element *el= element(i); > + if (el->eq(handler1, handler2) || el->eq(handler2, handler1)) > + return el; > + } > + return NULL; > + } > + const Type_handler *find_handler(const Type_handler *handler1, > + const Type_handler *handler2) const > + { > + const Element *el= find_element(handler1, handler2); > + return el ? el->m_result : NULL; > + } > +}; > + > +extern Type_aggregator type_aggregator_for_result; > > #endif /* SQL_TYPE_H_INCLUDED */

2 2

Re: [Maria-developers] 33b01257190: MDEV-11584: GRANT inside an SP does not work well on 2nd execution
by Sergei Golubchik 28 Dec '16

28 Dec '16

Hi, Oleksandr! On Dec 21, Oleksandr Byelkin wrote: > revision-id: 33b01257190cef414a4b0f00bf36ee6a3ceda0a7 (mariadb-10.1.20-6-g33b01257190) > parent(s): 9e032d6150b39cf36a22e7c1503e06fae9c4016d > committer: Oleksandr Byelkin > timestamp: 2016-12-21 20:11:14 +0100 > message: > > MDEV-11584: GRANT inside an SP does not work well on 2nd execution > > Allocate password hash in statment memory > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 3f26bc2e08a..b44b120cf04 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -1067,7 +1067,14 @@ static bool fix_lex_user(THD *thd, LEX_USER *user) > make_scramble= my_make_scrambled_password; > } > > - char *buff= (char *) thd->alloc(scramble_length + 1); > + char *buff; > + { > + Query_arena *arena, backup; > + arena= thd->activate_stmt_arena_if_needed(&backup); > + buff= (char *) thd->alloc(scramble_length + 1); > + if (arena) > + thd->restore_active_arena(arena, &backup); > + } where is it called from, why does it happen during the parsing? > if (buff == NULL) > return true; > make_scramble(buff, user->pwtext.str, user->pwtext.length); Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] Please review MDEV-11672 mysql_list_field() returns wrong default values for VIEW
by Alexander Barkov 28 Dec '16

28 Dec '16

Sanja, On 12/27/2016 09:39 PM, Oleksandr Byelkin wrote: > Hi, Alexander! > > It is OK to push after fixing in the comment of the patch > mysql_list_fields with mysqld_list_fields (IMHO it is server function > do not set type etc.) Thanks for review! I fixed the comment. But then I realized that it's a good idea to add tests for other field types (e.g. DECIMAL, DATETIME, ENUM), and found that my fix was slightly incomplete: Item_ident_for_show::Item_ident_for_show() should copy not only collation from the field, but the entire Type_std_attributes (i.e. unsigned_flag, decimals and max_length). So I also fixed this, with some additional minor changes to avoid code duplication. This is an updated version. Please have a look. Thanks! > > On Tue, Dec 27, 2016 at 4:46 PM, Alexander Barkov <bar(a)mariadb.org > <mailto:bar@mariadb.org>> wrote: > > Hello Sanja, > > Can you please review a patch for MDEV-11672? > > Thanks! > >

1 0

Re: [Maria-developers] [Commits] 9416779: MDEV-11636 Extra persistent columns on slave always gets NULL in RBR
by Kristian Nielsen 28 Dec '16

28 Dec '16

abc <sachin.setiya(a)mariadb.com> writes: > I have updates the patch, please have a look, > revision-id: 941677928aa40aa1f5abf981b74d2c3c80441459 (mariadb-galera-10.0.28-8-g9416779) > parent(s): be430b80df0cdd4eba32df1570195721dbfd1b39 > author: Sachin Setiya > committer: Sachin Setiya > timestamp: 2016-12-27 15:40:28 +0530 > message: > > MDEV-11636 Extra persistent columns on slave always gets NULL in RBR The patch looks ok now. > Problem:- In replication if slave has extra persistent column then these > column are not computed while applying write-set from master. > > Solution:- While applying write-set from server, we will generate values > for extra persistent columns. "write-set" is a Galera term. In replication, they are refered to as binlog events, or row events in the specific case of row-based replication events. > diff --git a/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test b/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test > +--echo #SELECT quert > +--connection master > +select * from t1 where a > 2 and a < 4; > + > +--sync_slave_with_master > +select * from t1 where a > 2 and a < 4; You do not need to sync_slave_with_master for SELECT. > diff --git a/sql/rpl_record.cc b/sql/rpl_record.cc > index df8036c..21b8a80 100644 > --- a/sql/rpl_record.cc > +++ b/sql/rpl_record.cc > @@ -408,6 +408,12 @@ unpack_row(rpl_group_info *rgi, > } > > /* > + Add Extra slave persistent columns > + */ > + if ((error= fill_extra_persistent_columns(table, cols->n_bits))) > + DBUG_RETURN(error); You moved this from write_row() to unpack_row(), so I assume this was to fix the similar bug for updates? - Kristian.

2 1

[Maria-developers] check_sql_mode
by jerome brauge 27 Dec '16

27 Dec '16

Hello, I'm trying to add an option to sql_mode. For this, I'm adding in sql_class.h : #define MODE_CONCAT_NULL_YIELDS_NULL (1ULL << 32) But now, my serveur doesn't start and show "Sysvar 'sql_mode' failed 'def_val <= my_set_bits(typelib.count)'" The problem seems to be in sys_vars.ic (Sys_var_set) : { option.var_type|= GET_SET; global_var(ulonglong)= def_val; SYSVAR_ASSERT(typelib.count > 0); SYSVAR_ASSERT(typelib.count <= 64); SYSVAR_ASSERT(def_val <= my_set_bits(typelib.count)); SYSVAR_ASSERT(size == sizeof(ulonglong)); } Function my_set_bits works on 32bit and typelib.count may be up to 64. In my case typelib.count is 33 and my_set_bits(33) returns 1, and the assert is false. What is my mistake ? Best regards, Jérôme.

3 6

[Maria-developers] Please review MDEV-11672 mysql_list_field() returns wrong default values for VIEW
by Alexander Barkov 27 Dec '16

27 Dec '16

Hello Sanja, Can you please review a patch for MDEV-11672? Thanks!

1 0

[Maria-developers] MDEV-11636 Extra persistent columns on slave always gets NULL in RBR
by Sachin Setiya 27 Dec '16

27 Dec '16

Hi Kristian, I have updates the patch, please have a look, http://lists.askmonty.org/pipermail/commits/2016-December/010338.html. I also created a branch bb-10-0-gal-sachin , where I pushed this commit . Build bot status:- https://buildbot.askmonty.org/buildbot/grid?category=main&branch=bb-10.0-ga… -- Regards Sachin Setiya Software Engineer at MariaDB

1 0

Re: [Maria-developers] [Commits] b60fb6d: MDEV-11636 Extra persistent columns on slave always gets NULL in RBR
by Kristian Nielsen 22 Dec '16

22 Dec '16

sachin.setiya(a)mariadb.com writes: > revision-id: b60fb6daee2fcc2f433a50b5a5639065f5a46fe8 (mariadb-galera-10.0.28-8-gb60fb6d) > parent(s): be430b80df0cdd4eba32df1570195721dbfd1b39 > author: Sachin Setiya > committer: Sachin Setiya > timestamp: 2016-12-22 19:22:36 +0530 > message: > > MDEV-11636 Extra persistent columns on slave always gets NULL in RBR > > Problem:- In replication if slave has extra persistent column then these > column are not computed while applying write-set from master. Generally, when slave and master differs there will be situations where things will not fully work. However, in this case, the result is really corrupt data (a persistent column with wrong values), and I agree this should be fixed (and the patch looks simple). I am not familiar with the code for virtual columns. I assume you looked at the normal code path for virtual columns and made similar functionality for row-based replication applier. Your code looks reasonable to me. You add code in Rows_log_event::write_row(). What about update_row? It is not tested in the testcase, you should add test of the UPDATE case (and fix the code, if needed). The testcase needs more work, see comments inline. Getting replication tests to work reliably (not generate occasional spurious errors on buildbot etc.) is quite tricky (spoken from painful experience :-). Please push this to a feature tree and make sure the test passes on all builders before pushing to main. (A useful trick also is to run on your own machine something like: ./mysql-test-run.pl rpl.rpl_alter_extra_persistent --repeat=1000 to catch at least some timing-dependent errors). > diff --git a/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test b/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test > +--sync_slave_with_master > + > +connection slave; > +select * from t1; Use ORDER BY a (here and below), to avoid any risk of getting different output order. > +--connection master > +insert into t1 values(5); > +insert into t1 values(6); > + > +--connection slave > +sleep 2; Never use sleep in test cases. Use an appropriate mechanism to sync the slave to the master. > +select * from t1; > + > +--echo #Break Unique Constraint > +alter table t1 add column z4 int as (a % 6) persistent unique; > + > +--connection master > + > +--echo #entering duplicate value for slave persistent column > +insert into t1 values(7); > +select * from t1; > + > +--connection slave > +select * from t1; > +--query_vertical show slave status; This will not work reliably. You cannot put SHOW SLAVE STATUS output in .result, it will not be stable between test runs. And there is a race on whether the slave has time to hit the duplicate error. For this, use the existing include/wait_for_slave_sql_error.inc file, which can reliably detect a slave failure. See comments in the file and example usage in other test files. > + > +--connection master > +drop table t1; > + > +--connection slave > +alter table t1 drop column z4 ; > +start slave; > +drop table t1; This drop table should not be there (the drop table from the master will be replicated). > diff --git a/sql/log_event.cc b/sql/log_event.cc > index 1cc58cd..a8f035b 100644 > --- a/sql/log_event.cc > +++ b/sql/log_event.cc > @@ -11541,6 +11541,9 @@ Rows_log_event::write_row(rpl_group_info *rgi, > DBUG_RETURN(error); > } > > + if ((error= fill_extra_persistent_columns(table, &m_cols)) > + DBUG_RETURN(error); > + Is similar code needed for update_row? At least, this should be tested in the test case. - Kristian.

1 0

[Maria-developers] aria engine crash at recovery
by Roberto Spadim 22 Dec '16

22 Dec '16

hi guys i got a crash and aria engine don't startup, any help? 161222 03:41:13 mysqld_safe Starting mysqld daemon with databases from /home/mysql/data/local/ 2016-12-22 3:41:13 139878188566400 [Warning] 'THREAD_CONCURRENCY' is deprecated and will be removed in a future release. 2016-12-22 3:41:13 139878188566400 [Note] /usr/mariadb/mariadb/bin//mysqld (mysqld 10.1.5-MariaDB-log) starting as process 19217 ... 2016-12-22 3:41:13 139878188566400 [Note] mysqld: Aria engine: starting recovery recovered pages: 0% 3082852397% 0%161222 3:41:13 [ERROR] mysqld got signal 11 ; This could be because you hit a bug. It is also possible that this binary or one of the libraries it was linked against is corrupt, improperly built, or misconfigured. This error can also be caused by malfunctioning hardware. To report this bug, see http://kb.askmonty.org/en/reporting-bugs We will try our best to scrape up some info that will hopefully help diagnose the problem, but since we have already crashed, something is definitely wrong and this may fail. Server version: 10.1.5-MariaDB-log key_buffer_size=629145600 read_buffer_size=4194304 max_used_connections=0 max_threads=52 thread_count=0 It is possible that mysqld could use up to key_buffer_size + (read_buffer_size + sort_buffer_size)*max_threads = 1041422 K bytes of memory Hope that's ok; if not, decrease some variables in the equation. Thread pointer: 0x0x0 Attempting backtrace. You can use the following information to find out where mysqld died. If you see no messages after this, something went terribly wrong... stack_bottom = 0x0 thread_stack 0x48000 mysys/stacktrace.c:247(my_print_stacktrace)[0x7f37ee645b0b] sql/signal_handler.cc:153(handle_fatal_signal)[0x7f37ee1dfdb5] /lib64/libpthread.so.0(+0x10d00)[0x7f37ecd72d00] maria/ma_recovery.c:2142(exec_REDO_LOGREC_CLR_END)[0x7f37ee5683b7] maria/ma_recovery.c:588(display_and_apply_record)[0x7f37ee56985d] maria/ma_recovery.c:350(maria_apply_log)[0x7f37ee56d45c] maria/ma_recovery.c:242(maria_recovery_from_log)[0x7f37ee56df1e] maria/ha_maria.cc:3594(ha_maria_init)[0x7f37ee546c2d] sql/handler.cc:512(ha_initialize_handlerton(st_plugin_int*))[0x7f37ee1e2518] sql/sql_plugin.cc:1403(plugin_initialize)[0x7f37ee089e65] sql/sql_plugin.cc:1675(plugin_init(int*, char**, int))[0x7f37ee08add2] sql/mysqld.cc:5064(init_server_components)[0x7f37edfc37e8] sql/mysqld.cc:5659(mysqld_main(int, char**))[0x7f37edfc61ca] /lib64/libc.so.6(__libc_start_main+0xf0)[0x7f37ec3a79f0] /usr/mariadb/mariadb/bin//mysqld(+0x39dbad)[0x7f37edfbabad]

1 4

[Maria-developers] How correctly allocate memory to avoid problems with 2nd execution (instruction)
by Oleksandr Byelkin 22 Dec '16

22 Dec '16

Hi! It is for all who decide to write or already is writing something for server. There is 2 ways to execute query/statement: 1) conventional (one time) 2) with storing the statement and executing it many times (Prepared statements (PS)/ Stored Procedures(SP)) Also there is so called Query_arena, which is a MEM_ROOT and list of allocated Item-s in it. (we need list of items because they could require not trivial destruction/cleanup procedure before freeing memory, see cleanup() methods of Item-s) For conventional execution there is only one (run-time) query arena (part of THD in this case) where all memory allocated, and all Item-s linked. After execution Item-s cleaned up and memory freed. For case with stored statements (PS/SP) there is one Query_arena where Parsing result allocated (statement arena), and the other one so called run-time arena (which freed after execution as for conventional execution). During parsing statement arena is active (default), during all execution steps the run-time arena is active (if nothing changed it specially). So if some one decided allocate some memory during execution one should think if it is: I) only for execution II) should live with the statement All references on the memory allocated as case I) should be returned in the state as it was before execution or taken other measures to prevent using them during the next execution. All object which allocated as persistent (for statement of course) should be a) allocated only once (can be first execution or prepare phase, it does not matter, what does matter: do not make memory leak by allocating on each execution) b) reference on them should not be lost (because it supposed to be reused during next execution) c) procedure of allocation during run-time should looks like this: Query_arena *arena, backup; // backup run-time arena and set as default statement arena if needed // (if it is not conventional execution) arena= thd->activate_stmt_arena_if_needed(&backup);\ <MAKE ALLOOCATION HERE USING thd->mem_root> // return old default arena if it was changed before if (arena) thd->restore_active_arena(arena, &backup);

1 0

[Maria-developers] Fwd: Update of RPM layout
by Michal Schorm 21 Dec '16

21 Dec '16

Hello all, I went through an idea [1] to lighten the server package. I tested it, and I found out, I can save about 40MB of space, if I'd move all Perl utilites to a standalone subpackage. It would make sense to move them, so the users don't need to install Perl with the server package. Especially in technologies like containers (or cloud images) size matters. It all depends on how often are those utilities [2] used together with the server, sice we don't want to displace importatnt scripts. In case, it would look like a good idea, there is also option to move to this subpackage other - infrequently used - utilities. -> Q: Hey, Users and Admins, which utilities do you think are essential for the server? Note: It can be solved by soft dependencies, so those utils would be installed with the server by default. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1205082 [2] mysql_find_rows mysqlaccess mysql_convert_table_format mysql_fix_extensions mysql_setpermission mysql_zap mysqldumpslow mysqld_multi mysqlhotcopy -- Michal Schorm Core Services - Databases Team mail: mschorm(a)redhat.com Brno-IRC: mschorm

1 0

[Maria-developers] Please review MDEV-10386 Assertion `fixed == 1' failed in virtual String* Item_func_conv_charset::val_str(String*)
by Alexander Barkov 21 Dec '16

21 Dec '16

Hello Sergei, Please review a patch for MDEV-10386. Thanks!

2 3

[Maria-developers] mysql_secure_installation documentation
by Michal Schorm 20 Dec '16

20 Dec '16

Hello all, I took care about bug against "mysql_secure_installation" script. [1] The problem is pretty simple. When you use "mysql_secure_installation" script, it will ingore all of yours current configuration. However, there are some options, that can be used. One of them is used to add your custom config file. It is documented, for example, here: [2] Unfortunatelly, the "man mysql_secure_installation" is quite short of infromation. (same as [3]) I believe, the information about options really should be added to both manpage and [3]. That should also solve the bug, without any [1] https://bugzilla.redhat.com/show_bug.cgi?id=1260821 [2] https://dev.mysql.com/doc/refman/5.7/en/mysql-secure-installation.html [3] https://mariadb.com/kb/en/mariadb/mysql_secure_installation/ -- I would like your general opinion about this. Based on it, I can file issue on JIRA. -- Michal Schorm Core Services - Databases Team mail: mschorm(a)redhat.com Brno-IRC: mschorm

1 0

[Maria-developers] Please review MDEV-11615 Split Item_hybrid_func::fix_attributes into virtual methods in Type_handler
by Alexander Barkov 20 Dec '16

20 Dec '16

Hello Sanja, Please review a patch for MDEV-11615. Thanks!

1 0

[Maria-developers] 10.3 branches
by Alexander Barkov 20 Dec '16

20 Dec '16

Hello Serg, all, A few weeks ago I created a new branch "10.3" and pushed essential patches needed for the compatibility project. It worked as follows: - In 10.3: git rebase origin/10.2 git push --force - In bb-10.2-compatibility: git rebase origin/10.3 git push --force Now as other developers start to push into 10.3, this won't work anymore. We briefly discussed with Monty that, as the compatibility project should be as stable as possible and merging all 10.3 patches into bb-10.2-compatibility is not desirable, we could try something like this: Add a new branch (Monty proposed "10.3-base" as the name) and propagate changes as follows: - In 10.3-base: git rebase origin/10.2 git push --force - In bb-10.2-compatibility git rebase origin/10.3-base git push --force - In 10.3: git merge origin/10.3-base git push (no --force) 10.2 ---(rebase)--->10.3-base--(rebase)-->bb-10.2-compatibility | | (merge) (merge) | | V | 10.3<-----------------/ Any objections about this scheme? I'd only propose to use "10.2-ext" instead of "10.3-base", as this intermediate branch is going to be more 10.2 than 10.3. Thanks.

3 6

Re: [Maria-developers] [Commits] 9f863a15b03: MDEV-11602 InnoDB leaks foreign key metadata on DDL operations
by Jan Lindström 19 Dec '16

19 Dec '16

ok to push. Most likely the double free was result of some other bug fixed later. R: Jan On Mon, Dec 19, 2016 at 4:00 PM, <marko.makela(a)mariadb.com> wrote: > revision-id: 9f863a15b037c0ddffc2d12c1f7d841466f9f345 (mariadb-10.0.28-27- > g9f863a15b03) > parent(s): eb4f2e063c341d9f3644339c68cb01679e782001 > author: Marko Mäkelä > committer: Marko Mäkelä > timestamp: 2016-12-19 15:57:41 +0200 > message: > > MDEV-11602 InnoDB leaks foreign key metadata on DDL operations > > Essentially revert MDEV-6759, which addressed a double free of memory > by removing the freeing altogether, introducing the memory leaks. > No double free was observed when running the test suite -DWITH_ASAN. > > Replace some mem_heap_free(foreign->heap) with dict_foreign_free(foreign) > so that the calls can be located and instrumented more easily when needed. > > --- > storage/innobase/dict/dict0dict.cc | 8 ++++---- > storage/innobase/dict/dict0load.cc | 2 +- > storage/xtradb/dict/dict0dict.cc | 8 ++++---- > storage/xtradb/dict/dict0load.cc | 2 +- > 4 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/ > dict0dict.cc > index f75d584e70d..2e40946224e 100644 > --- a/storage/innobase/dict/dict0dict.cc > +++ b/storage/innobase/dict/dict0dict.cc > @@ -1614,6 +1614,7 @@ struct dict_foreign_remove_partial > if (table != NULL) { > table->referenced_set.erase(foreign); > } > + dict_foreign_free(foreign); > } > }; > > @@ -3535,8 +3536,7 @@ dict_foreign_add_to_cache( > } > > if (for_in_cache) { > - /* Free the foreign object */ > - mem_heap_free(foreign->heap); > + dict_foreign_free(foreign); > } else { > for_in_cache = foreign; > } > @@ -3564,7 +3564,7 @@ dict_foreign_add_to_cache( > " the ones in table."); > > if (for_in_cache == foreign) { > - mem_heap_free(foreign->heap); > + dict_foreign_free(foreign); > } > > return(DB_CANNOT_ADD_CONSTRAINT); > @@ -3620,7 +3620,7 @@ dict_foreign_add_to_cache( > be one */ > } > > - mem_heap_free(foreign->heap); > + dict_foreign_free(foreign); > } > > return(DB_CANNOT_ADD_CONSTRAINT); > diff --git a/storage/innobase/dict/dict0load.cc b/storage/innobase/dict/ > dict0load.cc > index e68bbe7d10a..77a31e5de63 100644 > --- a/storage/innobase/dict/dict0load.cc > +++ b/storage/innobase/dict/dict0load.cc > @@ -489,7 +489,7 @@ dict_process_sys_foreign_rec( > } > > /* This recieves a dict_foreign_t* that points to a stack variable. > - So mem_heap_free(foreign->heap) is not used as elsewhere. > + So dict_foreign_free(foreign) is not used as elsewhere. > Since the heap used here is freed elsewhere, foreign->heap > is not assigned. */ > foreign->id = mem_heap_strdupl(heap, (const char*) field, len); > diff --git a/storage/xtradb/dict/dict0dict.cc b/storage/xtradb/dict/ > dict0dict.cc > index 5339ecfaed6..3845138c8bf 100644 > --- a/storage/xtradb/dict/dict0dict.cc > +++ b/storage/xtradb/dict/dict0dict.cc > @@ -1614,6 +1614,7 @@ struct dict_foreign_remove_partial > if (table != NULL) { > table->referenced_set.erase(foreign); > } > + dict_foreign_free(foreign); > } > }; > > @@ -3539,8 +3540,7 @@ dict_foreign_add_to_cache( > } > > if (for_in_cache) { > - /* Free the foreign object */ > - mem_heap_free(foreign->heap); > + dict_foreign_free(foreign); > } else { > for_in_cache = foreign; > } > @@ -3564,7 +3564,7 @@ dict_foreign_add_to_cache( > " the ones in table."); > > if (for_in_cache == foreign) { > - mem_heap_free(foreign->heap); > + dict_foreign_free(foreign); > } > > return(DB_CANNOT_ADD_CONSTRAINT); > @@ -3620,7 +3620,7 @@ dict_foreign_add_to_cache( > be one */ > } > > - mem_heap_free(foreign->heap); > + dict_foreign_free(foreign); > } > > return(DB_CANNOT_ADD_CONSTRAINT); > diff --git a/storage/xtradb/dict/dict0load.cc b/storage/xtradb/dict/ > dict0load.cc > index ca7de72c9b9..9ea4f4d873a 100644 > --- a/storage/xtradb/dict/dict0load.cc > +++ b/storage/xtradb/dict/dict0load.cc > @@ -491,7 +491,7 @@ dict_process_sys_foreign_rec( > } > > /* This recieves a dict_foreign_t* that points to a stack variable. > - So mem_heap_free(foreign->heap) is not used as elsewhere. > + So dict_foreign_free(foreign) is not used as elsewhere. > Since the heap used here is freed elsewhere, foreign->heap > is not assigned. */ > foreign->id = mem_heap_strdupl(heap, (const char*) field, len); > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

Re: [Maria-developers] [Commits] 8bfc350: MDEV-10148: Database crashes in the query to the View
by Oleksandr Byelkin 19 Dec '16

19 Dec '16

Hi, Sergei! On 12/16/2016 03:16 PM, Sergei Petrunia wrote: > revision-id: 8bfc35041195b46c8f4666dd02f600e878f45643 > parent(s): 96d2e0480ffbc36c803db5a7d5bcefd3470c5f72 > committer: Sergei Petrunia > branch nick: 5.5 > timestamp: 2016-12-16 17:16:02 +0300 > message: > > MDEV-10148: Database crashes in the query to the View > > Fix st_select_lex::is_merged_child_of to work across merged views or > derived tables. > OK to push! [skip]

1 0

[Maria-developers] Please review MDEV-10524 Assertion `arg1_int >= 0' failed in Item_func_additive_op::result_precision()
by Alexander Barkov 19 Dec '16

19 Dec '16

Hello Sergei, Please review a patch for MDEV-10524. Thanks!

2 2

Re: [Maria-developers] [Commits] 12034ae: MDEV-10927: Crash When Using sort_union Optimization
by Sergey Petrunia 16 Dec '16

16 Dec '16

The patch is ok, but there is a problem with the testcase: when I run the testcase on the unmodified 5.5 binary, it passes. When I run the tescase from the MDEV, I get the crash. Please fix the testcase. This will also provide test coverage (which was lacking, and this is why have this bug in the first place). On Fri, Dec 16, 2016 at 11:19:43AM +0530, Varun wrote: > revision-id: 12034aec831d776725a43c7537ab4256efb1c974 (mariadb-5.5.53-25-g12034ae) > parent(s): 14e1f32894cdbe63a614738cfd93e9d0818dedee > author: Varun Gupta > committer: Varun Gupta > timestamp: 2016-12-16 11:03:24 +0530 > message: > > MDEV-10927: Crash When Using sort_union Optimization > > In file sql/filesort.cc,when merge_buffers() is called then > - queue_remove(&queue,0) is called > - For the function queue_remove there is assertion states that the element to be removed should have index >=1 > - this is causing the assertion to fail. > > Fixed by removing the top element. > > --- > mysql-test/r/index_merge_innodb.result | 52 ++++++++++++++++++++++++++++++++++ > mysql-test/t/index_merge_innodb.test | 42 ++++++++++++++++++++++++++- > sql/filesort.cc | 2 +- > 3 files changed, 94 insertions(+), 2 deletions(-) > > diff --git a/mysql-test/r/index_merge_innodb.result b/mysql-test/r/index_merge_innodb.result > index b93d15f..49e0a57 100644 > --- a/mysql-test/r/index_merge_innodb.result > +++ b/mysql-test/r/index_merge_innodb.result > @@ -792,4 +792,56 @@ a b c > 8 m m > 9 d d > DROP TABLE t1; > +# > +# MDEV-10927: Crash When Using sort_union Optimization > +# > +SET optimizer_switch='index_merge_sort_intersection=on'; > +CREATE TABLE IF NOT EXISTS t1 ( > +id int(11) NOT NULL AUTO_INCREMENT, > +contact_id int(11) NOT NULL, > +role_id int(11) NOT NULL, > +company_type_id int(11) NOT NULL, > +company_id int(11) NOT NULL, > +team_id int(11) NOT NULL, > +user_id int(11) NOT NULL, > +PRIMARY KEY (id), > +KEY contact_id (contact_id), > +KEY user_id (user_id), > +KEY company_id (company_id), > +KEY team_id (team_id) USING BTREE > +) ENGINE=InnoDB AUTO_INCREMENT=12860259 DEFAULT CHARSET=utf8 ROW_FORMAT=COMPACT; > +INSERT INTO t1 (id, contact_id, role_id, company_type_id, company_id, team_id, user_id) VALUES > +(5343408, 23913609, 0, 6, 2321, 0, 0), > +(7841449, 39582328, 1194, 0, 0, 0, 57291), > +(7841507, 28707292, 1194, 0, 0, 0, 57291), > +(7841543, 26376778, 1194, 0, 0, 0, 137571), > +(7841563, 42475686, 1194, 0, 0, 0, 137571), > +(7841573, 35956831, 1194, 0, 0, 0, 447223), > +(7841575, 37975231, 1194, 0, 0, 0, 447223), > +(7843049, 42693923, 1194, 0, 0, 0, 376261), > +(7843600, 42696877, 1194, 0, 0, 0, 376261), > +(12858344, 82851915, 1194, 0, 0, 0, 993011), > +(12858554, 80183799, 1194, 0, 0, 0, 993011), > +(12858803, 82649193, 1194, 0, 0, 0, 1288749), > +(12858878, 82700925, 1194, 0, 0, 0, 1288749), > +(12860248, 28642729, 1194, 0, 0, 0, 1234288), > +(12860258, 28642729, 0, 0, 0, 0, 1234288), > +(12860259, 28642729, 1194, 0, 0, 0, 0); > +SELECT SQL_NO_CACHE contact_id FROM t1 FORCE INDEX (user_id,company_id) WHERE user_id = '57291' OR (user_id IN(55838,55839,56998,57260,57291,60227,121997,137571,173600,219233,306480,354991,358594,376261,398572,447223,472009,646282,932548,993011,1004609,1021262,1177117,1234288,1283121,1288749,1337771,57291) AND role_id = '1194') OR company_id = '2321'; > +contact_id > +23913609 > +26376778 > +28642729 > +28707292 > +35956831 > +37975231 > +39582328 > +42475686 > +42693923 > +42696877 > +80183799 > +82649193 > +82700925 > +82851915 > +DROP TABLE t1; > set optimizer_switch= @optimizer_switch_save; > diff --git a/mysql-test/t/index_merge_innodb.test b/mysql-test/t/index_merge_innodb.test > index 6a1cb53..f1fdbbd 100644 > --- a/mysql-test/t/index_merge_innodb.test > +++ b/mysql-test/t/index_merge_innodb.test > @@ -172,5 +172,45 @@ WHERE ( tb.b != ta.b OR tb.a = ta.a ) > > DROP TABLE t1; > > -set optimizer_switch= @optimizer_switch_save; > +--echo # > +--echo # MDEV-10927: Crash When Using sort_union Optimization > +--echo # > > +SET optimizer_switch='index_merge_sort_intersection=on'; > + > +CREATE TABLE IF NOT EXISTS t1 ( > + id int(11) NOT NULL AUTO_INCREMENT, > + contact_id int(11) NOT NULL, > + role_id int(11) NOT NULL, > + company_type_id int(11) NOT NULL, > + company_id int(11) NOT NULL, > + team_id int(11) NOT NULL, > + user_id int(11) NOT NULL, > + PRIMARY KEY (id), > + KEY contact_id (contact_id), > + KEY user_id (user_id), > + KEY company_id (company_id), > + KEY team_id (team_id) USING BTREE > + ) ENGINE=InnoDB AUTO_INCREMENT=12860259 DEFAULT CHARSET=utf8 ROW_FORMAT=COMPACT; > + > +INSERT INTO t1 (id, contact_id, role_id, company_type_id, company_id, team_id, user_id) VALUES > + (5343408, 23913609, 0, 6, 2321, 0, 0), > + (7841449, 39582328, 1194, 0, 0, 0, 57291), > + (7841507, 28707292, 1194, 0, 0, 0, 57291), > + (7841543, 26376778, 1194, 0, 0, 0, 137571), > + (7841563, 42475686, 1194, 0, 0, 0, 137571), > + (7841573, 35956831, 1194, 0, 0, 0, 447223), > + (7841575, 37975231, 1194, 0, 0, 0, 447223), > + (7843049, 42693923, 1194, 0, 0, 0, 376261), > + (7843600, 42696877, 1194, 0, 0, 0, 376261), > + (12858344, 82851915, 1194, 0, 0, 0, 993011), > + (12858554, 80183799, 1194, 0, 0, 0, 993011), > + (12858803, 82649193, 1194, 0, 0, 0, 1288749), > + (12858878, 82700925, 1194, 0, 0, 0, 1288749), > + (12860248, 28642729, 1194, 0, 0, 0, 1234288), > + (12860258, 28642729, 0, 0, 0, 0, 1234288), > + (12860259, 28642729, 1194, 0, 0, 0, 0); > +--sorted_result > +SELECT SQL_NO_CACHE contact_id FROM t1 FORCE INDEX (user_id,company_id) WHERE user_id = '57291' OR (user_id IN(55838,55839,56998,57260,57291,60227,121997,137571,173600,219233,306480,354991,358594,376261,398572,447223,472009,646282,932548,993011,1004609,1021262,1177117,1234288,1283121,1288749,1337771,57291) AND role_id = '1194') OR company_id = '2321'; > +DROP TABLE t1; > +set optimizer_switch= @optimizer_switch_save; > diff --git a/sql/filesort.cc b/sql/filesort.cc > index 5bb5c64..38404b0 100644 > --- a/sql/filesort.cc > +++ b/sql/filesort.cc > @@ -1411,7 +1411,7 @@ int merge_buffers(SORTPARAM *param, IO_CACHE *from_file, > if (!(error= (int) read_to_buffer(from_file, buffpek, > rec_length))) > { > - queue_remove(&queue,0); > + (void) queue_remove_top(&queue); > reuse_freed_buff(&queue, buffpek, rec_length); > } > else if (error == -1) BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] MDEV-11558 Split Item_type_holder::display_length into virtual methods in Type_handler
by Alexander Barkov 16 Dec '16

16 Dec '16

Hello Nirbhay, Can you please review a patch for MDEV-11558? Thanks!

2 1

Re: [Maria-developers] [Commits] 5f02c66: MDEV-11227 - mysqlimport -l doesn't issue UNLOCK TABLES
by Sergei Golubchik 15 Dec '16

15 Dec '16

Hi, Sergey! On Dec 13, Sergey Vojtovich wrote: > revision-id: 5f02c661f3d30baf08071acad29cef40fad0739c (mariadb-10.2.2-225-g5f02c66) > parent(s): 65b4d7457e40ee25302d2df28b0d200ff59d9e6d > committer: Sergey Vojtovich > timestamp: 2016-12-13 16:17:49 +0400 > message: > > MDEV-11227 - mysqlimport -l doesn't issue UNLOCK TABLES > > Implementation of MDEV-7660 introduced unwanted incompatible change: > modifications under LOCK TABLES with autocommit enabled are rolled back on > disconnect. Previously everything was committed, because LOCK TABLES didn't > adjust autocommit setting. Thanks, very helpful comment (that I didn't quote in my reply for brevity)! Few questions below: > diff --git a/mysql-test/t/delayed.test b/mysql-test/t/delayed.test > index dea16c8..30fb694 100644 > --- a/mysql-test/t/delayed.test > +++ b/mysql-test/t/delayed.test > @@ -319,7 +319,7 @@ drop table if exists t1; > --enable_warnings > create table t1 (a int, b int); > insert into t1 values (1,1); > -lock table t1 read; > +lock table t1 read local; why? > connect (update,localhost,root,,); > connection update; > --send insert delayed into t1 values (2,2); > diff --git a/sql/mdl.h b/sql/mdl.h > index 7d659af..d77c07c 100644 > --- a/sql/mdl.h > +++ b/sql/mdl.h > @@ -196,6 +196,12 @@ enum enum_mdl_type { > */ > MDL_SHARED_UPGRADABLE, > /* > + A shared metadata lock for cases when we need to read data from table > + and block all concurrent modifications to it (for both data and metadata). > + Used by LOCK TABLES READ statement. > + */ > + MDL_SHARED_READ_ONLY, Hmm, stronger than MDL_SHARED_WRITE? All read locks were before write locks until now. Why is that changed? > + /* > An upgradable shared metadata lock which blocks all attempts to update > table data, allowing reads. > A connection holding this kind of lock can read table metadata and read > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 1ca7e9c..d3f4517 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -1496,6 +1496,7 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) > to general/slow log would be disabled in read only transactions. > */ > if (table_list->mdl_request.type >= MDL_SHARED_WRITE && > + table_list->mdl_request.type != MDL_SHARED_READ_ONLY && if you're going to keep read and write locks mixed, then I'd suggest to move this condition into a small inline helper, like table_list->mdl_request.is_write_lock(). it's used few times already in this very file and there will be only one place to change if we'll need to add, say, MDL_SHARED_WRITE_LOW_PRIO > thd->tx_read_only && > !(flags & (MYSQL_LOCK_LOG_TABLE | MYSQL_OPEN_HAS_MDL_LOCK))) > { > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 6dc9192..0d740bb 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -2728,27 +2728,76 @@ bool sp_process_definer(THD *thd) > static bool lock_tables_open_and_lock_tables(THD *thd, TABLE_LIST *tables) > { > Lock_tables_prelocking_strategy lock_tables_prelocking_strategy; > + MDL_deadlock_and_lock_abort_error_handler deadlock_handler; > + MDL_savepoint mdl_savepoint= thd->mdl_context.mdl_savepoint(); > uint counter; > TABLE_LIST *table; > > thd->in_lock_tables= 1; > > +retry: > + > if (open_tables(thd, &tables, &counter, 0, &lock_tables_prelocking_strategy)) > goto err; > > - /* > - We allow to change temporary tables even if they were locked for read > - by LOCK TABLES. To avoid a discrepancy between lock acquired at LOCK > - TABLES time and by the statement which is later executed under LOCK TABLES > - we ensure that for temporary tables we always request a write lock (such > - discrepancy can cause problems for the storage engine). > - We don't set TABLE_LIST::lock_type in this case as this might result in > - extra warnings from THD::decide_logging_format() even though binary logging > - is totally irrelevant for LOCK TABLES. > - */ > for (table= tables; table; table= table->next_global) > - if (!table->placeholder() && table->table->s->tmp_table) > - table->table->reginfo.lock_type= TL_WRITE; > + { > + if (!table->placeholder()) > + { > + if (table->table->s->tmp_table) > + { > + /* > + We allow to change temporary tables even if they were locked for read > + by LOCK TABLES. To avoid a discrepancy between lock acquired at LOCK > + TABLES time and by the statement which is later executed under LOCK > + TABLES we ensure that for temporary tables we always request a write > + lock (such discrepancy can cause problems for the storage engine). > + We don't set TABLE_LIST::lock_type in this case as this might result > + in extra warnings from THD::decide_logging_format() even though > + binary logging is totally irrelevant for LOCK TABLES. > + */ > + table->table->reginfo.lock_type= TL_WRITE; > + } > + else if (table->mdl_request.type == MDL_SHARED_READ && > + ! table->prelocking_placeholder && > + table->table->file->lock_count() == 0) > + { > + /* > + In case when LOCK TABLE ... READ LOCAL was issued for table with > + storage engine which doesn't support READ LOCAL option and doesn't > + use THR_LOCK locks we need to upgrade weak SR metadata lock acquired > + in open_tables() to stronger SRO metadata lock. > + This is not needed for tables used through stored routines or > + triggers as we always acquire SRO (or even stronger SNRW) metadata > + lock for them. > + */ > + deadlock_handler.init(); > + thd->push_internal_handler(&deadlock_handler); > + > + bool result= thd->mdl_context.upgrade_shared_lock( > + table->table->mdl_ticket, > + MDL_SHARED_READ_ONLY, > + thd->variables.lock_wait_timeout); > + > + thd->pop_internal_handler(); > + > + if (deadlock_handler.need_reopen()) > + { > + /* > + Deadlock occurred during upgrade of metadata lock. > + Let us restart acquring and opening tables for LOCK TABLES. will you use MDL_SHARED_READ_ONLY right away when retrying? or it'll do the upgrade thing again? > + */ > + close_tables_for_reopen(thd, &tables, mdl_savepoint); > + if (thd->open_temporary_tables(tables)) > + goto err; > + goto retry; > + } > + > + if (result) > + goto err; > + } > + } > + } > > if (lock_tables(thd, tables, counter, 0) || > thd->locked_tables_list.init_locked_tables(thd)) Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 3

[Maria-developers] MDEV-11514, MDEV-11497, MDEV-11554, MDEV-11555 - IN and CASE type aggregation problems
by Alexander Barkov 15 Dec '16

15 Dec '16

Hello Sanja, Please review a patch fixing a number of problems in IN and CASE. This is a per-requisite for further Type_handler related work, to clean-up the code before moving it to more IN and CASE related methods in Type_handler. Thanks!

1 0

Re: [Maria-developers] [Commits] bbc52df: MDEV-11490 Galera_3nodes test suite does not suppress Warnings.
by Nirbhay Choubey 14 Dec '16

14 Dec '16

Hi Sachin, I see that you have added a lot of warnings to the suite-level suppression list. Does *all* the new warnings show up for all the tests? I'd only add ones that make sense for the entire suite, and add the individuals to the specific test by calling mtr.add_suppression("<pattern>"). Ping me on slack if you need to discuss this further. Best, Nirbhay On Tue, Dec 6, 2016 at 1:53 AM, <sachin.setiya(a)mariadb.com> wrote: > revision-id: bbc52df39b7dedca50943a7cd99ee80b9e2f5b12 > (mariadb-10.1.19-24-gbbc52df) > parent(s): 611f91605adce17df87acf96b5aede0b73d0fc12 > author: SachinSetiya > committer: SachinSetiya > timestamp: 2016-12-06 12:21:16 +0530 > message: > > MDEV-11490 Galera_3nodes test suite does not suppress Warnings. > > Problem:- While running individual tests of Galera_3nodes , > We get warnings like '[Warning] WSREP: Could not open state file > for reading: '. And because of this individual tests fails. > > Solution:- We change suite.pm of Galera_3nodes to supress these warnings. > > --- > mysql-test/suite/galera_3nodes/suite.pm | 34 > ++++++++++++++++++++++++++++----- > 1 file changed, 29 insertions(+), 5 deletions(-) > > diff --git a/mysql-test/suite/galera_3nodes/suite.pm > b/mysql-test/suite/galera_3nodes/suite.pm > index 39d5acb..74f7dfa 100644 > --- a/mysql-test/suite/galera_3nodes/suite.pm > +++ b/mysql-test/suite/galera_3nodes/suite.pm > @@ -25,7 +25,8 @@ return "No my_print_defaults" unless $epath; > push @::global_suppressions, > ( > qr(WSREP: wsrep_sst_receive_address is set to '127.0.0.1), > - qr(WSREP: Could not open saved state file for reading: ), > + qr(WSREP: Could not open saved state file for reading: .*), > + qr(WSREP: Could not open state file for reading: .*), > qr(WSREP: Gap in state sequence. Need state transfer.), > qr(WSREP: Failed to prepare for incremental state transfer:), > qr(WSREP:.*down context.*), > @@ -33,16 +34,39 @@ push @::global_suppressions, > qr(WSREP: last inactive check more than .* skipping check), > qr(WSREP: SQL statement was ineffective), > qr(WSREP: Releasing seqno [0-9]* before [0-9]* was assigned.), > - qr|WSREP: access file$gvwstate.dat$ failed$No such file or > directory$|, > + qr|WSREP: access file$.*gvwstate.dat$ failed$No such file or > directory$|, > qr(WSREP: Quorum: No node with complete state), > qr(WSREP: Initial position was provided by configuration or SST, > avoiding override), > qr|WSREP: discarding established $time wait$ .*|, > qr(WSREP: There are no nodes in the same segment that will ever be > able to become donors, yet there is a suitable donor outside. Will use that > one.), > qr(WSREP: evs::proto.*), > - qr|WSREP: Ignoring possible split-brain (allowed by configuration) > from view:.*|, > + qr|WSREP: Ignoring possible split-brain $allowed by configuration$ > from view:.*|, > + qr(WSREP: no nodes coming from prim view, prim not possible), > + qr(WSREP: Member .* requested state transfer from .* but it is > impossible to select State Transfer donor: Resource temporarily > unavailable), > + qr(WSREP: user message in state LEAVING), > + qr(WSREP: .* sending install message failed: Transport endpoint is > not connected), > + qr(WSREP: .* sending install message failed: Resource temporarily > unavailable), > + qr(WSREP: Maximum writeset size exceeded by .*), > + qr(WSREP: transaction size exceeded.*), > + qr(WSREP: RBR event .*), > + qr(WSREP: Ignoring error for TO isolated action: .*), > + qr(WSREP: transaction size limit .*), > + qr(WSREP: rbr write fail, .*), > + qr(WSREP: .*Backend not supported: foo.*), > + qr(WSREP: .*Failed to initialize backend using .*), > + qr(WSREP: .*Failed to open channel 'my_wsrep_cluster' at .*), > + qr(WSREP: gcs connect failed: Socket type not supported), > + qr(WSREP: failed to open gcomm backend connection: 110: failed to > reach primary view: 110 .*), > + qr(WSREP: .*Failed to open backend connection: -110 .*), > + qr(WSREP: .*Failed to open channel 'my_wsrep_cluster' at .*), > + qr(WSREP: gcs connect failed: Connection timed out), > + qr|WSREP: wsrep::connect$.*$ failed: 7|, > + qr(WSREP: SYNC message from member .* in non-primary configuration. > Ignored.), > qr(WSREP: Could not find peer:), > - qr(WSREP: Protocol violation. JOIN message sender .*), > - qr(WSREP: JOIN message from member [0-9]* in non-primary > configuration. Ignored.), > + qr(WSREP: TO isolation failed for: .*), > + qr|WSREP: gcs_caused returned .*|, > + qr|WSREP: Protocol violation. JOIN message sender .* is not in state > transfer $SYNCED$. Message ignored.|, > + qr(WSREP: Action message in non-primary configuration from member > [0-9]*), > ); > > > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

2 1

Re: [Maria-developers] [Commits] 9bd354e: MDEV-11060 sql/protocol.cc:532: void Protocol::end_statement(): Assertion `0' failed
by Sergey Petrunia 13 Dec '16

13 Dec '16

Hi Varun, Please find a small note below. Ok to push after it has been addressed. On Tue, Dec 13, 2016 at 05:09:58AM +0530, Varun wrote: > revision-id: 9bd354ef91ae81e0f997f29acdae4f8a2997b3c8 (mariadb-10.1.19-40-g9bd354e) > parent(s): 67b570af506a42f14a8ae30603d52ca9de68cce5 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2016-12-13 05:07:02 +0530 > message: > > MDEV-11060 sql/protocol.cc:532: void Protocol::end_statement(): Assertion `0' failed > > In file sql/opt_range.cc,when calculate_cond_selectivity_for_table() is called with optimizer_use_condition_selectivity=4 then > - thd->no_errors is set to 1 > - the original value of thd->no_error is not restored to its original value > - this is causing the assertion to fail in the subsequent queries > > Fixed by restoring the original value of thd->no_errors > > --- > mysql-test/r/selectivity_innodb.result | 23 +++++++++++++++++++++++ > mysql-test/t/selectivity_innodb.test | 28 ++++++++++++++++++++++++++++ > sql/opt_range.cc | 1 + > 3 files changed, 52 insertions(+) > > diff --git a/mysql-test/r/selectivity_innodb.result b/mysql-test/r/selectivity_innodb.result > index 4355812..b57873d 100644 > --- a/mysql-test/r/selectivity_innodb.result > +++ b/mysql-test/r/selectivity_innodb.result > @@ -1670,6 +1670,29 @@ DROP TABLE t1,t2; > # > # End of 10.0 tests > # > +# > +# MDEV-11060: sql/protocol.cc:532: void Protocol::end_statement(): Assertion `0' failed > +# > +set optimizer_use_condition_selectivity=4; > +drop table if exists t1, t2; > +Warnings: > +Note 1051 Unknown table 'test.t1' > +Note 1051 Unknown table 'test.t2' > +drop view if exists v1; > +Warnings: > +Note 1051 Unknown table 'test.v1' Please remove 'DROP TABLE IF EXISTS' statements as they are not necessary. The previous test ends with DROP TABLE commands for tables t1, t2. There is no DROP VIEW command, so it is useful that the VIEW is dropped before it is used (just in case the test is run against a server that already has the view). Check out other test files, most of them start with lines like: --disable_warnings drop table if exists t1,t2; --enable_warnings Please also suppress warnings for the DROP VIEW statement. > +create table t1 (a int not null, b int, c int) engine=InnoDB; > +create trigger trgi before insert on t1 for each row set new.a=if(new.a is null,new.b,new.c); > +create table t2 (d int, e int) engine=InnoDB; > +update t1, t2 set a=NULL, b=2, c=NULL where b=d and e=200; > +create view v1 as select * from t1, t2 where d=2; > +insert v1 (a,c) values (NULL, 20); > +ERROR 23000: Column 'a' cannot be null > +drop table t1,t2; > +drop view v1; > +# > +# End of 10.1 tests > +# > set use_stat_tables= @tmp_ust; > set optimizer_use_condition_selectivity= @tmp_oucs; > SET SESSION STORAGE_ENGINE=DEFAULT; > diff --git a/mysql-test/t/selectivity_innodb.test b/mysql-test/t/selectivity_innodb.test > index 25aa0ab..bb87106 100644 > --- a/mysql-test/t/selectivity_innodb.test > +++ b/mysql-test/t/selectivity_innodb.test > @@ -138,6 +138,34 @@ DROP TABLE t1,t2; > --echo # End of 10.0 tests > --echo # > > + > +--echo # > +--echo # MDEV-11060: sql/protocol.cc:532: void Protocol::end_statement(): Assertion `0' failed > +--echo # > + > + > +set optimizer_use_condition_selectivity=4; > + > +drop table if exists t1, t2; > +drop view if exists v1; > + > +create table t1 (a int not null, b int, c int) engine=InnoDB; > +create trigger trgi before insert on t1 for each row set new.a=if(new.a is null,new.b,new.c); > + > +create table t2 (d int, e int) engine=InnoDB; > +update t1, t2 set a=NULL, b=2, c=NULL where b=d and e=200; > + > +create view v1 as select * from t1, t2 where d=2; > +--error ER_BAD_NULL_ERROR > +insert v1 (a,c) values (NULL, 20); > + > +drop table t1,t2; > +drop view v1; > + > +--echo # > +--echo # End of 10.1 tests > +--echo # > + > set use_stat_tables= @tmp_ust; > set optimizer_use_condition_selectivity= @tmp_oucs; > SET SESSION STORAGE_ENGINE=DEFAULT; > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > index bed57f3..0577ef0 100644 > --- a/sql/opt_range.cc > +++ b/sql/opt_range.cc > @@ -3112,6 +3112,7 @@ bool calculate_cond_selectivity_for_table(THD *thd, TABLE *table, Item **cond) > } > > free_alloc: > + thd->no_errors= 0; > thd->mem_root= param.old_root; > free_root(&alloc, MYF(0)); > > _______________________________________________ > 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] [Commits] 27af988: MDEV-11479 Improved wsrep_dirty_reads
by Vicențiu Ciorbaru 13 Dec '16

13 Dec '16

Hi Sachin! Comments inline: On Sun, 4 Dec 2016 at 08:36 <sachin.setiya(a)mariadb.com> 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;

2 1

Re: [Maria-developers] [Commits] 514b557: MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir:
by Sergei Golubchik 12 Dec '16

12 Dec '16

Hi, Vicentiu! I think mariadb should support 5.7 user table (without the password column) without any errors. That shouldn't be too difficult, just detect at run-time whether the column "Password" exists and adjust all other column numbers accordingly. On Dec 12, Vicentiu wrote: > revision-id: 514b557d066fcd1a90ccbbcdbaf5fa5fe65311eb (mariadb-10.2.2-138-g514b557) > parent(s): 9320d8ae30c18420bef659618175836221d363ea > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2016-12-12 04:26:27 +0200 > message: > > MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir: > > Fatal error: mysql.user table is damaged or in unsupported 3.20 format > > The problem stems from MySQL 5.7.6. According to MySQL documentation: > In MySQL 5.7.6, the Password column was removed and all credentials are > stored in the authentication_string column. > > If opening a MySQL 5.7.6 (and up) datadir with MariaDB 10.2, the user table > appears corrupted. In order to fix this, the server must be started with > --skip-grant-tables and then a subsequent mysql_upgrade command must be > issued. > > This patch updates the mysql_upgrade command to also add the removed > Password column. The password column is necessary, otherwise > the mysql_upgrade script fails due to the Event_scheduler not being able > to start, as it can't find Event_priv in the table where it ought to be. > MySQL's version has column position 28 (0 index) vs our datadir version > expects position 29. > > --- > scripts/mysql_system_tables_fix.sql | 6 ++++++ > 1 file changed, 6 insertions(+) > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 02e74a5: MDEV-11700: MariaDB 10.2 cannot start on MySQL 5.7 datadir:
by Vicențiu Ciorbaru 12 Dec '16

12 Dec '16

Hi Sergei! Can you please review this patch for MDEV-11700? Details presented in the commit message. I'm not sure which solution we want. Possible solutions: A) have the user fix it by first starting the server with --skip-grant-tables to avoid "mysql.user table is damaged", then run mysql_upgrade, to fix the problem. Without this patch, this solution fails at mysql_upgrade. B) Have the server understand MySQL 5.7.6 + table format where the Password column is missing. For 10.2 I've chosen solution A. Although this should probably also be targeted for 10.1? For 10.3, since we're planning on doing some authentication changes, we can implement a variant of B. Thoughts? On Mon, 12 Dec 2016 at 03:50 vicentiu <vicentiu(a)mariadb.org> wrote: > revision-id: 02e74a5614b08cfce505002abc26d4634648308e > (mariadb-10.2.2-138-g02e74a5) > parent(s): 9320d8ae30c18420bef659618175836221d363ea > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2016-12-12 03:42:11 +0200 > message: > > MDEV-11700: MariaDB 10.2 cannot start on MySQL 5.7 datadir: > > Fatal error: mysql.user table is damaged or in unsupported 3.20 format > > The problem stems from MySQL 5.7.6. According to MySQL documentation: > In MySQL 5.7.6, the Password column was removed and all credentials are > stored in the authentication_string column. > > If opening a MySQL 5.7.6 (and up) datadir with MariaDB 10.2, the user table > appears corrupted. In order to fix this, the server must be started with > --skip-grant-tables and then a subsequent mysql_upgrade command must be > issued. > > This patch updates the mysql_upgrade command to also add the removed > Password column. The password column is necessary, otherwise > the mysql_upgrade script fails due to the Event_scheduler not being able > to start, as it can't find Event_priv in the table where it ought to be. > MySQL's version has column position 28 (0 index) vs our datadir version > expects position 29. > > --- > scripts/mysql_system_tables_fix.sql | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/scripts/mysql_system_tables_fix.sql > b/scripts/mysql_system_tables_fix.sql > index ea1059c..80d0ef7 100644 > --- a/scripts/mysql_system_tables_fix.sql > +++ b/scripts/mysql_system_tables_fix.sql > @@ -164,6 +164,12 @@ ALTER TABLE user > MODIFY Host char(60) NOT NULL default '', > MODIFY User char(80) NOT NULL default '', > ENGINE=MyISAM, CONVERT TO CHARACTER SET utf8 COLLATE utf8_bin; > + > +# In MySQL 5.7.6 the Password column is removed. Recreate it to preserve > the number > +# of columns MariaDB expects in the user table. > +ALTER TABLE user > + ADD Password char(41) character set latin1 collate latin1_bin NOT NULL > default '' AFTER User; > + > ALTER TABLE user > MODIFY Password char(41) character set latin1 collate latin1_bin NOT > NULL default '', > MODIFY Select_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' > NOT NULL, >

2 2

[Maria-developers] Review for version#2 of MDEV-11297: Add support for LIMIT clause in GROUP_CONCAT
by Sergey Petrunia 11 Dec '16

11 Dec '16

Hi Varun, Nice to see that nearly all comments from the previous review were addressed. Please find my further input below. General questions: How is DISTINCT handled? Syntax like GROUP_CONCAT(DISTINCT col ORDER BY ... LIMIT ...) is allowed by the parser, however I don't see any handling for DISTINCT in your patch. Did you forget to update group_concat_limit.result file? Please check. > diff -urpN '--exclude=.*' 10.2-varun-mdev11297-orig/sql/item_sum.cc 10.2-varun-mdev11297-noc/sql/item_sum.cc > --- 10.2-varun-mdev11297-orig/sql/item_sum.cc 2016-12-11 12:17:04.023961449 +0300 > +++ 10.2-varun-mdev11297-noc/sql/item_sum.cc 2016-12-11 12:18:40.411957365 +0300 > @@ -3114,6 +3114,11 @@ int dump_leaf_key(void* key_arg, element > String *result= &item->result; > Item **arg= item->args, **arg_end= item->args + item->arg_count_field; > uint old_length= result->length(); > + ulonglong *offset_limit= &item->copy_offset_limit; > + ulonglong *row_limit = &item->copy_row_limit; > + > + if (item->limit_clause && !(*row_limit)) > + return 1; > > if (item->no_appended) > item->no_appended= FALSE; > @@ -3122,20 +3127,23 @@ int dump_leaf_key(void* key_arg, element > > tmp.length(0); > > - for (; arg < arg_end; arg++) > + if (!item->limit_clause || !(*offset_limit)) > { > - String *res; > - /* > - We have to use get_tmp_table_field() instead of > - real_item()->get_tmp_table_field() because we want the field in > - the temporary table, not the original field > - We also can't use table->field array to access the fields > - because it contains both order and arg list fields. > - */ > - if ((*arg)->const_item()) > - res= (*arg)->val_str(&tmp); > - else > + > + for (; arg < arg_end; arg++) > { > + String *res; > + /* > + We have to use get_tmp_table_field() instead of > + real_item()->get_tmp_table_field() because we want the field in > + the temporary table, not the original field > + We also can't use table->field array to access the fields > + because it contains both order and arg list fields. > + */ > + if ((*arg)->const_item()) > + res= (*arg)->val_str(&tmp); > + else > + { > Field *field= (*arg)->get_tmp_table_field(); Please make the code to use the right indentation. > if (field) > { > @@ -3146,9 +3154,19 @@ int dump_leaf_key(void* key_arg, element > } > else > res= (*arg)->val_str(&tmp); > + } > + if (res) > + { > + result->append(*res); > + } > } > - if (res) > - result->append(*res); > + if ((*row_limit)) > + (*row_limit)--; > + } > + else > + { > + item->no_appended= TRUE; If I comment out the above line, the test still passes. Please add test coverage that shows it is needed, or remove it. > + (*offset_limit)--; > } > > item->row_count++; > @@ -3199,7 +3217,8 @@ Item_func_group_concat:: > Item_func_group_concat(THD *thd, Name_resolution_context *context_arg, > bool distinct_arg, List<Item> *select_list, > const SQL_I_List<ORDER> &order_list, > - String *separator_arg) > + String *separator_arg, bool limit_clause, > + Item *rowLimit, Item *offsetLimit) camelCase or javaStyleVariableNames are against the coding style. Please use something like row_limit_arg if there is no better option. (In general, please try to make your new code fit in with the rest of the code. Did you see camelCase anywhere? If not, it's a sign not to use it). > :Item_sum(thd), tmp_table_param(0), separator(separator_arg), tree(0), > unique_filter(NULL), table(0), > order(0), context(context_arg), > @@ -3208,7 +3227,9 @@ Item_func_group_concat(THD *thd, Name_re > row_count(0), > distinct(distinct_arg), > warning_for_row(FALSE), > - force_copy_fields(0), original(0) > + force_copy_fields(0),row_limit(0), > + offset_limit(0),limit_clause(limit_clause), > + copy_offset_limit(0), copy_row_limit(0),original(0) Please use 'NULL' for pointers, not '0'. > { > Item *item_select; > Item **arg_ptr; > @@ -3250,6 +3271,16 @@ Item_func_group_concat(THD *thd, Name_re > /* orig_args is only used for print() */ > orig_args= (Item**) (order + arg_count_order); > memcpy(orig_args, args, sizeof(Item*) * arg_count); > + > + if (limit_clause) > + { > + if (!(row_limit=(Item*) thd->alloc(sizeof(Item)))) > + return; I'm not sure what the above is supposed to mean? Why allocate sizeof(Item) bytes? > + row_limit= rowLimit; ... especially when the next thing you do is the above assignment, so you lose the value that you've got from the thd->alloc() call? > + if (!(offset_limit= (Item*)thd->alloc(sizeof(Item)))) > + return; > + offset_limit= offsetLimit; The same question as above. > + } > } > > > @@ -3269,7 +3300,9 @@ Item_func_group_concat::Item_func_group_ > warning_for_row(item->warning_for_row), > always_null(item->always_null), > force_copy_fields(item->force_copy_fields), > - original(item) > + row_limit(item->row_limit), offset_limit(item->offset_limit), > + limit_clause(item->limit_clause),copy_offset_limit(item->copy_offset_limit), > + copy_row_limit(item->copy_row_limit), original(item->original) > { > quick_group= item->quick_group; > result.set_charset(collation.collation); > @@ -3382,6 +3415,10 @@ void Item_func_group_concat::clear() > null_value= TRUE; > warning_for_row= FALSE; > no_appended= TRUE; > + if (offset_limit) > + copy_offset_limit= offset_limit->val_int(); > + if (row_limit) > + copy_row_limit= row_limit->val_int(); > if (tree) > reset_tree(tree); > if (unique_filter) > diff -urpN '--exclude=.*' 10.2-varun-mdev11297-orig/sql/item_sum.h 10.2-varun-mdev11297-noc/sql/item_sum.h > --- 10.2-varun-mdev11297-orig/sql/item_sum.h 2016-12-11 12:17:04.043961448 +0300 > +++ 10.2-varun-mdev11297-noc/sql/item_sum.h 2016-12-11 12:18:40.415957365 +0300 > @@ -1583,6 +1583,12 @@ class Item_func_group_concat : public It > bool always_null; > bool force_copy_fields; > bool no_appended; > + Item *row_limit; > + Item *offset_limit; > + bool limit_clause; > + ulonglong copy_offset_limit; > + ulonglong copy_row_limit; > + In previous review I have requested: >> Please add (short) comments describing each new member variable. And you still haven't done that. > /* > Following is 0 normal object and pointer to original one for copy > (to correctly free resources) > @@ -1602,7 +1608,8 @@ protected: > public: > Item_func_group_concat(THD *thd, Name_resolution_context *context_arg, > bool is_distinct, List<Item> *is_select, > - const SQL_I_List<ORDER> &is_order, String *is_separator); > + const SQL_I_List<ORDER> &is_order, String *is_separator, > + bool limit_clause, Item *row_limit, Item *offset_limit); > > Item_func_group_concat(THD *thd, Item_func_group_concat *item); > ~Item_func_group_concat(); > diff -urpN '--exclude=.*' 10.2-varun-mdev11297-orig/sql/sql_yacc.yy 10.2-varun-mdev11297-noc/sql/sql_yacc.yy > --- 10.2-varun-mdev11297-orig/sql/sql_yacc.yy 2016-12-11 12:17:04.107961446 +0300 > +++ 10.2-varun-mdev11297-noc/sql/sql_yacc.yy 2016-12-11 12:18:40.887957345 +0300 > @@ -1786,7 +1786,7 @@ bool my_yyoverflow(short **a, YYSTYPE ** > %type <num> > order_dir lock_option > udf_type opt_local opt_no_write_to_binlog > - opt_temporary all_or_any opt_distinct > + opt_temporary all_or_any opt_distinct opt_glimit_clause > opt_ignore_leaves fulltext_options union_option > opt_not > select_derived_init transaction_access_mode_types > @@ -1844,7 +1844,7 @@ bool my_yyoverflow(short **a, YYSTYPE ** > opt_escape > sp_opt_default > simple_ident_nospvar simple_ident_q > - field_or_var limit_option > + field_or_var limit_option glimit_option > part_func_expr > window_func_expr > window_func > @@ -10449,16 +10449,34 @@ sum_expr: > | GROUP_CONCAT_SYM '(' opt_distinct > { Select->in_sum_expr++; } > expr_list opt_gorder_clause > - opt_gconcat_separator > + opt_gconcat_separator opt_glimit_clause > ')' > { > SELECT_LEX *sel= Select; > sel->in_sum_expr--; > - $$= new (thd->mem_root) > - Item_func_group_concat(thd, Lex->current_context(), $3, $5, > - sel->gorder_list, $7); > - if ($$ == NULL) > - MYSQL_YYABORT; > + if (!$8) > + $$= new (thd->mem_root) > + Item_func_group_concat(thd, Lex->current_context(), $3, $5, > + sel->gorder_list, $7, FALSE, 0, 0); > + else > + { > + if (sel->select_limit && sel->offset_limit) > + $$= new (thd->mem_root) > + Item_func_group_concat(thd, Lex->current_context(), $3, $5, > + sel->gorder_list, $7, TRUE, sel->select_limit, > + sel->offset_limit); > + else if (sel->select_limit) > + $$= new (thd->mem_root) > + Item_func_group_concat(thd, Lex->current_context(), $3, $5, > + sel->gorder_list, $7, TRUE, sel->select_limit, 0); > + } > + if ($$ == NULL) > + MYSQL_YYABORT; > + > + sel->select_limit= NULL; > + sel->offset_limit= NULL; > + sel->explicit_limit= 0; > + > $5->empty(); > sel->gorder_list.empty(); > } > @@ -10680,6 +10698,95 @@ gorder_list: > { if (add_gorder_to_list(thd, $1,(bool) $2)) MYSQL_YYABORT; } > ; What is the difference between glimit_option and limit_option? Looking at the grammar, they look the same. Please either remove glimit_option or explain why do you need it. I can see that limit_clause and glimit_clause have to be different because limit_clause also supports LIMIT ROWS EXAMINED. Actually, there is one thing in limit_clause that I think should be in glimit_clause also. It's this call: Lex->set_stmt_unsafe(LEX::BINLOG_STMT_UNSAFE_LIMIT); Did you omit it intentionally? Let's meet and discuss it. > +opt_glimit_clause: > + /* empty */ { $$ = 0; } > + | glimit_clause { $$ = 1; } > + ; > + > +glimit_clause_init: > + LIMIT{} > + ; > + > +glimit_clause: > + glimit_clause_init glimit_options{} > + ; > + > +glimit_options: > + glimit_option > + { > + SELECT_LEX *sel= Select; > + sel->select_limit= $1; > + sel->offset_limit= 0; > + sel->explicit_limit= 1; > + } > + | glimit_option ',' glimit_option > + { > + SELECT_LEX *sel= Select; > + sel->select_limit= $3; > + sel->offset_limit= $1; > + sel->explicit_limit= 1; > + } > + | glimit_option OFFSET_SYM glimit_option > + { > + SELECT_LEX *sel= Select; > + sel->select_limit= $1; > + sel->offset_limit= $3; > + sel->explicit_limit= 1; > + } > + ; > + > +glimit_option: > + ident > + { > + Item_splocal *splocal; > + LEX *lex= thd->lex; > + Lex_input_stream *lip= & thd->m_parser_state->m_lip; > + sp_variable *spv; > + sp_pcontext *spc = lex->spcont; > + if (spc && (spv = spc->find_variable($1, false))) > + { > + splocal= new (thd->mem_root) > + Item_splocal(thd, $1, spv->offset, spv->sql_type(), > + lip->get_tok_start() - lex->sphead->m_tmp_query, > + lip->get_ptr() - lip->get_tok_start()); > + if (splocal == NULL) > + MYSQL_YYABORT; > +#ifndef DBUG_OFF > + splocal->m_sp= lex->sphead; > +#endif > + lex->safe_to_cache_query=0; > + } > + else > + my_yyabort_error((ER_SP_UNDECLARED_VAR, MYF(0), $1.str)); > + if (splocal->type() != Item::INT_ITEM) > + my_yyabort_error((ER_WRONG_SPVAR_TYPE_IN_LIMIT, MYF(0))); > + splocal->limit_clause_param= TRUE; > + $$= splocal; > + } > + | param_marker > + { > + $1->limit_clause_param= TRUE; > + } > + | ULONGLONG_NUM > + { > + $$= new (thd->mem_root) Item_uint(thd, $1.str, $1.length); > + if ($$ == NULL) > + MYSQL_YYABORT; > + } > + | LONG_NUM > + { > + $$= new (thd->mem_root) Item_uint(thd, $1.str, $1.length); > + if ($$ == NULL) > + MYSQL_YYABORT; > + } > + | NUM > + { > + $$= new (thd->mem_root) Item_uint(thd, $1.str, $1.length); > + if ($$ == NULL) > + MYSQL_YYABORT; > + } > + ; > + > in_sum_expr: > opt_all > { BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] Please review MDEV-11528 Split Item_func_min_max::val_xxx() and Item_func_min_max::get_date() into methods in Type_handler
by Alexander Barkov 10 Dec '16

10 Dec '16

Hello Alexey, Can you please review a patch for MDEV-11528. Thanks!

1 0

[Maria-developers] MDEV-11503 Introduce Type_handler::make_in_vector() and Item_func_in_fix_comparator_compatible_types()
by Alexander Barkov 09 Dec '16

09 Dec '16

Hello Sanja, Please review a patch for MDEV-11503. Thanks!

1 1

[Maria-developers] Fix for TokuDB and parallel replication
by Kristian Nielsen 09 Dec '16

09 Dec '16

Parallel replication so far did not work well with TokuDB, as some people who tried it found out. I have now pushed to 10.1 some patches to solve the problems. There are two main fixes: 1. Fix some races where a waiting transaction would miss its wakeup and get a lock timeout on a waiting row lock, even though the lock was released by the holding transaction. This fix is due to great work by Rich Prohaska. This problem is actually not specific to replication, normal transactions on a master will experience it too. But it hurts replication a lot, since replication must commit transactions in order, so one stalled transaction stalls all following transactions as well. 2. Implement the conflict detection and handling necessary for optimistic parallel replication to work. This basically implements thd_rpl_deadlock_check() and hton->kill_query methods. This should solve the problems where optimistic parallel replication with TokuDB breaks with lock wait timeouts. If someone wants to test it, I have made a tree available with just these fixes on top of MariaDB 10.1.19: https://github.com/knielsen/server/tree/tokudb_optimistic_parallel_replicat… The fix should appear in 10.1.20 eventually. The first part of the patch has also been submitted by Rich to upstream. When this is (hopefully) merged upstream, and upstream merged into MariaDB, the MariaDB version of the fix should be replaced with the Percona one. I tried making the MariaDB version of the fix identical to Rich's pull request and keeping it in a separate commit, so this should hopefully be simple to do when the time comes. - Kristian.

3 9

Re: [Maria-developers] [Commits] 00f5065: MDEV-11422 rpl.rpl_parallel_optimistic_nobinlog failed
by Jan Lindström 08 Dec '16

08 Dec '16

Hi, All other looks correct, I'm concerned only following change that is not needed to fix this bug: On Thu, Dec 8, 2016 at 2:35 PM, <marko.makela(a)mariadb.com> wrote: > > > - err = lock_trx_handle_wait(trx, > - (lock_mutex_taken || already_have_lock_mutex), > - (trx_mutex_taken || already_have_trx_mutex)); > +#ifdef UNIV_DEBUG > + dberr_t err = > +#endif > + lock_trx_handle_wait(trx, true, true); > + > + ut_ad(err == DB_SUCCESS || err == DB_LOCK_WAIT > + || err == DB_DEADLOCK); > > > In above is true, true always correct? we conditionally take lock and trx mutexes on some cases if we already do not have them, and below function will take them if they are not already taken. There is complex rules as Galera can call this from different situations and we already have had cases where some mutex was taken twice or no mutex was taken. R: Jan

1 0

[Maria-developers] MDEV-10787 Assertion `ltime->neg == 0' failed in void date_to_datetime(MYSQL_TIME*)
by Alexander Barkov 07 Dec '16

07 Dec '16

Hello Sergei, please review a patch for MDEV-10787. Thanks!

2 2

[Maria-developers] Temporary push of multi_range_read support to 10.2
by Michael Widenius 07 Dec '16

07 Dec '16

Hi! Kentoku, I have now pushed into 10.2 the code for spider patches: 014,015,023,033,035,037,040,042,044,045,049,050,051,053,059 The push is done to make it easier for Jacob and you to make test cases for the new code. I will work on reviewing and cleaning up this patch during the next few days and then do a push a final version when I have got some more test cases that covers most of the new code. Regards, Monty

1 0

[Maria-developers] OSS-Fuzz
by Daniel Black 07 Dec '16

07 Dec '16

In case anyone is interested. Code needs to be written to take a random bit of data and push this into the server somehow. Apart from that it appears reasonably automated. https://github.com/google/oss-fuzz

1 0

Re: [Maria-developers] [Commits] 895309f: MDEV-10663: Use of Inline table columns in HAVING clause throws 1463 Error
by Sergei Golubchik 06 Dec '16

06 Dec '16

Hi, Oleksandr! On Dec 06, Oleksandr Byelkin wrote: > revision-id: 895309f441e0262229671dcec5d49c4ff8f8ca2c (mariadb-5.5.53-16-g895309f) > parent(s): 18cdff6765b954431934f6e6d0b1e281c8f0e0b8 > committer: Oleksandr Byelkin > timestamp: 2016-12-06 14:18:24 +0100 > message: > > MDEV-10663: Use of Inline table columns in HAVING clause throws 1463 Error > > check for VIEW/DERIVED fields ok to push! Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] f79d9d4: [MDEV]-11162 Assertion failing because cleanup does not get called
by Nirbhay Choubey 06 Dec '16

06 Dec '16

Hi Varun, On Tue, Dec 6, 2016 at 9:34 AM, Varun <varunraiko1803(a)gmail.com> wrote: > revision-id: f79d9d454bc3fa6fecb34fec850752a688c0ccc7 > (mariadb-10.0.28-17-gf79d9d4) > parent(s): e99990c631905cd923257331ef124a2bc9276e0d > author: Varun > committer: Varun > timestamp: 2016-12-06 19:48:58 +0530 > message: > > [MDEV]-11162 Assertion failing because cleanup does not get called > Looks like you forgot to add the result file. Besides, since the test is simple, you could instead add it to an existing _debug_ select/optimizer test. Best, Nirbhay > --- > mysql-test/t/bug-11162.test | 6 ++++++ > sql/sql_select.cc | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/mysql-test/t/bug-11162.test b/mysql-test/t/bug-11162.test > new file mode 100644 > index 0000000..5924851 > --- /dev/null > +++ b/mysql-test/t/bug-11162.test > @@ -0,0 +1,6 @@ > +--source include/have_innodb.inc > + > +CREATE TABLE t1 (i INT) ENGINE=InnoDB; > +SELECT ( SELECT DISTINCT GROUP_CONCAT(SLEEP(0)) FROM t1 GROUP BY i ); > +SELECT i FROM t1 order by i LIMIT 1; > +drop table t1; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 2db9a2b..f345d3c 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -3144,6 +3144,7 @@ JOIN::destroy() > */ > tmp_table_param.cleanup(); > tmp_join->tmp_table_param.copy_field= 0; > + cleanup(1); > DBUG_RETURN(tmp_join->destroy()); > } > cond_equal= 0; > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

Re: [Maria-developers] [Commits] 78c093a: MDEV-10435 crash with bad stat tables.
by Sergei Golubchik 06 Dec '16

06 Dec '16

Hi, Alexey! On Dec 06, Alexey Botchkov wrote: > revision-id: 78c093a042ccee6c7b8d11ab1b7e6069efb4af24 (mariadb-10.1.19-24-g78c093a) > parent(s): 611f91605adce17df87acf96b5aede0b73d0fc12 > committer: Alexey Botchkov > timestamp: 2016-12-06 18:32:48 +0400 > message: > > MDEV-10435 crash with bad stat tables. > > Functions from sql/statistics.cc don't seem to expect > stat tables to fail or to have inadequate structure. > Table open errors suppressed and some validity > checks added. > > diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc > index 27bc0fb..70d7d26 100644 > --- a/sql/sql_statistics.cc > +++ b/sql/sql_statistics.cc > @@ -129,6 +129,137 @@ inline void init_table_list_for_single_stat_table(TABLE_LIST *tbl, > } > > +class Stat_table_intact : public Table_check_intact grrr, I never understood why every user of Table_check_intact needs to create a new class. That's a lot of copy-pasting :( > +{ > +public: > + Stat_table_intact() {} > +protected: > + void report_error(uint code, const char *fmt, ...) {} You have an empty report_error() method here. How do report errors about incorrect table definition? May be you should copy (ugh!) report_error from, say, Event_db_intact? > +}; Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MDEV-11485 Split Item_func_between::val_int() into virtual methods in Type_handler
by Alexander Barkov 05 Dec '16

05 Dec '16

Hello Sanja, Please review a patch for MDEV-11485. Thanks!

1 0

[Maria-developers] Please review MDEV-10717 Assertion `!null_value' failed in virtual bool Item::send(Protocol*, String*)
by Alexander Barkov 05 Dec '16

05 Dec '16

Hello Sergei, Please review a patch for MDEV-10717. Thanks!

2 1

Re: [Maria-developers] Working on spider patches, MDEV-7698
by Michael Widenius 05 Dec '16

05 Dec '16

Hi! Now working on MDEV-7700 Spiral patch 002_mariadb-10.0.15.spider.diff +++ ./002_mariadb-10.1.8-spider/sql/ha_partition.cc 2015-10-14 01:48:53.392665313 +0900 @@ -327,7 +327,9 @@ void ha_partition::init_handler_variable m_file_buffer= NULL; m_name_buffer_ptr= NULL; m_engine_array= NULL; +/* m_connect_string= NULL; +*/ m_file= NULL; m_file_tot_parts= 0; m_reorged_file= NULL; @@ -1516,4 +1518,6 @@ int ha_partition::prepare_new_partition( if ((error= set_up_table_before_create(tbl, part_name, create_info, p_elem))) goto error_create; +/* tbl->s->connect_string = p_elem->connect_string; +*/ I don't think this it will work removing the usage of p_elem->connect_string This is because each partition may have a different connect string. Here is an example from fedarated_partion.test: eval create table t1 (s1 int primary key) engine=federated partition by list (s1) (partition p1 values in (1,3) connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_1', partition p2 values in (2,4) connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_2'); The above works in mariadb 10.2 but not in your spider tree. >From this patch I will, for now, only take the code related to HA_EXTRA_WRITE_CAN_REPLACE HA_EXTRA_WRITE_CANNOT_REPLACE --- ./001_mariadb-10.1.8-partition_cond_push/sql/sql_plugin.cc 2015-10-13 23:49:10.188129839 +0900 +++ ./002_mariadb-10.1.8-spider/sql/sql_plugin.cc 2015-10-14 01:48:54.296665317 +0900 @@ -2757,6 +2757,7 @@ static void update_func_str(THD *thd, st *(char**) tgt= my_strdup(value, MYF(0)); else *(char**) tgt= 0; - my_free(old); + if (old) + my_free(old); As my_free is safe to call with NULL, the above is not needed diff -Narup ./001_mariadb-10.1.8-partition_cond_push/sql/sql_priv.h ./002_mariadb-10.1.8-spider/sql/sql_priv.h --- ./001_mariadb-10.1.8-partition_cond_push/sql/sql_priv.h 2015-10-13 23:49:10.189129839 +0900 +++ ./002_mariadb-10.1.8-spider/sql/sql_priv.h 2015-10-14 01:48:54.642665315 +0900 @@ -27,6 +27,8 @@ #ifndef SQL_PRIV_INCLUDED #define SQL_PRIV_INCLUDED +#define PLUGIN_VAR_CAN_MEMALLOC + #ifndef MYSQL_CLIENT The above is not needed, as all code that is testing this is doing: #if defined(MARIADB_BASE_VERSION) && MYSQL_VERSION_ID >= 100000 Which is always true in MariaDB 10.x Regards, Monty

2 7

[Maria-developers] Working on spider patches, MDEV-7701
by Michael Widenius 05 Dec '16

05 Dec '16

Hi! Next patch, MDEV 7701 +++ ./003_mariadb-10.2.0-vp/sql/ha_partition.cc 2016-05-05 21:25:04.289731712 +0900 @@ -7238,13 +7239,30 @@ int ha_partition::extra(enum ha_extra_fu } /* Category 9) Operations only used by MERGE */ case HA_EXTRA_ADD_CHILDREN_LIST: + DBUG_RETURN(loop_extra(operation)); case HA_EXTRA_ATTACH_CHILDREN: + int result; + handler **file; + if ((result = loop_extra(operation))) + DBUG_RETURN(result); + m_num_locks = 0; + additional_table_flags = + (HA_CAN_BG_SEARCH | HA_CAN_BG_INSERT | HA_CAN_BG_UPDATE); + file = m_file; + do + { + m_num_locks += (*file)->lock_count(); + additional_table_flags &= ~((ulonglong) ((*file)->ha_table_flags() ^ + (HA_CAN_BG_SEARCH | HA_CAN_BG_INSERT | HA_CAN_BG_UPDATE))); The above test could be simple: additional_table_flags &= (ulonglong) ((*file)->ha_table_flags(); Should be the same thing. I did however remove this code, as HA_CAN_BG... are not used anywhere in the current spider code. See comment below. I also changed so that m_num_locks always contains the upper limit number of locks needed. Without this change, the call to lock_count() would have returneed m_total_parts * 'new-counted-locks' which would be been way too much. @@ -9123,6 +9141,19 @@ const COND *ha_partition::cond_push(cons handler **file= m_file; COND *res_cond = NULL; DBUG_ENTER("ha_partition::cond_push"); +#ifdef HANDLER_HAS_TOP_TABLE_FIELDS + if (set_top_table_fields) + { + do + { + if ((*file)->set_top_table_and_fields(top_table, + top_table_field, + top_table_fields)) + DBUG_RETURN(cond); + } while (*(++file)); + file= m_file; + } +#endif I added this without the define, but I merged it to the loop for cond_push. Instead of having defines in the sql code, I will fix that in spider and vp that I will set the proper defines based on the MariaDB version. However, I would like to have a clear comment the purpose of the function set_top_table_and_fields(). struct st_mysql_storage_engine partition_storage_engine= { MYSQL_HANDLERTON_INTERFACE_VERSION }; +++ ./003_mariadb-10.2.0-vp/sql/handler.h 2016-05-05 21:25:04.314731713 +0900 @@ -255,6 +257,11 @@ enum enum_alter_inplace_result { */ #define HA_BINLOG_FLAGS (HA_BINLOG_ROW_CAPABLE | HA_BINLOG_STMT_CAPABLE) +#define HA_CAN_BG_SEARCH (1LL << 45) +#define HA_CAN_BG_INSERT (1LL << 46) +#define HA_CAN_BG_UPDATE (1LL << 47) +#define HA_CAN_MULTISTEP_MERGE (1LL << 48) Do you really need HA_CAN_BG_XXX flags? I checked your spider tree and this is not used anywhere, so I think we should leave them out for now. If we don't need these, we can remove the loop for setting them in HA_EXTRA_ATTACH_CHILDREN too. If we need them, I would like to have a comment for why they are needed and when we will need then add these in a separate patch. @@ -3525,6 +3540,29 @@ public: Pops the top if condition stack, if stack is not empty. */ virtual void cond_pop() { return; }; + virtual int set_top_table_and_fields(TABLE *top_table, + Field **top_table_field, + uint top_table_fields) + { + if (!set_top_table_fields) + { + set_top_table_fields = TRUE; + this->top_table = top_table; + this->top_table_field = top_table_field; + this->top_table_fields = top_table_fields; + } + return 0; + } Why is this int and not void ? Do you have in mind that this may be possible to fail in the future? If it's never going to change, then better to do this void. I will make it void for now and change the relevant code. We can make it int again if needed in the future. diff -Narup ./002_mariadb-10.2.0-spider/sql/sql_base.cc ./003_mariadb-10.2.0-vp/sql/sql_base.cc --- ./002_mariadb-10.2.0-spider/sql/sql_base.cc 2016-04-17 02:47:27.000000000 +0900 +++ ./003_mariadb-10.2.0-vp/sql/sql_base.cc 2016-05-05 21:25:04.319731713 +0900 @@ -1475,6 +1475,10 @@ unique_table(THD *thd, TABLE_LIST *table table= table->find_table_for_update(); - if (table->table && table->table->file->ht->db_type == DB_TYPE_MRG_MYISAM) - { - TABLE_LIST *child; + if (table->table && + ( + table->table->file->ht->db_type == DB_TYPE_MRG_MYISAM || + (table->table->file->ha_table_flags() & HA_CAN_MULTISTEP_MERGE) + ) + ) + continue I added HA_CAN_MULTISTEP_MERGE as a flag to DB_TYPE_MRG_MYISAM, which allwoed me to make the above tests and the following identical ones much simpler. This is now pushed into 10.2-spider Regards, Monty

2 2

Re: [Maria-developers] MDEV-11330 Split Item_func_hybrid_field_type::val_xxx() into methods in Type_handler
by Vicențiu Ciorbaru 05 Dec '16

05 Dec '16

Hi Alexander! I find this version much cleaner! Looks good to me. What I'd really like to see later is if we can write some unit tests for these conversions. Not mtr linked tests but actual unit tests specifically using these Items. I am up for chipping in and writing some myself. One thing I'd like to discuss later in the context of reentrant functions are these side effects such as "return (null_value= true);" when fetching data from an Item. I think they should go from base items and only perhaps happen in "cache" items. We can potentially then turn all these methods to be const methods. Vicențiu On Thu, 1 Dec 2016 at 15:18 Alexander Barkov <abarkov73(a)gmail.com> wrote: > Hello Vicențiu, > > Please review a modified version. > > As agreed, I'm not using the cast approach for the built-in types > and define a set of data type conversion methods > directly Type_handler_hybrid_field_type methods instead. > > Thanks! >

1 0

[Maria-developers] MariaRocks port: why handler::init_with_fields() call is needed + call for comments
by Sergey Petrunia 02 Dec '16

02 Dec '16

Hello, I think me and Serg have tried having this discussion before on several occasions but were not successful due to me not having the details back then. Now I'm trying with the details. == Description of the issue == Look at MariaDB, the code in TABLE_SHARE::init_from_binary_frm_image (or open_binary_frm() in mysql-5.6 codebase) The code goes like this: ... /* Allocate handler */ if (!(handler_file= get_new_handler(share, thd->mem_root, plugin_hton(se_plugin)))) // ^(LOCATION-1) ... /* Fix key->name and key_part->field */ if (key_parts) { longlong ha_option= handler_file->ha_table_flags(); // ^(LOCATION-2) ... for (uint key=0 ; key < keys ; key++,keyinfo++) { ... for (i=0; i < key_parts; key_part++, i++) { ... field= key_part->field= share->field[key_part->fieldnr-1]; // (LOCATION-3) ... } } Note that - a handler object is created at (LOCATION-1). A TABLE_SHARE object exists at that point, but it has table_share->key_info==NULL. This means the storage object has no information about the indexes in the table it is representing. - At (LOCATION-2), the code retrieves ha_table_flags(). A bit below that point it checks for "ha_option & HA_PRIMARY_KEY_IN_READ_INDEX" (there are also other checks, both harmful and harmless, but let's focus on this one for now). In MyRocks, whether HA_PRIMARY_KEY_IN_READ_INDEX flag is set depends on the datatypes of the primary key columns. However, ha_rocksdb object is requested to provide ha_table_flags() before it has a chance of looking at the table DDL and actually knowing the value. == Solution in MyRocks == It's here: https://github.com/facebook/mysql-5.6/commit/c1f1f0b64eaecd132fc6ec21663faf… Basically, there is a new handler::init_with_fields() call which SQL layer calls after information about table fields is filled. == Questions == Any objections to this approach? Alternative solutions that do not require a lot of re-writing? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] cf04c06: MDEV-10435 crash with bad stat tables.
by Sergei Golubchik 01 Dec '16

01 Dec '16

Hi, Alexey! On Nov 02, Alexey Botchkov wrote: > revision-id: cf04c06a58e3d0e700491f8f9167e9323cf1de1d (mariadb-10.1.18-28-gcf04c06) > parent(s): c18054deb2b5cfcf1f13aa71574406f2bafb87c6 > committer: Alexey Botchkov > timestamp: 2016-11-02 13:02:32 +0400 > message: > > MDEV-10435 crash with bad stat tables. > > Functions from sql/statistics.cc don't seem to expect > stat tables to fail or to have inadequate structure. > Table open errors suppressed and some validity checks added. > > diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc > index 4020cbc..3f341ac 100644 > --- a/sql/sql_statistics.cc > +++ b/sql/sql_statistics.cc > @@ -129,6 +129,30 @@ inline void init_table_list_for_single_stat_table(TABLE_LIST *tbl, > } > > > +static > +inline int stat_tables_are_inadequate(TABLE_LIST *tables) > +{ > + TABLE_SHARE *cur_s; > + > + /* If the number of tables changes, we should revise this function. */ > + DBUG_ASSERT(STATISTICS_TABLES == 3); > + > + cur_s= tables[TABLE_STAT].table->s; > + if (cur_s->fields < TABLE_STAT_N_FIELDS || cur_s->keys == 0) > + return TRUE; > + > + cur_s= tables[COLUMN_STAT].table->s; > + if (cur_s->fields < COLUMN_STAT_N_FIELDS || cur_s->keys == 0) > + return TRUE; > + > + cur_s= tables[INDEX_STAT].table->s; > + if (cur_s->fields < INDEX_STAT_N_FIELDS || cur_s->keys == 0) > + return TRUE; > + > + return FALSE; > +} I'd suggest to use Table_check_intact interface instead. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] 10.2 tree, innodb_gis.alter_spatial_index test broken after your push
by Sergey Petrunia 01 Dec '16

01 Dec '16

Hi Jan, Your push https://github.com/MariaDB/server/commit/dc9f919f27fccfeb0de3ab392f33bc5efd… broke the innodb_gis.alter_spatial_index test in 10.2 tree. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] 2c9bb42: MDEV-11432 Change the informational redo log format tag to "MariaDB 10.2.3"
by Jan Lindström 01 Dec '16

01 Dec '16

Hi, ok to push. R: Jan On Thu, Dec 1, 2016 at 8:36 AM, <marko.makela(a)mariadb.com> wrote: > revision-id: 2c9bb42d901fc4f48f4884e4a85af74eae6d0929 > (mariadb-10.2.2-91-g2c9bb42) > parent(s): dc9f919f27fccfeb0de3ab392f33bc5efdfd59a0 > author: Marko Mäkelä > committer: Marko Mäkelä > timestamp: 2016-12-01 08:28:59 +0200 > message: > > MDEV-11432 Change the informational redo log format tag to "MariaDB 10.2.3" > > MariaDB 10.2 incorporates MySQL 5.7. MySQL 5.7.9 (the first GA release > of the series) introduced an informational field to the InnoDB redo log > header, which identifies the server version where the redo log files > were created (initialized, resized or updated), in > WL#8845: InnoDB: Redo log format version identifier. > > The informational message would be displayed to the user, for example > if someone tries to start up MySQL 8.0 after killing a MariaDB 10.2 server. > In the current MariaDB 10.2 source code, the identifier string would > misleadingly say "MySQL 5.7.14" (using the hard-coded version number in > univ.i) instead of "MariaDB 10.2.3" (using the contents of the VERSION > file, the build system copies to config.h and my_config.h). > > This is only a cosmetic change. The compatibility check is based on a > numeric identifier. > > We should probably also change the numeric identifier and some logic > around it. MariaDB 10.2 should refuse to recover from a crashed MySQL 5.7 > instance, because the redo log might contain references to shared > tablespaces, > which are not supported by MariaDB 10.2. Also, when MariaDB 10.2 creates > an encrypted redo log, there should be a redo log format version tag that > will prevent MySQL 5.7 or 8.0 from starting up. > > --- > storage/innobase/include/log0log.h | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/storage/innobase/include/log0log.h > b/storage/innobase/include/log0log.h > index c1f2570..caa067c 100644 > --- a/storage/innobase/include/log0log.h > +++ b/storage/innobase/include/log0log.h > @@ -555,7 +555,11 @@ or the MySQL version that created the redo log file. > */ > /** End of the log file creator field. */ > #define LOG_HEADER_CREATOR_END (LOG_HEADER_CREATOR + 32) > /** Contents of the LOG_HEADER_CREATOR field */ > -#define LOG_HEADER_CREATOR_CURRENT "MySQL " INNODB_VERSION_STR > +#define LOG_HEADER_CREATOR_CURRENT \ > + "MariaDB " \ > + IB_TO_STR(MYSQL_VERSION_MAJOR) "." \ > + IB_TO_STR(MYSQL_VERSION_MINOR) "." \ > + IB_TO_STR(MYSQL_VERSION_PATCH) > > /** The redo log format identifier corresponding to the current format > version. > Stored in LOG_HEADER_FORMAT. */ > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

Re: [Maria-developers] Please review MDEV-11298 Split Item_func_hex::val_str_ascii() into virtual methods in Type_handler
by Alexander Barkov 01 Dec '16

01 Dec '16

Hello Alexey, Thanks for review! I addressed your suggestions. Also, Vicentiu expressed a concern (when reviewing another patch with the same approach) that this kind of cast that I used in sql_type.cc might be unsafe, because it's non-standard. We're discussion other possible solutions now. So I thought that in the meanwhile for the built-in data types we can create methods: String *val_str_ascii_from_val_int(String *str); String *val_str_ascii_from_val_real(String *str); String *val_str_ascii_from_val_str(String *str); and use them from Type_handler_xxx. A new patch is attached. Please see comments below. On 11/30/2016 05:46 PM, Alexey Botchkov wrote: > Hi, Alexander! > > Basically ok with the patch. > Few comments though: > > > +String *Item_func_hex::val_str_ascii_from_int(String *str, ulonglong > num) > > { > ... > > + char ans[65], *ptr; > > + if (!(ptr= longlong2str(num, ans, 16)) || > > + str->copy(ans, (uint32) (ptr - ans), &my_charset_numeric)) > > + return make_empty_result(); // End of memory > > + return str; > > } > > > We can avoid extra copying here like this: > > { > char *n_end; > str->set_charset(&my_charset_numeric); > if (str->alloc(65) || > !(n_end= longlong2str(num, (char *) str->ptr(), 16))) > return make_empty_result(); > str->length((uint32) (n_end - str->ptr()); > return str; > } Thanks for the idea. It's nice to fix it at once. I moved this to String rather than Item_func_hex though: - this helps to avoid (char*) str->ptr(). - it can be useful for other purposes I tried to move octex2hex() from passport.c to int2str.c and reuse it in sql_string.cc, but failed because of strange compilation failures in mysqlbinlog.cc which includes both password.c and sql_string.cc using #include. From my understanding there is a bug in password.c: instead of #include <mysql.h> it should be: #include "mysql.h" I would like to avoid making changes in here for now, so I gave up trying to reuse octet2str(). Instead of that I added APPEND_HEX() and reused it in 3 places in sql_string.cc. > > > class Item_func_hex :public Item_str_ascii_func > > { > .... > > Shouldn't we implement this too: ? > const Type_handler *Item_func_hex::type_handler() > { return m_handler; } Not really. Type handler for Item_func_hex is type_handler_varchar, and it does not depend on the data type of the argument. To avoid confusion, I renamed m_handler to m_arg0_type_handler. > Personally i wouldn't add the m_handler member at all and just do > const Type_handler *Item_func_hex::type_handler() > { return args[0]->m_handler; } > Are you sure the m_handler member works faster? Well you can just forget > that > comment by now, but i'd like to test this someday. I think we should cache it, because it can be expensive to get the handler of args[0] on every row, as it involves virtual methods, which can call more virtual methods recursively. I added a comment about this. > > > +class Item_func_hex_str: public Item_func_hex > > +{ > > +public: > > + String *val_str_ascii(String *str) > > + { > > + /* Convert given string to a hex string, character by character */ > > + String *res= args[0]->val_str(str); > > + if ((null_value= (!res || tmp_value.alloc(res->length() * 2 + 1)))) > > + return 0; > > + tmp_value.length(res->length() * 2); > > + tmp_value.set_charset(&my_charset_latin1); > > + octet2hex((char*) tmp_value.ptr(), res->ptr(), res->length()); > > + return &tmp_value; > > + } > > +}; > > I think it makes more sence to switch the *str and tmp_value > usage here. So the caller gets the 'str' value he sends to the > function as a result. Not that it changes a lot, still i think it's > nicer. Good idea! Done. > > Best regards. > HF >

1 0