- developers - lists.mariadb.org

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

Re: [Maria-developers] 3c941d4: MDEV-5214 Status variables for number of global/db/table/column/role grants
by Sergei Golubchik 13 Mar '15

13 Mar '15

Hi, Vicențiu! That's pretty good, thanks! Although I would like to suggest a somewhat different approach, see below. And there are few minor comments too. On Mar 13, vicentiu(a)mariadb.org wrote: > revision-id: 3c941d471b79ba5d9939a4a1ac61ce5ed8bebaef > parent(s): 197afb413fcc9f06b5e5e6ef41ce980d108b354f > committer: Vicențiu Ciorbaru > branch nick: server > timestamp: 2015-03-13 17:45:07 +0200 > message: > > MDEV-5214 Status variables for number of global/db/table/column/role grants > > Implemented the status variables for use with the feedback plugin. > > --- > mysql-test/suite/plugins/r/acl_statistics.result | 107 +++++++++++++++++++++++ > mysql-test/suite/plugins/t/acl_statistics.test | 70 +++++++++++++++ > sql/mysqld.cc | 34 +++++++ > sql/sql_acl.cc | 34 +++++++ > sql/sql_acl.h | 10 +++ > 5 files changed, 255 insertions(+) > > diff --git a/mysql-test/suite/plugins/t/acl_statistics.test b/mysql-test/suite/plugins/t/acl_statistics.test > new file mode 100644 > index 0000000..e10d907 > --- /dev/null > +++ b/mysql-test/suite/plugins/t/acl_statistics.test > @@ -0,0 +1,70 @@ > +# Test case for validating acl statistics for the feedback plugin. > +--source include/not_embedded.inc > +if (`select count(*) = 0 from information_schema.plugins where plugin_name = 'feedback' and plugin_status='active'`) > +{ > + --skip Feedback plugin is not active > +} Don't bother using feedback plugin for this test. There is nothing in the patch that depends on or uses feedback plugin, so there's no need to make the test depending on it. Please, use SHOW STATUS (or SELECT ... FROM INFORMATION_SCHEMA.GLOBAL_STATUS). And then move the test out of plugins suite. > + > +# First get a baseline of the initial statistics. ... > diff --git a/mysql-test/suite/plugins/r/acl_statistics.result b/mysql-test/suite/plugins/r/acl_statistics.result > new file mode 100644 > index 0000000..dad37bf > --- /dev/null > +++ b/mysql-test/suite/plugins/r/acl_statistics.result > @@ -0,0 +1,107 @@ > +SELECT * FROM information_schema.feedback > +WHERE variable_name like '%Acl%'; > +VARIABLE_NAME VARIABLE_VALUE > +ACL_COUNT_COLUMN_GRANTS 0 > +ACL_COUNT_DATABASE_GRANTS 2 > +ACL_COUNT_FUNCTION_GRANTS 0 > +ACL_COUNT_PROCEDURE_GRANTS 0 > +ACL_COUNT_PROXY_USERS 2 > +ACL_COUNT_ROLE_MAPPINGS 0 > +ACL_COUNT_ROLES 0 > +ACL_COUNT_TABLE_GRANTS 0 > +ACL_COUNT_USERS 4 > +SELECT count(*) COLUMN_GRANTS_COUNT from mysql.columns_priv; > +COLUMN_GRANTS_COUNT > +0 > +SELECT count(*) DATABASE_GRANTS_COUNT from mysql.db; > +DATABASE_GRANTS_COUNT > +2 > +SELECT count(*) FUNCTION_GRANTS_COUNT from mysql.procs_priv where routine_type='FUNCTION'; > +FUNCTION_GRANTS_COUNT > +0 > +SELECT count(*) PROCEDURE_GRANTS_COUNT from mysql.procs_priv where routine_type='PROCEDURE'; > +PROCEDURE_GRANTS_COUNT > +0 > +SELECT count(*) PROXY_USERS from mysql.proxies_priv; > +PROXY_USERS > +2 > +SELECT count(*) ROLE_GRANTS_COUNT from mysql.user where is_role='Y'; > +ROLE_GRANTS_COUNT > +0 > +SELECT count(*) ROLE_MAPPINGS_COUNT from mysql.roles_mapping; > +ROLE_MAPPINGS_COUNT > +0 > +SELECT count(*) TABLE_GRANTS_COUNT from mysql.tables_priv; > +TABLE_GRANTS_COUNT > +0 > +SELECT count(*) USERS_COUNT from mysql.user where is_role='N'; > +USERS_COUNT > +4 > +CREATE USER u1; > +CREATE ROLE r1; > +CREATE ROLE r2; > +GRANT PROXY ON root TO u1; > +GRANT SELECT ON *.* to u1; > +GRANT SELECT ON *.* to r1; > +GRANT DELETE ON mysql.* to u1; > +GRANT DELETE ON mysql.* to r1; > +GRANT INSERT ON mysql.user to u1; > +GRANT INSERT ON mysql.user to r1; > +GRANT UPDATE (host) ON mysql.user to u1; > +GRANT UPDATE (host) ON mysql.user to r1; > +GRANT r1 to u1; > +GRANT r2 to r1; > +create procedure mysql.test_proc (OUT param1 INT) > +begin > +select COUNT(*) into param1 from mysql.roles_mapping; > +end| > +GRANT EXECUTE ON PROCEDURE mysql.test_proc TO r1; > +GRANT EXECUTE ON PROCEDURE mysql.test_proc TO u1; > +CREATE FUNCTION mysql.test_func (param INT) RETURNS INT > +RETURN (SELECT COUNT(*) FROM mysql.user); > +GRANT EXECUTE ON FUNCTION mysql.test_func TO r1; > +GRANT EXECUTE ON FUNCTION mysql.test_func TO u1; > +SELECT * FROM information_schema.feedback > +WHERE variable_name like '%Acl%'; > +VARIABLE_NAME VARIABLE_VALUE > +ACL_COUNT_COLUMN_GRANTS 2 > +ACL_COUNT_DATABASE_GRANTS 4 > +ACL_COUNT_FUNCTION_GRANTS 2 > +ACL_COUNT_PROCEDURE_GRANTS 2 > +ACL_COUNT_PROXY_USERS 3 > +ACL_COUNT_ROLE_MAPPINGS 4 > +ACL_COUNT_ROLES 2 > +ACL_COUNT_TABLE_GRANTS 2 > +ACL_COUNT_USERS 5 > +SELECT count(*) COLUMN_GRANTS_COUNT from mysql.columns_priv; > +COLUMN_GRANTS_COUNT > +2 > +SELECT count(*) DATABASE_GRANTS_COUNT from mysql.db; > +DATABASE_GRANTS_COUNT > +4 > +SELECT count(*) FUNCTION_GRANTS_COUNT from mysql.procs_priv where routine_type='FUNCTION'; > +FUNCTION_GRANTS_COUNT > +2 > +SELECT count(*) PROCEDURE_GRANTS_COUNT from mysql.procs_priv where routine_type='PROCEDURE'; > +PROCEDURE_GRANTS_COUNT > +2 > +SELECT count(*) PROXY_USERS from mysql.proxies_priv; > +PROXY_USERS > +3 > +SELECT count(*) ROLE_GRANTS_COUNT from mysql.user where is_role='Y'; > +ROLE_GRANTS_COUNT > +2 > +SELECT count(*) ROLE_MAPPINGS_COUNT from mysql.roles_mapping; > +ROLE_MAPPINGS_COUNT > +4 Please, swap these two selects (ROLE_GRANTS_COUNT and ROLE_MAPPINGS_COUNT) to have values go in the same order as in the I_S table (it's easier to compare numbers that way). Same for the previous set of selects. > +SELECT count(*) TABLE_GRANTS_COUNT from mysql.tables_priv; > +TABLE_GRANTS_COUNT > +2 > +SELECT count(*) USERS_COUNT from mysql.user where is_role='N'; > +USERS_COUNT > +5 > +DROP PROCEDURE mysql.test_proc; > +DROP FUNCTION mysql.test_func; > +DROP ROLE r2; > +DROP ROLE r1; > +DROP USER u1; > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index aa85a02..a7be18f 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -8149,6 +8149,39 @@ int show_threadpool_idle_threads(THD *thd, SHOW_VAR *var, char *buff, > } > #endif > > +int show_acl_statistics(THD *thd, SHOW_VAR *var, char *buff, > + enum enum_var_type scope) > +{ > + > + var->type= SHOW_ARRAY; > + var->value= buff; > + > + SHOW_VAR *v= (SHOW_VAR *)buff; > + static ACL_statistics acl_stats; > + acl_stats= get_acl_statistics(thd); > + static SHOW_VAR statistics_array[]= { > + {"column_grants", (char *)&acl_stats.column_grants, SHOW_ULONG}, > + {"database_grants", (char *)&acl_stats.database_grants, SHOW_ULONG}, > + {"function_grants", (char *)&acl_stats.function_grants, SHOW_ULONG}, > + {"procedure_grants", (char *)&acl_stats.procedure_grants, SHOW_ULONG}, > + {"proxy_users", (char *)&acl_stats.proxy_users, SHOW_ULONG}, > + {"role_mappings", (char *)&acl_stats.role_mappings, SHOW_ULONG}, rename to "role_grants" please. > + {"roles", (char *)&acl_stats.roles, SHOW_ULONG}, > + {"table_grants", (char *)&acl_stats.table_grants, SHOW_ULONG}, > + {"users", (char *)&acl_stats.users, SHOW_ULONG}, > + {NullS, NullS, SHOW_LONG} > + }; > + v->name= "count"; the name doesn't matter, no need to set it. > + v->value = statistics_array; > + v->type= SHOW_ARRAY; > + v++; > + v->name= NullS; > + v->value= NullS; > + v->type= SHOW_LONG; > + > + return 0; > +} > + > /* > Variables shown by SHOW STATUS in alphabetical order > */ > @@ -8157,6 +8190,7 @@ SHOW_VAR status_vars[]= { > {"Aborted_clients", (char*) &aborted_threads, SHOW_LONG}, > {"Aborted_connects", (char*) &aborted_connects, SHOW_LONG}, > {"Access_denied_errors", (char*) offsetof(STATUS_VAR, access_denied_errors), SHOW_LONG_STATUS}, > + {"Acl", (char*) &show_acl_statistics, SHOW_FUNC}, > {"Binlog_bytes_written", (char*) offsetof(STATUS_VAR, binlog_bytes_written), SHOW_LONGLONG_STATUS}, > {"Binlog_cache_disk_use", (char*) &binlog_cache_disk_use, SHOW_LONG}, > {"Binlog_cache_use", (char*) &binlog_cache_use, SHOW_LONG}, > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index aaa1276..e97d094 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -10340,6 +10340,14 @@ applicable_roles_insert(ACL_USER_BASE *grantee, ACL_ROLE *role, void *ptr) > return 0; > } > > +static my_bool count_column_privileges(void *grant_table, > + void *current_count) > +{ > + HASH hash_columns = ((GRANT_TABLE *)grant_table)->hash_columns; > + *(ulong *) current_count+= hash_columns.records; > + return 0; > +} > + > #else > bool check_grant(THD *, ulong, TABLE_LIST *, bool, uint, bool) > { > @@ -10347,6 +10355,32 @@ bool check_grant(THD *, ulong, TABLE_LIST *, bool, uint, bool) > } > #endif /*NO_EMBEDDED_ACCESS_CHECKS */ > > +ACL_statistics get_acl_statistics(THD* thd) > +{ > + ACL_statistics result; > + bzero(&result, sizeof(result)); > +#ifndef NO_EMBEDDED_ACCESS_CHECKS > + mysql_rwlock_rdlock(&LOCK_grant); > + mysql_mutex_lock(&acl_cache->lock); > + > + result.users= acl_users.elements; > + result.roles= acl_roles.records; > + result.database_grants= acl_dbs.elements; > + result.function_grants= proc_priv_hash.records; > + result.procedure_grants= proc_priv_hash.records; > + result.proxy_users= acl_proxy_users.elements; > + result.role_mappings= acl_roles_mappings.records; > + result.table_grants= column_priv_hash.records; > + my_hash_iterate(&column_priv_hash, count_column_privileges, > + &result.column_grants); > + > + mysql_mutex_unlock(&acl_cache->lock); > + mysql_rwlock_unlock(&LOCK_grant); > + > +#endif > + return result; > +} > + > int fill_schema_enabled_roles(THD *thd, TABLE_LIST *tables, COND *cond) > { > TABLE *table= tables->table; > diff --git a/sql/sql_acl.h b/sql/sql_acl.h > index 55b00a9..3abf5d7 100644 > --- a/sql/sql_acl.h > +++ b/sql/sql_acl.h > @@ -406,4 +406,14 @@ int acl_set_default_role(THD *thd, const char *host, const char *user, > extern ulong role_global_merges, role_db_merges, role_table_merges, > role_column_merges, role_routine_merges; > #endif > +/** > + Specify which statistical information (counts) to retrieve from > + the ACL data structures. > +*/ > +struct ACL_statistics > +{ > + ulong column_grants, database_grants, function_grants, procedure_grants, > + proxy_users, role_mappings, roles, table_grants, users; > +}; > +ACL_statistics get_acl_statistics(THD *thd); > #endif /* SQL_ACL_INCLUDED */ I would do it slightly differently. No ACL_statistics structure and no get_acl_statistics function, but instead I'd use SHOW_ARRAY in mysqld.cc: {"Acl", (char*) &show_acl_statistics, SHOW_ARRAY}, and in sql_acl.cc I'd have an array static SHOW_VAR statistics_array[]= { {"column_grants", (char *)&get_column_grants, SHOW_SIMPLE_FUNC}, {"database_grants", (char *)&acl_dbs.elements, SHOW_ULONG}, {"function_grants", (char *)&proc_priv_hash.records, SHOW_ULONG}, {"procedure_grants", (char *)&proc_priv_hash.records, SHOW_ULONG}, {"proxy_users", (char *)&acl_proxy_users.elements, SHOW_ULONG}, {"role_grants", (char *)&acl_roles_mappings.records, SHOW_ULONG}, {"roles", (char *)&acl_roles.records, SHOW_ULONG}, {"table_grants", (char *)&column_priv_hash.records, SHOW_ULONG}, {"users", (char *)&acl_users.elements, SHOW_ULONG}, {NullS, NullS, SHOW_LONG} }; That is, only column_grants need a function call (with mutex locks), and it's SHOW_SIMPLE_FUNC, not SHOW_FUNC. The different is that a typical (for example) SHOW STATUS LIKE '%sort%' will have to invoke all SHOW_FUNC functions, because otherwise it cannot know names of the variables and cannot match them. But SHOW_SIMPLE_FUNC never returns a SHOW_ARRAY, so it can be matched without a function call (so, no mutex lock unless the user specifically asked for Acl_column_grants value). Also, status variables are allowed to be "slightly imprecise", so they're often incremented without a mutex. That is, you aren't required to protect them here with a mutex either. Regards, Sergei

