- developers - lists.mariadb.org

Re: [Maria-developers] [Commits] 97037da: Replace static usage of AES_CTR with current encryption algorithm.
by Jeremy Cole 17 Mar '15

17 Mar '15

> > I don't get it. Why is example_key_management_plugin special here? If >> you're going to support "none", why not here as well? >> > > This is exactly similar to file_key_management_plugin i.e. do not > initialize if NONE. > Heh, okay. > + >>> + my_aes_init_dynamic_encrypt(current_aes_dynamic_method); >>> >> >> Isn't this redundant? It should already be initialized by the call in >> init_server_components... >> >> >> > Maybe, but what if you load this plugin after server has started ? > Why would it matter? Unless I miss something, my_aes_encrypt_dynamic/my_aes_decrypt_dynamic are global. But the whole encryption_algorithm stuff seems not well thought out in any case. Regards, Jeremy

2 1

Re: [Maria-developers] [Commits] 97037da: Replace static usage of AES_CTR with current encryption algorithm.
by Jeremy Cole 17 Mar '15

17 Mar '15

Jan, > - my_aes_init_dynamic_encrypt(MY_AES_ALGORITHM_CTR); > + if (current_aes_dynamic_method == MY_AES_ALGORITHM_NONE) > + { > + sql_print_error("No encryption method choosen with > --encryption-algorithm. " > + "example_key_management_plugin disabled"); > + return 1; > + } > I don't get it. Why is example_key_management_plugin special here? If you're going to support "none", why not here as well? > + > + my_aes_init_dynamic_encrypt(current_aes_dynamic_method); > Isn't this redundant? It should already be initialized by the call in init_server_components... Regards, Jeremy

1 0

[Maria-developers] MDEV-6714: A couple of questions
by Vicențiu Ciorbaru 16 Mar '15

16 Mar '15

Hi Sergei, I've achieved the desired performance speedup related to MDEV-6714. You can check the preliminary patch on the commits mailing-list. I'll clean up the code and then submit a final version for review. I do however have two concerns: * By using SHOW TABLES LIKE %s, we performed a case insensitive search to find the table's correct name case. This command takes a long time with a lot of tables. Using an equality operator in I_D speeds it up, but it no longer matches if we have a different case. Possible solution: Try to search for the table with the provided string via INFORMATION_SCHEMA using the equals operator. If we can not find the table, fall back to a case insensitive search that takes longer. * The I_S.triggers and I_S.tables are available since 5.5 and 5.1 respectively. I suppose it is safe to assume we will have these tables available when running mysqldump. I can code all the changes to fall back to the previous query if the one that uses I_S tables fails. Regards, Vicentiu

3 3

Re: [Maria-developers] 53b73a7: MDEV-6714 mysqldump slow with tables in big databases
by Sergei Golubchik 16 Mar '15

16 Mar '15

Hi, Vicentiu! On Mar 16, vicentiu(a)mariadb.org wrote: > revision-id: 53b73a7940039809a02184a69502142d767b0741 > parent(s): 197afb413fcc9f06b5e5e6ef41ce980d108b354f > committer: Vicențiu Ciorbaru > branch nick: server > timestamp: 2015-03-16 21:59:26 +0200 > message: > > MDEV-6714 mysqldump slow with tables in big databases > > First draft commit. Performance speedup achieved. > > --- > client/mysqldump.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/client/mysqldump.c b/client/mysqldump.c > index 2da4ce6..6138e94 100644 > --- a/client/mysqldump.c > +++ b/client/mysqldump.c > @@ -3341,7 +3341,8 @@ static int dump_triggers_for_table(char *table_name, char *db_name) > /* Get list of triggers. */ > > my_snprintf(query_buff, sizeof(query_buff), > - "SHOW TRIGGERS LIKE %s", > + "SELECT TRIGGER_NAME from INFORMATION_SCHEMA.TRIGGERS " > + "where TRIGGER_SCHEMA = DATABASE() and EVENT_OBJECT_TABLE = %s", This is wrong, should be EVENT_OBJECT_SCHEMA, not TRIGGER_SCHEMA. Why? That's how I_S optimizations work - for file-based objects it might either use readdir to get all files and then select those that it needs. Or it can open the requested file directly, if it knows the database (= directory name) and the object name (= file name). It kind of works like an index over (db_name, object_name). And what fields are "indexed" is specified in the ST_SCHEMA_TABLE structure for the I_S table in question. For I_S.TRIGGERS it's {"TRIGGERS", triggers_fields_info, 0, get_all_tables, make_old_format, get_schema_triggers_record, 5, 6, 0, OPEN_TRIGGER_ONLY|OPTIMIZE_I_S_TABLE}, where triggers_fields_info is the list of fields and 5,6 are "indexed" fields. Looking at triggers_fields_info you'll see that 5,6 fields are EVENT_OBJECT_SCHEMA and EVENT_OBJECT_TABLE. > quote_for_like(table_name, name_buff)); > > if (mysql_query_with_error_report(mysql, &show_triggers_rs, query_buff)) > @@ -4716,8 +4717,12 @@ static char *get_actual_table_name(const char *old_table_name, MEM_ROOT *root) > > /* Check memory for quote_for_like() */ > DBUG_ASSERT(2*sizeof(old_table_name) < sizeof(show_name_buff)); > - my_snprintf(query, sizeof(query), "SHOW TABLES LIKE %s", > + my_snprintf(query, sizeof(query), > + "select table_name from information_schema.tables " > + "where table_schema = DATABASE() " > + "and table_name = %s", > quote_for_like(old_table_name, show_name_buff)); This is wrong, here and everywhere. quote_for_like() quotes, exactly, for LIKE. It is a wrong kind of quoting for equality operator. > + verbose_msg(query); > Regards, Sergei

1 0

Re: [Maria-developers] Gsoc 2015 - Enhancing mysqlbinlog
by Kristian Nielsen 16 Mar '15

16 Mar '15

Christos Daskalakis <daskalakis.chris(a)gmail.com> writes: > i found this very interesting project on gsoc 2015 and with another > colleague (Stelios Agapiou steagap06(a)gmail.com) we would be very > interested and happy to work on Enhancing mysqlbinlog , as it is referred > to in mariadb.com . I assume you are refering to MDEV-4989: https://mariadb.atlassian.net/browse/MDEV-4989 Rohit Kashyap also showed some interest in this project, and it was discussed a bit on the maria-developers@ mailing list (Cc:ed). So let's coordinate the efforts here. I updated the MDEV-4989 with what was discussed so far, it now better reflects what the project entails. It also now has some pointers to suggested background reading. Thank you for your interest in MariaDB, and MariaDB GTID! - Kristian.

2 1

Re: [Maria-developers] 7c21ea9: MDEV-7772: SIGSEGV on my_aes_encrypt_cbc when -DWITH_SSL=bundled
by Sergei Golubchik 14 Mar '15

14 Mar '15

Hi, Jan! On Mar 14, Jan Lindström wrote: > > and thanks for the review, some comments/questions below: > > > 1. Does file_key_management_plugin work with CTR? > > > Yes it does, but only with openSSL. Then I'd suggest to * add aes_ctr combination to have_file_key_management_plugin.combinations * use only one combinations file, not two identical ones (it can be called, say, encryption_algorithms.inc) and included into both have_*_key_management_plugin.inc files * disable aes_ctr combination from suite.pm, now it has $skip{'include/have_openssl_ctr.inc'} = 'no or too old openssl' unless $::mysqld_variables{'version-ssl-library'} =~ /OpenSSL (\S+)/ and $1 ge "1.0.1"; which disables the complete have_openssl_ctr.inc, it can be changed to disable just one combination (like for have_innodb.inc): $skip{'include/encryption_algorithms.combinations'} = [ 'aes_ctr' ] unless $::mysqld_variables{'version-ssl-library'} =~ /OpenSSL (\S+)/ and $1 ge "1.0.1"; > > This doesn't make a lot of sense now, because > > example_key_management_plugin forces CTR: > > > > my_aes_init_dynamic_encrypt(MY_AES_ALGORITHM_CTR); > > > Is there some reason why it does this here? I will change that on separate > fix to current_aes_dynamic_method if != MY_AES_ALGORITHM_NONE I suspect it was in the original google patch and nobody fixed it yet. It would be good to change it. Regards, Sergei

1 0

Re: [Maria-developers] 7ef6793: MDEV-6650 - LINT_INIT emits code in non-debug builds
by Sergei Golubchik 13 Mar '15

13 Mar '15

Hi, Sergey! On Feb 10, svoj(a)mariadb.org wrote: > revision-id: 7ef679355dfe07eba88964e78a3303e6eddd618c > parent(s): f13939061d468a47985dee0268652a6fe2db7862 > committer: Sergey Vojtovich > branch nick: 10.1 > timestamp: 2015-02-10 14:05:49 +0400 > message: > > MDEV-6650 - LINT_INIT emits code in non-debug builds > > Replaced all references to LINT_INIT with UNINIT_VAR and LINT_INIT_STRUCT. > Removed LINT_INIT macro. Ok to push Regards, Sergei

1 0

Re: [Maria-developers] 7c21ea9: MDEV-7772: SIGSEGV on my_aes_encrypt_cbc when -DWITH_SSL=bundled
by Sergei Golubchik 13 Mar '15

13 Mar '15

Hi, Jan! On Mar 13, Jan Lindström wrote: > revision-id: 7c21ea9f502ded155c12a0ee3c5ff0602e9d4c9a > parent(s): 4d0e52189ca945c94146035c2733d85c9c6fd18d > committer: Jan Lindström > branch nick: 10.1-innodb > timestamp: 2015-03-13 14:18:07 +0200 > message: > > MDEV-7772: SIGSEGV on my_aes_encrypt_cbc when -DWITH_SSL=bundled > > Two problems: > - Read/Write outside of buffer at memcpy() because of incorrect parameters > . OPENSSL_assert(EVP_CIPHER_CTX_iv_length(&ctx.ctx) == iv_length); // ECB does not use IV, thus incorrect assertion > > Added: > mysql-test/include/have_file_key_management_plugin.combinations ( to run with aes_cbc and aes_ecb) > mysql-test/include/have_openssl_ctr.combinations ( to run with aes_cbc, aes_ecb and aes_ctr) The patch is ok, of course. Just a few suggestions, see below. Ok to push after that. > diff --git a/mysql-test/include/have_file_key_management_plugin.combinations b/mysql-test/include/have_file_key_management_plugin.combinations > new file mode 100644 > index 0000000..6a63b5a > --- /dev/null > +++ b/mysql-test/include/have_file_key_management_plugin.combinations > @@ -0,0 +1,6 @@ > +[aes_cbc] > +encryption-algorithm=aes_cbc > + > +[aes_ecb] > +encryption-algorithm=aes_ecb > + 1. Does file_key_management_plugin work with CTR? 2. suggestion: use shorter names for combinations (I'd remove "aes_" prefix) test names are long enough already. See replication combinations - they are 3-4 letters only (rbr/mix/stmt). > diff --git a/mysql-test/include/have_openssl_ctr.combinations b/mysql-test/include/have_openssl_ctr.combinations > new file mode 100644 > index 0000000..2c49c78 > --- /dev/null > +++ b/mysql-test/include/have_openssl_ctr.combinations > @@ -0,0 +1,9 @@ > +[aes_cbc] > +encryption-algorithm=aes_cbc > + > +[aes_ecb] > +encryption-algorithm=aes_ecb > + > +[aes_ctr] > +encryption-algorithm=aes_ctr > + This doesn't make a lot of sense now, because example_key_management_plugin forces CTR: my_aes_init_dynamic_encrypt(MY_AES_ALGORITHM_CTR); so it always uses ctr, no matter what encryption-algorithm is. which is very confusing, SHOW VARIABLES will show, say, aes_ecb, but in fact it will be aes_ctr under the hood. furthermore, it doesn't check for the return value (so it'll fail later or will misbehave on old openssl versions that don't support CTR). This all is a separate bug (in Jira already), but for now I'm just saying that it's kind of pointless to run example_key_management_plugin with different encryption-algorithm values. > diff --git a/mysql-test/suite/innodb/t/innodb_scrub_background.test b/mysql-test/suite/innodb/t/innodb_scrub_background.test > index 44cb16b..881c124 100644 > --- a/mysql-test/suite/innodb/t/innodb_scrub_background.test > +++ b/mysql-test/suite/innodb/t/innodb_scrub_background.test > @@ -1,6 +1,7 @@ > -- source include/have_innodb.inc > -- source include/not_embedded.inc > -- source include/have_example_key_management_plugin.inc > +-- source include/have_openssl_ctr.inc that's fine, although you could've simply included have_openssl_ctr.inc from have_example_key_management_plugin.inc :) > > let $MYSQLD_DATADIR=`select @@datadir`; > let ib1_IBD = $MYSQLD_DATADIR/ibdata1; > diff --git a/storage/innobase/fil/fil0crypt.cc b/storage/innobase/fil/fil0crypt.cc > index 823c74a..10d49bb 100644 > --- a/storage/innobase/fil/fil0crypt.cc > +++ b/storage/innobase/fil/fil0crypt.cc > @@ -734,7 +734,7 @@ fil_space_encrypt(ulint space, ulint offset, lsn_t lsn, > uint32 dstlen; > > if (page_compressed) { > - srclen = page_size - FIL_PAGE_DATA;; > + srclen = page_size - FIL_PAGE_DATA; I'd suggest to move this and all following changes to a separate commit. > } > > int rc = (* my_aes_encrypt_dynamic)(src, srclen, Regards, Sergei

1 0

Re: [Maria-developers] 20e5c37: MDEV-7728 - Spiral patch 032_mariadb-10.0.15.scale_registering_xid.diff
by Sergei Golubchik 13 Mar '15

13 Mar '15

Hi, Sergey! On Mar 12, svoj(a)mariadb.org wrote: > revision-id: 20e5c37ccfac9d95a2db1f295f211656df1ce160 > parent(s): 190858d996e7dc90e01fe8a5e7daec6af0012b23 > committer: Sergey Vojtovich > branch nick: 10.1 > timestamp: 2015-03-12 18:15:08 +0400 > message: > > MDEV-7728 - Spiral patch 032_mariadb-10.0.15.scale_registering_xid.diff > > XID cache is now based on lock-free hash. > Also fixed lf_hash_destroy() to call alloc destructor. > > Note that previous implementation had race condition when thread was accessing > XA owned by different thread. This new implementation doesn't fix it either. > > diff --git a/include/lf.h b/include/lf.h > index 88024bc..19bdafc 100644 > --- a/include/lf.h > +++ b/include/lf.h > @@ -117,7 +117,12 @@ void lf_pinbox_init(LF_PINBOX *pinbox, uint free_ptr_offset, > #define lf_alloc_free(PINS, PTR) lf_pinbox_free((PINS), (PTR)) > #define lf_alloc_get_pins(A) lf_pinbox_get_pins(&(A)->pinbox) > #define lf_alloc_put_pins(PINS) lf_pinbox_put_pins(PINS) > -#define lf_alloc_direct_free(ALLOC, ADDR) my_free((ADDR)) > +#define lf_alloc_direct_free(ALLOC, ADDR) \ > + do { \ > + if ((ALLOC)->destructor) \ > + (ALLOC)->destructor((uchar*) ADDR); \ > + my_free(ADDR); \ > + } while(0) > > void *lf_alloc_new(LF_PINS *pins); > > diff --git a/mysql-test/suite/innodb/r/innodb_bug59641.result b/mysql-test/suite/innodb/r/innodb_bug59641.result > index 5062c69..476385f 100644 > --- a/mysql-test/suite/innodb/r/innodb_bug59641.result > +++ b/mysql-test/suite/innodb/r/innodb_bug59641.result > @@ -43,8 +43,8 @@ COMMIT; > XA RECOVER; > formatID gtrid_length bqual_length data > 1 3 0 789 > -1 3 0 456 > 1 3 0 123 > +1 3 0 456 > XA ROLLBACK '123'; > XA ROLLBACK '456'; > XA COMMIT '789'; > diff --git a/mysql-test/suite/perfschema/r/server_init.result b/mysql-test/suite/perfschema/r/server_init.result > index 20dc130..1bdb988 100644 > --- a/mysql-test/suite/perfschema/r/server_init.result > +++ b/mysql-test/suite/perfschema/r/server_init.result > @@ -120,10 +120,6 @@ where name like "wait/synch/mutex/sql/LOCK_audit_mask"; > count(name) > 1 > select count(name) from mutex_instances > -where name like "wait/synch/mutex/sql/LOCK_xid_cache"; > -count(name) > -1 > -select count(name) from mutex_instances > where name like "wait/synch/mutex/sql/LOCK_plugin"; > count(name) > 1 > diff --git a/mysql-test/suite/perfschema/r/setup_instruments_defaults.result b/mysql-test/suite/perfschema/r/setup_instruments_defaults.result > index e502376..b0570d7 100644 > --- a/mysql-test/suite/perfschema/r/setup_instruments_defaults.result > +++ b/mysql-test/suite/perfschema/r/setup_instruments_defaults.result > @@ -5,14 +5,14 @@ SELECT * FROM performance_schema.setup_instruments > WHERE name IN ( > 'wait/synch/mutex/sql/LOCK_user_conn', > 'wait/synch/mutex/sql/LOCK_uuid_generator', > -'wait/synch/mutex/sql/LOCK_xid_cache', > +'wait/synch/mutex/sql/LOCK_plugin', > 'stage/sql/creating table') > AND enabled = 'yes' AND timed = 'no' > ORDER BY name; > NAME ENABLED TIMED > stage/sql/creating table YES NO > +wait/synch/mutex/sql/LOCK_plugin YES NO > wait/synch/mutex/sql/LOCK_user_conn YES NO > -wait/synch/mutex/sql/LOCK_xid_cache YES NO > SELECT * FROM performance_schema.setup_instruments > WHERE name = 'wait/synch/mutex/sql/LOCK_thread_count' > AND enabled = 'no' AND timed = 'no'; > diff --git a/mysql-test/suite/perfschema/t/server_init.test b/mysql-test/suite/perfschema/t/server_init.test > index d5a7e18..c6d25f1 100644 > --- a/mysql-test/suite/perfschema/t/server_init.test > +++ b/mysql-test/suite/perfschema/t/server_init.test > @@ -118,9 +118,6 @@ select count(name) from mutex_instances > where name like "wait/synch/mutex/sql/LOCK_audit_mask"; > > select count(name) from mutex_instances > - where name like "wait/synch/mutex/sql/LOCK_xid_cache"; > - > -select count(name) from mutex_instances > where name like "wait/synch/mutex/sql/LOCK_plugin"; > > # Not a global variable, may be destroyed already. > diff --git a/mysql-test/suite/perfschema/t/setup_instruments_defaults-master.opt b/mysql-test/suite/perfschema/t/setup_instruments_defaults-master.opt > index 408eb5c..ed6702e 100644 > --- a/mysql-test/suite/perfschema/t/setup_instruments_defaults-master.opt > +++ b/mysql-test/suite/perfschema/t/setup_instruments_defaults-master.opt > @@ -12,7 +12,7 @@ > --loose-performance-schema-instrument='wait/synch/mutex/sql/LOCK_thread_count=OFF' > --loose-performance-schema-instrument=' wait/synch/mutex/sql/LOCK_user_conn = COUNTED' > --loose-performance-schema-instrument='wait%/synch/mutex/sql/LOCK_uu%_genera%/= COUNTED' > ---loose-performance-schema-instrument='%%wait/synch/mutex/sql/LOCK_xid_cache=COUNTED' > +--loose-performance-schema-instrument='%%wait/synch/mutex/sql/LOCK_plugin=COUNTED' > --loose-performance-schema-instrument='%=FOO' > --loose-performance-schema-instrument='%=%' > --loose-performance-schema-instrument='%' > diff --git a/mysql-test/suite/perfschema/t/setup_instruments_defaults.test b/mysql-test/suite/perfschema/t/setup_instruments_defaults.test > index e1f6140..5e0a3a5 100644 > --- a/mysql-test/suite/perfschema/t/setup_instruments_defaults.test > +++ b/mysql-test/suite/perfschema/t/setup_instruments_defaults.test > @@ -15,7 +15,7 @@ SELECT * FROM performance_schema.setup_instruments > WHERE name IN ( > 'wait/synch/mutex/sql/LOCK_user_conn', > 'wait/synch/mutex/sql/LOCK_uuid_generator', > - 'wait/synch/mutex/sql/LOCK_xid_cache', > + 'wait/synch/mutex/sql/LOCK_plugin', > 'stage/sql/creating table') > AND enabled = 'yes' AND timed = 'no' > ORDER BY name; > diff --git a/sql/handler.cc b/sql/handler.cc > index 12d7ffb..a45419f 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -1931,12 +1931,28 @@ int ha_recover(HASH *commit_list) > so mysql_xa_recover does not filter XID's to ensure uniqueness. > It can be easily fixed later, if necessary. > */ > + > +static my_bool xa_recover_callback(XID_STATE *xs, Protocol *protocol) > +{ > + if (xs->xa_state == XA_PREPARED) > + { > + protocol->prepare_for_resend(); > + protocol->store_longlong((longlong) xs->xid.formatID, FALSE); > + protocol->store_longlong((longlong) xs->xid.gtrid_length, FALSE); > + protocol->store_longlong((longlong) xs->xid.bqual_length, FALSE); > + protocol->store(xs->xid.data, xs->xid.gtrid_length + xs->xid.bqual_length, > + &my_charset_bin); > + if (protocol->write()) > + return TRUE; > + } > + return FALSE; > +} > + > + > bool mysql_xa_recover(THD *thd) > { > List<Item> field_list; > Protocol *protocol= thd->protocol; > - int i=0; > - XID_STATE *xs; > DBUG_ENTER("mysql_xa_recover"); > > field_list.push_back(new Item_int("formatID", 0, MY_INT32_NUM_DECIMAL_DIGITS)); > @@ -1948,26 +1964,9 @@ bool mysql_xa_recover(THD *thd) > Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF)) > DBUG_RETURN(1); > > - mysql_mutex_lock(&LOCK_xid_cache); > - while ((xs= (XID_STATE*) my_hash_element(&xid_cache, i++))) > - { > - if (xs->xa_state==XA_PREPARED) > - { > - protocol->prepare_for_resend(); > - protocol->store_longlong((longlong)xs->xid.formatID, FALSE); > - protocol->store_longlong((longlong)xs->xid.gtrid_length, FALSE); > - protocol->store_longlong((longlong)xs->xid.bqual_length, FALSE); > - protocol->store(xs->xid.data, xs->xid.gtrid_length+xs->xid.bqual_length, > - &my_charset_bin); > - if (protocol->write()) > - { > - mysql_mutex_unlock(&LOCK_xid_cache); > - DBUG_RETURN(1); > - } > - } > - } > - > - mysql_mutex_unlock(&LOCK_xid_cache); > + if (xid_cache_iterate(thd, (my_hash_walk_action) xa_recover_callback, > + protocol)) > + DBUG_RETURN(1); > my_eof(thd); > DBUG_RETURN(0); > } > diff --git a/sql/handler.h b/sql/handler.h > index 5ef9208..5ab67c0 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -612,11 +612,11 @@ struct xid_t { > return sizeof(formatID)+sizeof(gtrid_length)+sizeof(bqual_length)+ > gtrid_length+bqual_length; > } > - uchar *key() > + uchar *key() const > { > return (uchar *)&gtrid_length; > } > - uint key_length() > + uint key_length() const > { > return sizeof(gtrid_length)+sizeof(bqual_length)+gtrid_length+bqual_length; > } > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index b97742d..6c06240 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -4889,11 +4889,7 @@ static int init_server_components() > my_charset_error_reporter= charset_error_reporter; > #endif > > - if (xid_cache_init()) > - { > - sql_print_error("Out of memory"); > - unireg_abort(1); > - } > + xid_cache_init(); > > /* > initialize delegates for extension observers, errors have already > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 5b0d04d..d59ef52 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -914,7 +914,8 @@ bool Drop_table_error_handler::handle_condition(THD *thd, > wait_for_commit_ptr(0), > main_da(0, false, false), > m_stmt_da(&main_da), > - tdc_hash_pins(0) > + tdc_hash_pins(0), > + xid_hash_pins(0) > #ifdef WITH_WSREP > , > wsrep_applier(is_wsrep_applier), > @@ -1593,7 +1594,7 @@ void THD::cleanup(void) > > transaction.xid_state.xa_state= XA_NOTR; > trans_rollback(this); > - xid_cache_delete(&transaction.xid_state); > + xid_cache_delete(this, &transaction.xid_state); > > DBUG_ASSERT(open_tables == NULL); > /* > @@ -1704,6 +1705,8 @@ void THD::cleanup(void) > main_da.free_memory(); > if (tdc_hash_pins) > lf_hash_put_pins(tdc_hash_pins); > + if (xid_hash_pins) > + lf_hash_put_pins(xid_hash_pins); > /* Ensure everything is freed */ > if (status_var.local_memory_used != 0) > { > @@ -5106,120 +5109,205 @@ void mark_transaction_to_rollback(THD *thd, bool all) > /*************************************************************************** > Handling of XA id cacheing > ***************************************************************************/ > +class XID_cache_element > +{ > + uint m_state; shouldn't it be uint32 ? That's the type that my_atomic_add32/cas32 work with. > + static const uint DELETED= 1 << 31; > +public: > + XID_STATE *m_xid_state; O-okay. I *think* your hand-written locking logic works. But please add a comment, explaining how it works, all four methods, the meaning of DELETED, etc. Perhaps DELETED should rather be NOT_INITIALIZED? > + bool lock() > + { > + if (my_atomic_add32_explicit(&m_state, 1, MY_MEMORY_ORDER_ACQUIRE) & DELETED) > + { > + unlock(); > + return false; > + } > + return true; > + } > + void unlock() > + { > + my_atomic_add32_explicit(&m_state, -1, MY_MEMORY_ORDER_RELEASE); > + } > + void mark_deleted() > + { > + uint old= 0; > + while (!my_atomic_cas32_weak_explicit(&m_state, &old, DELETED, > + MY_MEMORY_ORDER_RELAXED, > + MY_MEMORY_ORDER_RELAXED)) usually one should use LF_BACKOFF inside spin-loops. > + old= 0; > + } > + void mark_not_deleted() > + { > + DBUG_ASSERT(m_state & DELETED); > + my_atomic_add32_explicit(&m_state, -DELETED, MY_MEMORY_ORDER_RELAXED); > + } > + static void lf_hash_initializer(LF_HASH *hash __attribute__((unused)), > + XID_cache_element *element, > + XID_STATE *xid_state) > + { > + element->m_xid_state= xid_state; > + xid_state->xid_cache_element= element; > + } > + static void lf_alloc_constructor(uchar *ptr) > + { > + XID_cache_element *element= (XID_cache_element*) (ptr + LF_HASH_OVERHEAD); > + element->m_state= DELETED; > + } > + static void lf_alloc_destructor(uchar *ptr) > + { > + XID_cache_element *element= (XID_cache_element*) (ptr + LF_HASH_OVERHEAD); > + if (element->m_state != DELETED) How can this happen? You mark an element DELETED before lf_hash_delete() > + { > + DBUG_ASSERT(!element->m_xid_state->in_thd); > + my_free(element->m_xid_state); > + } > + } > + static uchar *key(const XID_cache_element *element, size_t *length, > + my_bool not_used __attribute__((unused))) > + { > + *length= element->m_xid_state->xid.key_length(); > + return element->m_xid_state->xid.key(); > + } > +}; > > -mysql_mutex_t LOCK_xid_cache; > -HASH xid_cache; > > -extern "C" uchar *xid_get_hash_key(const uchar *, size_t *, my_bool); > -extern "C" void xid_free_hash(void *); > +static LF_HASH xid_cache; > +static bool xid_cache_inited; > > -uchar *xid_get_hash_key(const uchar *ptr, size_t *length, > - my_bool not_used __attribute__((unused))) > -{ > - *length=((XID_STATE*)ptr)->xid.key_length(); > - return ((XID_STATE*)ptr)->xid.key(); > -} > > -void xid_free_hash(void *ptr) > +bool THD::fix_xid_hash_pins() > { > - if (!((XID_STATE*)ptr)->in_thd) > - my_free(ptr); > + if (!xid_hash_pins) > + xid_hash_pins= lf_hash_get_pins(&xid_cache); > + return !xid_hash_pins; > } > > -#ifdef HAVE_PSI_INTERFACE > -static PSI_mutex_key key_LOCK_xid_cache; > > -static PSI_mutex_info all_xid_mutexes[]= > +void xid_cache_init() > { > - { &key_LOCK_xid_cache, "LOCK_xid_cache", PSI_FLAG_GLOBAL} > -}; > - > -static void init_xid_psi_keys(void) > -{ > - const char* category= "sql"; > - int count; > - > - if (PSI_server == NULL) > - return; > - > - count= array_elements(all_xid_mutexes); > - PSI_server->register_mutex(category, all_xid_mutexes, count); > + xid_cache_inited= true; > + lf_hash_init(&xid_cache, sizeof(XID_cache_element), LF_HASH_UNIQUE, 0, 0, > + (my_hash_get_key) XID_cache_element::key, &my_charset_bin); > + xid_cache.alloc.constructor= XID_cache_element::lf_alloc_constructor; > + xid_cache.alloc.destructor= XID_cache_element::lf_alloc_destructor; > + xid_cache.initializer= > + (lf_hash_initializer) XID_cache_element::lf_hash_initializer; > } > -#endif /* HAVE_PSI_INTERFACE */ > - > -bool xid_cache_init() > -{ > -#ifdef HAVE_PSI_INTERFACE > - init_xid_psi_keys(); > -#endif > > - mysql_mutex_init(key_LOCK_xid_cache, &LOCK_xid_cache, MY_MUTEX_INIT_FAST); > - return my_hash_init(&xid_cache, &my_charset_bin, 100, 0, 0, > - xid_get_hash_key, xid_free_hash, 0) != 0; > -} > > void xid_cache_free() > { > - if (my_hash_inited(&xid_cache)) > + if (xid_cache_inited) > { > - my_hash_free(&xid_cache); > - mysql_mutex_destroy(&LOCK_xid_cache); > + lf_hash_destroy(&xid_cache); > + xid_cache_inited= false; > } > } > > -XID_STATE *xid_cache_search(XID *xid) > + > +XID_STATE *xid_cache_search(THD *thd, XID *xid) > { > - mysql_mutex_lock(&LOCK_xid_cache); > - XID_STATE *res=(XID_STATE *)my_hash_search(&xid_cache, xid->key(), > - xid->key_length()); > - mysql_mutex_unlock(&LOCK_xid_cache); > - return res; > + DBUG_ASSERT(thd->xid_hash_pins); > + XID_cache_element *element= > + (XID_cache_element*) lf_hash_search(&xid_cache, thd->xid_hash_pins, > + xid->key(), xid->key_length()); > + if (element) > + { > + lf_hash_search_unpin(thd->xid_hash_pins); > + return element->m_xid_state; Normally, one should not access the element after it's unpinned, so the protocol is XID_STATE *state= element->m_xid_state; lf_hash_search_unpin(thd->xid_hash_pins); return state; Perhaps your locking/deleted m_state guarantees that element is safe to access? But I failed to see that :( > + } > + return 0; > } > > > bool xid_cache_insert(XID *xid, enum xa_states xa_state) > { > XID_STATE *xs; > - my_bool res; > - mysql_mutex_lock(&LOCK_xid_cache); > - if (my_hash_search(&xid_cache, xid->key(), xid->key_length())) > - res=0; > - else if (!(xs=(XID_STATE *)my_malloc(sizeof(*xs), MYF(MY_WME)))) > - res=1; > - else > + LF_PINS *pins; > + int res= 1; > + > + if (!(pins= lf_hash_get_pins(&xid_cache))) > + return true; > + > + if ((xs= (XID_STATE*) my_malloc(sizeof(*xs), MYF(MY_WME)))) > { > xs->xa_state=xa_state; > xs->xid.set(xid); > xs->in_thd=0; > xs->rm_error=0; > - res=my_hash_insert(&xid_cache, (uchar*)xs); > + > + if ((res= lf_hash_insert(&xid_cache, pins, xs))) > + my_free(xs); > + else > + xs->xid_cache_element->mark_not_deleted(); > + if (res == 1) > + res= 0; > } > - mysql_mutex_unlock(&LOCK_xid_cache); > + lf_hash_put_pins(pins); > return res; > } > > > -bool xid_cache_insert(XID_STATE *xid_state) > +bool xid_cache_insert(THD *thd, XID_STATE *xid_state) > { > - mysql_mutex_lock(&LOCK_xid_cache); > - if (my_hash_search(&xid_cache, xid_state->xid.key(), > - xid_state->xid.key_length())) > + if (thd->fix_xid_hash_pins()) > + return true; > + > + int res= lf_hash_insert(&xid_cache, thd->xid_hash_pins, xid_state); > + switch (res) > { > - mysql_mutex_unlock(&LOCK_xid_cache); > + case 0: > + xid_state->xid_cache_element->mark_not_deleted(); > + break; > + case 1: > my_error(ER_XAER_DUPID, MYF(0)); > - return true; > + default: > + xid_state->xid_cache_element= 0; > } > - bool res= my_hash_insert(&xid_cache, (uchar*)xid_state); > - mysql_mutex_unlock(&LOCK_xid_cache); > return res; > } > > > -void xid_cache_delete(XID_STATE *xid_state) > +void xid_cache_delete(THD *thd, XID_STATE *xid_state) > +{ > + if (xid_state->xid_cache_element) > + { > + DBUG_ASSERT(thd->xid_hash_pins); > + xid_state->xid_cache_element->mark_deleted(); > + lf_hash_delete(&xid_cache, thd->xid_hash_pins, > + xid_state->xid.key(), xid_state->xid.key_length()); > + xid_state->xid_cache_element= 0; > + if (!xid_state->in_thd) > + my_free(xid_state); > + } > +} > + > + > +struct xid_cache_iterate_arg > +{ > + my_hash_walk_action action; > + void *argument; > +}; > + > +static my_bool xid_cache_iterate_callback(XID_cache_element *element, > + xid_cache_iterate_arg *arg) > +{ > + my_bool res= FALSE; > + if (element->lock()) > + { > + res= arg->action(element->m_xid_state, arg->argument); > + element->unlock(); > + } > + return res; > +} > + > +int xid_cache_iterate(THD *thd, my_hash_walk_action action, void *arg) > { > - mysql_mutex_lock(&LOCK_xid_cache); > - my_hash_delete(&xid_cache, (uchar *)xid_state); > - mysql_mutex_unlock(&LOCK_xid_cache); > + xid_cache_iterate_arg argument= { action, arg }; > + return thd->fix_xid_hash_pins() ? -1 : > + lf_hash_iterate(&xid_cache, thd->xid_hash_pins, > + (my_hash_walk_action) xid_cache_iterate_callback, > + &argument); > } > > > diff --git a/sql/sql_class.h b/sql/sql_class.h > index bd145bd..200f541 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -1105,6 +1105,7 @@ struct st_savepoint { > > enum xa_states {XA_NOTR=0, XA_ACTIVE, XA_IDLE, XA_PREPARED, XA_ROLLBACK_ONLY}; > extern const char *xa_state_names[]; > +class XID_cache_element; > > typedef struct st_xid_state { > /* For now, this is only used to catch duplicated external xids */ > @@ -1113,16 +1114,16 @@ struct st_savepoint { > bool in_thd; > /* Error reported by the Resource Manager (RM) to the Transaction Manager. */ > uint rm_error; > + XID_cache_element *xid_cache_element; > } XID_STATE; > > -extern mysql_mutex_t LOCK_xid_cache; > -extern HASH xid_cache; > -bool xid_cache_init(void); > +void xid_cache_init(void); > void xid_cache_free(void); > -XID_STATE *xid_cache_search(XID *xid); > +XID_STATE *xid_cache_search(THD *thd, XID *xid); > bool xid_cache_insert(XID *xid, enum xa_states xa_state); > -bool xid_cache_insert(XID_STATE *xid_state); > -void xid_cache_delete(XID_STATE *xid_state); > +bool xid_cache_insert(THD *thd, XID_STATE *xid_state); > +void xid_cache_delete(THD *thd, XID_STATE *xid_state); > +int xid_cache_iterate(THD *thd, my_hash_walk_action action, void *argument); > > /** > @class Security_context > @@ -3785,6 +3786,8 @@ class THD :public Statement, > } > > LF_PINS *tdc_hash_pins; > + LF_PINS *xid_hash_pins; > + bool fix_xid_hash_pins(); > > inline ulong wsrep_binlog_format() const > { > diff --git a/sql/transaction.cc b/sql/transaction.cc > index 22e3ad7..d6ef160 100644 > --- a/sql/transaction.cc > +++ b/sql/transaction.cc > @@ -738,7 +738,7 @@ bool trans_xa_start(THD *thd) > thd->transaction.xid_state.xa_state= XA_ACTIVE; > thd->transaction.xid_state.rm_error= 0; > thd->transaction.xid_state.xid.set(thd->lex->xid); > - if (xid_cache_insert(&thd->transaction.xid_state)) > + if (xid_cache_insert(thd, &thd->transaction.xid_state)) > { > thd->transaction.xid_state.xa_state= XA_NOTR; > thd->transaction.xid_state.xid.null(); > @@ -801,7 +801,7 @@ bool trans_xa_prepare(THD *thd) > my_error(ER_XAER_NOTA, MYF(0)); > else if (ha_prepare(thd)) > { > - xid_cache_delete(&thd->transaction.xid_state); > + xid_cache_delete(thd, &thd->transaction.xid_state); > thd->transaction.xid_state.xa_state= XA_NOTR; > my_error(ER_XA_RBROLLBACK, MYF(0)); > } > @@ -830,6 +830,11 @@ bool trans_xa_commit(THD *thd) > > if (!thd->transaction.xid_state.xid.eq(thd->lex->xid)) > { > + if (thd->fix_xid_hash_pins()) > + { > + my_error(ER_OUT_OF_RESOURCES, MYF(0)); > + DBUG_RETURN(TRUE); > + } > /* > xid_state.in_thd is always true beside of xa recovery procedure. > Note, that there is no race condition here between xid_cache_search > @@ -840,7 +845,7 @@ bool trans_xa_commit(THD *thd) > xa_cache_insert(XID, xa_states), which is called before starting > client connections, and thus is always single-threaded. > */ > - XID_STATE *xs= xid_cache_search(thd->lex->xid); > + XID_STATE *xs= xid_cache_search(thd, thd->lex->xid); > res= !xs || xs->in_thd; > if (res) > my_error(ER_XAER_NOTA, MYF(0)); > @@ -848,7 +853,7 @@ bool trans_xa_commit(THD *thd) > { > res= xa_trans_rolled_back(xs); > ha_commit_or_rollback_by_xid(thd->lex->xid, !res); > - xid_cache_delete(xs); > + xid_cache_delete(thd, xs); > } > DBUG_RETURN(res); > } > @@ -911,7 +916,7 @@ bool trans_xa_commit(THD *thd) > thd->server_status&= > ~(SERVER_STATUS_IN_TRANS | SERVER_STATUS_IN_TRANS_READONLY); > DBUG_PRINT("info", ("clearing SERVER_STATUS_IN_TRANS")); > - xid_cache_delete(&thd->transaction.xid_state); > + xid_cache_delete(thd, &thd->transaction.xid_state); > thd->transaction.xid_state.xa_state= XA_NOTR; > > DBUG_RETURN(res); > @@ -935,14 +940,20 @@ bool trans_xa_rollback(THD *thd) > > if (!thd->transaction.xid_state.xid.eq(thd->lex->xid)) > { > - XID_STATE *xs= xid_cache_search(thd->lex->xid); > + if (thd->fix_xid_hash_pins()) > + { > + my_error(ER_OUT_OF_RESOURCES, MYF(0)); > + DBUG_RETURN(TRUE); > + } > + > + XID_STATE *xs= xid_cache_search(thd, thd->lex->xid); > if (!xs || xs->in_thd) > my_error(ER_XAER_NOTA, MYF(0)); > else > { > xa_trans_rolled_back(xs); > ha_commit_or_rollback_by_xid(thd->lex->xid, 0); > - xid_cache_delete(xs); > + xid_cache_delete(thd, xs); > } > DBUG_RETURN(thd->get_stmt_da()->is_error()); > } > @@ -961,7 +972,7 @@ bool trans_xa_rollback(THD *thd) > thd->server_status&= > ~(SERVER_STATUS_IN_TRANS | SERVER_STATUS_IN_TRANS_READONLY); > DBUG_PRINT("info", ("clearing SERVER_STATUS_IN_TRANS")); > - xid_cache_delete(&thd->transaction.xid_state); > + xid_cache_delete(thd, &thd->transaction.xid_state); > thd->transaction.xid_state.xa_state= XA_NOTR; > > DBUG_RETURN(res); > diff --git a/storage/spider/spd_table.cc b/storage/spider/spd_table.cc > index 36768ae..411c7ae 100644 > --- a/storage/spider/spd_table.cc > +++ b/storage/spider/spd_table.cc > @@ -41,11 +41,13 @@ I'd prefer to have spider changes in a separate commit > #include "spd_malloc.h" > > ulong *spd_db_att_thread_id; > +#if MYSQL_VERSION_ID < 100103 > #ifdef XID_CACHE_IS_SPLITTED > uint *spd_db_att_xid_cache_split_num; > #endif > pthread_mutex_t *spd_db_att_LOCK_xid_cache; > HASH *spd_db_att_xid_cache; > +#endif > struct charset_info_st *spd_charset_utf8_bin; > const char **spd_defaults_extra_file; > const char **spd_defaults_file; > @@ -6263,7 +6265,7 @@ int spider_db_init( > "?LOCK_xid_cache@@3PAUst_mysql_mutex@@A")); > spd_db_att_xid_cache = *((HASH **) > GetProcAddress(current_module, "?xid_cache@@3PAUst_hash@@A")); > -#else > +#elif MYSQL_VERSION_ID < 100103 > spd_db_att_LOCK_xid_cache = (pthread_mutex_t *) > #if MYSQL_VERSION_ID < 50500 > GetProcAddress(current_module, > @@ -6289,7 +6291,7 @@ int spider_db_init( > spd_db_att_xid_cache_split_num = &opt_xid_cache_split_num; > spd_db_att_LOCK_xid_cache = LOCK_xid_cache; > spd_db_att_xid_cache = xid_cache; > -#else > +#elif MYSQL_VERSION_ID < 100103 > spd_db_att_LOCK_xid_cache = &LOCK_xid_cache; > spd_db_att_xid_cache = &xid_cache; > #endif > diff --git a/storage/spider/spd_trx.cc b/storage/spider/spd_trx.cc > index a66fa5a..1b02bb8 100644 > --- a/storage/spider/spd_trx.cc > +++ b/storage/spider/spd_trx.cc > @@ -38,11 +38,13 @@ > #include "spd_ping_table.h" > #include "spd_malloc.h" > > +#if MYSQL_VERSION_ID < 100103 > #ifdef XID_CACHE_IS_SPLITTED > extern uint *spd_db_att_xid_cache_split_num; > #endif > extern pthread_mutex_t *spd_db_att_LOCK_xid_cache; > extern HASH *spd_db_att_xid_cache; > +#endif > extern struct charset_info_st *spd_charset_utf8_bin; > > extern handlerton *spider_hton_ptr; > @@ -1641,6 +1643,13 @@ int spider_xa_lock( > int error_num; > const char *old_proc_info; > DBUG_ENTER("spider_xa_lock"); > +#if MYSQL_VERSION_ID >= 100103 > + old_proc_info = thd_proc_info(thd, "Locking xid by Spider"); > + error_num= 0; > + if (xid_cache_insert(thd, xid_state)) > + error_num= thd->get_stmt_da()->sql_errno() == ER_XAER_DUPID ? > + ER_SPIDER_XA_LOCKED_NUM : HA_ERR_OUT_OF_MEM; > +#else > #ifdef SPIDER_HAS_HASH_VALUE_TYPE > my_hash_value_type hash_value = my_calc_hash(spd_db_att_xid_cache, > (uchar*) xid_state->xid.key(), xid_state->xid.key_length()); > @@ -1699,6 +1708,7 @@ int spider_xa_lock( > #else > pthread_mutex_unlock(spd_db_att_LOCK_xid_cache); > #endif > +#endif > thd_proc_info(thd, old_proc_info); > DBUG_RETURN(error_num); > } > @@ -1709,6 +1719,10 @@ int spider_xa_unlock( > THD *thd = current_thd; > const char *old_proc_info; > DBUG_ENTER("spider_xa_unlock"); > +#if MYSQL_VERSION_ID >= 100103 > + old_proc_info = thd_proc_info(thd, "Unlocking xid by Spider"); > + xid_cache_delete(thd, xid_state); > +#else > #if defined(SPIDER_HAS_HASH_VALUE_TYPE) && defined(HASH_UPDATE_WITH_HASH_VALUE) > my_hash_value_type hash_value = my_calc_hash(spd_db_att_xid_cache, > (uchar*) xid_state->xid.key(), xid_state->xid.key_length()); > @@ -1738,6 +1752,7 @@ int spider_xa_unlock( > #else > pthread_mutex_unlock(spd_db_att_LOCK_xid_cache); > #endif > +#endif > thd_proc_info(thd, old_proc_info); > DBUG_RETURN(0); > } > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits Regards, Sergei