1 0

[Maria-developers] MDEV-6566 Different INSERT behaviour on bad bytes with and without character set conversion
by Alexander Barkov 13 Mar '15

13 Mar '15

Hi Sergei, Please review a patch for MDEV-6566. Thanks.

2 5

[Maria-developers] Enabling feedback pluging for MariaDB 10.1.4
by Michael Widenius 12 Mar '15

12 Mar '15

Hi! We had originally planned to enable the feedback plugin by default for the Alpha and Beta stages of MariaDB 10.1, but we forgot to do this for the alpha so I suggested Sergei today that we should enable it for the beta period of MariaDB 10.0 We agreed that before doing this we would have a discussion on the maria developer list to explain why we would like to do this. The reason we would like to enable it is to get information about which features people are testing and using in MariaDB 10.1, so that we can concentrate our testing and bug fixing during the beta phase for those areas that people are actually using. This would help our development work a lot and would also help us to plan for MariaDB 10.2 development. Anyone will be able to disable the feedback plugin by adding: --feedback=OFF to their my.cnf file. In MariaDB 10.1 gamma/release we would make it default OFF again. We will of course inform everyone about this change, including how to disable the feedback, in the MariaDB changelog and release notes. We do of course hope that as many as possible will have it on, even after gamma, to help us with our development work! Information about the MariaDB feedback plugin and the statistics it provides can be found at: http://mariadb.org/feedback_plugin https://mariadb.com/kb/feedback-plugin As most MariaDB users should know, the feedback is totally anonymous and no private or sensitive information is being sent. Any comments, suggestions or recommendations? Regards, Michael Widenius Creator of MySQL and MariaDB