2 1

Re: [Maria-developers] b723778: MDEV-6997: SET STATEMENT last_insert_id FOR ... does not affect the value written to the binlog
by Sergei Golubchik 13 Mar '15

13 Mar '15

Hi, Sanja! Ok to push, thanks. But see one confusing typo below. On Mar 13, sanja(a)mariadb.com wrote: > revision-id: b7237786f5b8e74ba9558db8325d29a64f1a1d9b > parent(s): ba3573cae8f0d269452cc6213f1a8243e455830e > committer: Oleksandr Byelkin > branch nick: server > timestamp: 2015-03-13 16:32:47 +0100 > message: > > MDEV-6997: SET STATEMENT last_insert_id FOR ... does not affect the value written to the binlog > > Problem was in reinitialization of first_successful_insert_id_in_prev_stmt_for_binlog after setting variables. > > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 0837a70..41df891 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -2743,6 +2743,18 @@ static bool do_execute_sp(THD *thd, sp_head *sp) > lex->restore_set_statement_var(); > goto error; > } > + /* > + The value of last_insert_id is remembered in THD to be written to binlog Hmm, I thought you'll put it in the commit comment. But ok, having it in the code is fine too, whatever you prefer. > + when it's used *the first time* in the statement. But SET STATEMENT > + must read the old value of last_insert_id to be able to restore it at > + the end. This should not count at "reading of last_insert_id" and > + should not remember last_insert_id for binlog. That is, it should set typo: it should *reset* (or *clear*) not *set* > + stmt_depends_on_first_successful_insert_id_in_prev_stmt flag. > + */ > + if (!thd->in_sub_stmt) > + { > + thd->stmt_depends_on_first_successful_insert_id_in_prev_stmt= 0; > + } > } Regards, Sergei

1 0