10 14

Re: [Maria-developers] 6c772ae: MDEV-6956:SET STATEMENT default_master_connection = ... has no effect
by Sergei Golubchik 12 Mar '15

12 Mar '15

Hi, Sanja! On Mar 06, sanja(a)mariadb.com wrote: > revision-id: 6c772ae62a0a43ea227f491509c02efb99c5ce92 > parent(s): da181fee4ec849985b15a758c6b26058a0f5e318 > committer: Oleksandr Byelkin > branch nick: server > timestamp: 2015-03-06 15:51:51 +0100 > message: > > MDEV-6956:SET STATEMENT default_master_connection = ... has no effect > > the problem was in assigning default value during parsing. Ok to push, but see below. > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 8025340..8255306 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -2445,7 +2445,9 @@ optional_connection_name: > /* empty */ > { > LEX *lex= thd->lex; > - lex->mi.connection_name= thd->variables.default_master_connection; > + lex->mi.connection_name.str= NullS; > + lex->mi.connection_name.length= 0; > + //thd->variables.default_master_connection; you've forgot to remove this commented-out line. and prefer to use null_lex_str: lex->mi.connection_name= null_lex_str; Regards, Sergei

2 2

Re: [Maria-developers] 7ceff99: MDEV-6954: SET STATEMENT rand_seedX = ...FOR ... makes the next rand() to return 0
by Sergei Golubchik 11 Mar '15

11 Mar '15

Hi, Sanja! On Mar 11, sanja(a)mariadb.com wrote: > revision-id: 7ceff99ff25b74bb188848ce0e5f9e2f1ddac3d0 > parent(s): 9f4ee16bf351ea2969d73abcdea92e70538c723c > committer: Oleksandr Byelkin > branch nick: server > timestamp: 2015-03-11 11:02:13 +0100 > message: > > MDEV-6954: SET STATEMENT rand_seedX = ...FOR ... makes the next rand() to return 0 > > The variables now return real values. They can't be made usual variables because they do not have DEFAULT value. > > diff --git a/mysql-test/suite/sys_vars/inc/sysvars_server.inc b/mysql-test/suite/sys_vars/inc/sysvars_server.inc > index fb6121d..3a9a312 100644 > --- a/mysql-test/suite/sys_vars/inc/sysvars_server.inc > +++ b/mysql-test/suite/sys_vars/inc/sysvars_server.inc > @@ -17,6 +17,7 @@ select * from information_schema.system_variables > where variable_name not like 'aria%' and > variable_name not like 'debug%' and > variable_name not like 'wsrep%' and > + variable_name not like 'rand_seed%%' and 1. why two %% ? :) 2. there are only two variables here, better list them explicilty in the "not in" list 3. if you remove variables from the output here, please add them to the select below. > variable_name not in ( > 'have_openssl', > 'have_symlink', Regards, Sergei

1 0

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

11 Mar '15

Hi, Sanja! On Feb 25, sanja(a)mariadb.com wrote: > revision-id: 60c1dc1ab24bfe9b3f4a898fe5e7cb070ee02cf7 > parent(s): d0d6284cab70af148269d95744f615454cac24bb > committer: Oleksandr Byelkin > branch nick: work-maria-10.1-MDEV-6997-real > timestamp: 2015-02-25 19:00:57 +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 5af700b..54abfd0 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -2728,6 +2728,10 @@ static bool do_execute_sp(THD *thd, sp_head *sp) > my_error(ER_WRONG_ARGUMENTS, MYF(0), "SET"); > goto error; > } > + if (!thd->in_sub_stmt) /* stored functions and triggers are a special case */ > + { > + thd->stmt_depends_on_first_successful_insert_id_in_prev_stmt= 0; > + } > } This seems to be a pretty complicated matter, and your comment isn't very helpful. Try this: The value of last_insert_id is remembered in THD to be written to binlog 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 stmt_depends_on_first_successful_insert_id_in_prev_stmt flag. And try the test case where SET STATEMENT last_insert_id is used inside a trigger or a stored function. I don't think you handle this case correctly. Try both cases, when stmt_depends... is 0 and is 1 at the point of this if(). I suspect you should not set stmt_depends_on_first_successful_insert_id_in_prev_stmt to 0, but you need to make sure it didn't change. That is, you need to set it to its old value (which may or may not be 0). Regards, Sergei

1 0

Re: [Maria-developers] [Commits] 828a7b6: MDEV-6858: enforce_storage_engine option
by Sergei Golubchik 11 Mar '15

11 Mar '15

Hi, Jan! This looks good, thanks! There's just one strange thing I've noticed, see sysvars_server_notembedded test below. I don't understand why it's happening, could you investigate, please? On Mar 10, Jan Lindström wrote: > revision-id: 828a7b67768c7e7a72c2a687f5a3ff77bc61536b > parent(s): f7d196bb2e605766668895027c48728f03803734 > committer: Jan Lindström > branch nick: 10.1-innodb > timestamp: 2015-03-10 08:28:51 +0200 > message: > > MDEV-6858: enforce_storage_engine option > > Merge from Percona Server enforced use of a specific storage engine > authored by Stewart Smith. > > Modified to be session variable and modifiable only by SUPER. Use > similar implementation as default_storage_engine. > > diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > index 83c0196..bf8f868 100644 > --- a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > +++ b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > @@ -721,6 +721,20 @@ NUMERIC_BLOCK_SIZE NULL > ENUM_VALUE_LIST OFF,ON > READ_ONLY NO > COMMAND_LINE_ARGUMENT OPTIONAL > +VARIABLE_NAME ENFORCE_STORAGE_ENGINE > +SESSION_VALUE > +GLOBAL_VALUE NULL This looks strange. Why session value is empty string, but global value is NULL? I believe they should be identical, shouldn't they? Preferrably, both should be empty string - I don't think we have system variables that can be NULL. > +GLOBAL_VALUE_ORIGIN COMPILE-TIME > +DEFAULT_VALUE NULL > +VARIABLE_SCOPE SESSION ONLY > +VARIABLE_TYPE VARCHAR > +VARIABLE_COMMENT Force the use of a storage engine for new tables > +NUMERIC_MIN_VALUE NULL > +NUMERIC_MAX_VALUE NULL > +NUMERIC_BLOCK_SIZE NULL > +ENUM_VALUE_LIST NULL > +READ_ONLY NO > +COMMAND_LINE_ARGUMENT NULL > VARIABLE_NAME ERROR_COUNT > SESSION_VALUE 0 > GLOBAL_VALUE NULL > diff --git a/mysql-test/t/enforce_storage_engine.test b/mysql-test/t/enforce_storage_engine.test > new file mode 100644 > index 0000000..80839e2 > --- /dev/null > +++ b/mysql-test/t/enforce_storage_engine.test > @@ -0,0 +1,64 @@ > +-- source include/have_innodb.inc You can keep it this way, but I'd recommend to change to either have_xtradb.inc or (better) not use innodb at all and test with myisam and memory engines. with have_innodb.inc the test will be run twice - for innodb and for xtradb, which is a waste of time, because it doesn't do anything engine-specific here. > +-- source include/not_embedded.inc > + > +--disable_warnings > +drop table if exists t1; > +--enable_warnings Regards, Sergei

2 1

Re: [Maria-developers] [Commits] Rev 4382: MDEV-6892: WHERE does not apply in file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6892/
by Sergey Petrunia 11 Mar '15

11 Mar '15

Hi Sanja, Please find my feedback below. We should discuss it. On Fri, Dec 19, 2014 at 01:24:12PM +0000, sanja(a)askmonty.org wrote: > At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6892/ > > ------------------------------------------------------------ > revno: 4382 > revision-id: sanja(a)askmonty.org-20141219132350-6bo1g7q6gbivqdky > parent: svoj(a)mariadb.org-20141201105829-ctaoe194abtorhz2 > committer: sanja(a)askmonty.org > branch nick: work-maria-5.5-MDEV-6892 > timestamp: Fri 2014-12-19 14:23:50 +0100 > message: > MDEV-6892: WHERE does not apply > > Taking into account implicit dependence of constant view field from nullable table of left join added. > > Fixed finding real table to check if ut turned to NULL (materialized view & derived taken into account) ... > === modified file 'sql/item.cc' > --- a/sql/item.cc 2014-10-06 17:53:55 +0000 > +++ b/sql/item.cc 2014-12-19 13:23:50 +0000 > @@ -9790,12 +9790,36 @@ void Item_ref::update_used_tables() > (*ref)->update_used_tables(); > } > > +void Item_direct_view_ref::update_used_tables() > +{ > + if (view->is_inner_table_of_outer_join()) > + { > + null_ref_table= NULL; > + check_null_ref(); update_used_tables() is called during optimization. Why does it call check_null_ref() which may check null_ref_table->null_row? Is check_null_ref() function an execution-time function (and so it computes "current" value of an item) or is it an optimization-time function (and so it computes static attributes and can be called at any point in the query) It looks like it's a bit of both, which is really confusing. Could you please clarify? check_null_ref() was introduced by you in another views+outer joins bugfix. Can we get it documented? > + } > + Item_direct_ref::update_used_tables(); > +} Overall, I don't understand why some checks are in used_tables() and some are in update_used_tables(). Could you clarify? > + > table_map Item_direct_view_ref::used_tables() const > { > + TABLE *null_ref= null_ref_table; > + > + if (view->is_inner_table_of_outer_join()) > + { > + if (null_ref == NULL) > + null_ref= view->get_real_join_table(); > + else if (null_ref == NO_NULL_TABLE) > + null_ref= NULL; > + } > + else > + null_ref= NULL; > + > return get_depended_from() ? > OUTER_REF_TABLE_BIT : > ((view->is_merged_derived() || view->merged || !view->table) ? > - (*ref)->used_tables() : > + ((*ref)->used_tables() ? > + (*ref)->used_tables() : > + (null_ref ? null_ref->map : (table_map)0 )) : > view->table->map); Two-level nesting of conditional expressions is too complicated. Please change into 'if' statement and a comment about what's going on. How can Item_direct_view_ref() deped on an outer table?? > } > > > === modified file 'sql/item.h' > --- a/sql/item.h 2014-11-18 14:42:40 +0000 > +++ b/sql/item.h 2014-12-19 13:23:50 +0000 > @@ -3319,7 +3319,8 @@ class Item_direct_view_ref :public Item_ > { > if (null_ref_table == NULL) > { > - if (!(null_ref_table= view->get_real_join_table())) > + if (!view->is_inner_table_of_outer_join() || > + !(null_ref_table= view->get_real_join_table())) > null_ref_table= NO_NULL_TABLE; > } > if (null_ref_table != NO_NULL_TABLE && null_ref_table->null_row) > @@ -3329,6 +3330,7 @@ class Item_direct_view_ref :public Item_ > } > return FALSE; > } > + > public: > Item_direct_view_ref(Name_resolution_context *context_arg, Item **item, > const char *table_name_arg, > @@ -3353,8 +3355,10 @@ public: > bool subst_argument_checker(uchar **arg); > Item *equal_fields_propagator(uchar *arg); > Item *replace_equal_field(uchar *arg); > - table_map used_tables() const; > + table_map used_tables() const; > + void update_used_tables(); > table_map not_null_tables() const; > + bool const_item() const { return used_tables() == 0; } > bool walk(Item_processor processor, bool walk_subquery, uchar *arg) > { > return (*ref)->walk(processor, walk_subquery, arg) || > > === modified file 'sql/table.cc' > --- a/sql/table.cc 2014-10-29 10:22:48 +0000 > +++ b/sql/table.cc 2014-12-19 13:23:50 +0000 The function get_real_join_table() lacks comments, which makes it difficult to review. Do I understand correctly that its action can be described by this comment: /* @brief Given a merged view (or a merged derived table), find its first* base** table. @detail (*) first means as defined in the syntax (**) base means we need a physical table, one that has a bit in table->map and is present in select_lex->leaf_tables. Example: t1 join ( (select * from t2, t3 ) as derivedA join (select * from t5, t6) as derivedC ) as derivedB here, derivedB->get_real_join_table()="t2" */ > @@ -5012,7 +5012,8 @@ TABLE *TABLE_LIST::get_real_join_table() > TABLE_LIST *tbl= this; > while (tbl->table == NULL || tbl->table->reginfo.join_tab == NULL) I'm quite surprised that joins are run over tables that don't have reginfo.join_tab set appropriately... > { > - if (tbl->view == NULL && tbl->derived == NULL) > + if ((tbl->view == NULL && tbl->derived == NULL) || > + tbl->is_materialized_derived()) I'm wondering why part of conditions are in while(...) and the other part is here. > break; > /* we do not support merging of union yet */ > DBUG_ASSERT(tbl->view == NULL || > The code below this line looks bizarre (and it is still from your recent fixes to VIEWs): { List_iterator_fast<TABLE_LIST> ti; { List_iterator_fast<TABLE_LIST> ti(tbl->view != NULL ? why define 'ti' and then immediately redefine it? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

2 1