- developers - lists.mariadb.org

[Maria-developers] Buildbot slow upload
by Kristian Nielsen 13 Jan '15

13 Jan '15

Hi Serg, Daniel, Some time ago there was discussion of slow Buildbot upload speed (sorry, don't have the original mail thread to reply to). I found some new information on this. Yesterday I had to do a custom build, but was not able to because my tarbake upload kept getting stuck. So I had to investigate. Turns out the real problem was that hasky:/media/backup/archive/pack was full. And apparently, the error handling in Buildbot upload step is broken, so the error is not caught properly but just causes the upload to hang. I have noticed this before, that upload gets stuck. I wonder if full disk on hasky has caused this before. Daniel: Do we have something that removes old builds from /archive/pack? I did not see anything in root or buildbot crontabs? With respect to slow uploads, while trying various things, one thing I did was increase the upload blocksize in Buildbot config. Now I see tarbake upload (50MB) taking just 1 minute, which seems fair (probably it can be more if the network or buildbot is busy). So I'm wondering if the slow upload that was seen was in fact a combination of too low default blocksize, and uploads occasionally getting _completely_ stuck? It's of course still fine to move to using rsync, as Serg proposed earlier; just wanted to mention what I found. And in either case, we still need a solution to prevent /archive/pack from running full in the future... - Kristian.

2 1

[Maria-developers] MDEV-7288 USER/ROLE: CREATE OR REPLACE, CREATE IF NOT EXISTS, DROP IF EXISTS
by Alexander Barkov 09 Jan '15

09 Jan '15

Hi Sergei, Please have a look into the patch for MDEV-7288. It consists of the USER/ROLE related part of the big GSoC patch from Sriram that you reviewed earlier, with review suggestions addressed. You earlier had the following suggestions/questions: >> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc >> index 5f077c2..1e156be 100644 >> --- a/sql/sql_acl.cc >> +++ b/sql/sql_acl.cc >> ... >> @@ -9215,10 +9252,26 @@ bool mysql_create_user(THD *thd, List <LEX_USER> &lis >> */ >> if (handle_grant_data(tables, 0, user_name, NULL)) >> { >> - append_user(thd, &wrong_users, user_name); >> - >> - result= TRUE; >> - continue; >> + if (thd->lex->is_create_or_replace()) >> + { >> + if (handle_grant_data(tables, 1, user_name, NULL) <= 0) >> + { >> + append_user(thd, &wrong_users, user_name); > > I don't understand that at all. If user creation failed, you try to drop > it, but don't create a user again, after the old one was dropped. How > could it work? > Yeah, it looks confusing, because handle_grant_data() can do different things depending on the arguments. The call for handle_grant_data(tables, 0, user_name, NULL) does not create any users. It only checks if the user already exists. The call for handle_grant_data(tables, 1, user_name, NULL) drops the user. And the call for replace_user_table() after this block is the one that actually creates the user. I have added some comments in the new code, to make it clearer. >>> @@ -9268,7 +9321,7 @@ bool mysql_create_user(THD *thd, List <LEX_USER> &list, >>> (handle_as_role) ? "CREATE ROLE" : "CREATE USER", >>> wrong_users.c_ptr_safe()); >>> >>> - if (some_users_created) >>> + if (some_users_created || !result) >> >> What is that for? To binlog CREATE IF NOT EXISTS? Yes, that was the reason. I renamed "some_users_created" to "binlog" and changed the logic slightly to set "binlog" to true under the block checking lex->create_info.if_not_exists(), instead of this additional test for "result". So now it should be easier to read: if (binlog) result |= write_bin_log(thd, FALSE, thd->query(), thd->query_length()); Thanks.

3 3

[Maria-developers] prefix matching for command-line options - how to disable?
by Sergei Golubchik 06 Jan '15

06 Jan '15

Hi, We've got a bit of a problem with command-line option prefixes in the test suite. As you know, in MariaDB a command line option (the same applies to my.cnf too) doesn't need to be specified completely, it's sufficient to use an unambigous prefix of it. For example, 'net-retry-count=5' in my.cnf can be abbreviated as 'net-ret=5'. But not as 'net-re=5', the latter will fail with an error mysqld: ambiguous option '--net-re' (net_read_timeout, net_retry_count) This is the historical behavior of MySQL since about 2002. But it creates problems for plugins. InnoDB is particularly badly affected - it has lots of I_S plugins that are prefixes of other I_S plugins. For example: INNODB_BUFFER_PAGE INNODB_BUFFER_PAGE_LRU ... INNODB_CMP INNODB_CMPMEM INNODB_CMPMEM_RESET INNODB_CMP_PER_INDEX INNODB_CMP_PER_INDEX_RESET INNODB_CMP_RESET ... INNODB_SYS_FOREIGN INNODB_SYS_FOREIGN_COLS ... INNODB_SYS_TABLES INNODB_SYS_TABLESPACES INNODB_SYS_TABLESTATS also, it has plugins that match other plugin variables, like INNODB_LOCKS and innodb_locks_unsafe_for_binlog. As a result one cannot see or predict what these options will do. For example --enable-innodb-buffer-page might enable INNODB_BUFFER_PAGE plugin. Or it might enable INNODB_BUFFER_PAGE_LRU plugin - depending on which plugin will loaded (and will see and consume the option) first. And one cannot possibly disable INNODB_BUFFER_PAGE plugin, if the server will try to load INNODB_BUFFER_PAGE_LRU first. I see few solutions for this issue: 1. support a special suffix to option names that will disable prefix matching, for example: mysqld --enable-innodb-buffer-page! this will not match innodb-buffer-page-lru, so it's unambiguous. Of course, any dedicated character can be used, --enable-innodb-buffer-page$, or even a prefix --enable-exact-innodb-buffer-page, --enable-strict-innodb-buffer-page 2. add a special command-line option to disable prefix matching: mysqld --disable-getopt-prefix-matching --enable-innodb-buffer-page 3. do like mysql-5.7 is doing and completely remove support for prefix matching. In 5.7 one has to specified option names in full. I suspect that this is likely to break many exising setups. Opinions? Anybody cares about this obscure issue at all? Regards, Sergei

3 3

[Maria-developers] Buildslave terrier2 disabled due to KVM problems, needs fixing to be re-enabled
by Kristian Nielsen 05 Jan '15

05 Jan '15

Jani, Daniel, I discovered that our buildslave terrier2 seems to have problems with its KVM. Builds are incredibly slow, and it appears that it is not using KVM at all. So it is apparently using just QEMU, and soft-emulating the CPU. This is too slow to be useful, I think. So for now, I've removed all usage of terrier2 from the Buildbot config. If one of you wants to fix terrier2 to be useful again, then the patch to revert (to re-enable terrier2) is this: revid:knielsen@knielsen-hq.org-20150105182519-7492d95v1blss8jf http://bazaar.launchpad.net/~maria-captains/mariadb-tools/trunk/revision/kn… Before re-enabling, KVM needs to be somehow fixed to work (eg. something like `time perl -le 'for (0..100000000){}'` needs to be tested to run at about the same speed inside a guest VM as directly on the host). Maybe it would be best to upgrade the OS on the machine, maybe to the same as we have on bb01/bb02? We have had a number of problems on terrier2 with various KVM issues over the time. - Kristian.

1 0

[Maria-developers] New buildbot? - debpkg101-sid from Otto's github
by Otto Kekäläinen 02 Jan '15

02 Jan '15

Hello Daniel! Thanks for setting up the current buildbots that build my branches of 5.5 and 10.0. Could you now please also set up one the build from my 10.1 branch as I have started working on it too? The buildbot should be a copy of http://buildbot.askmonty.org/buildbot/builders/debpkg-sid/ but so that it pulls in the branch 10.1 from the repository https://github.com/ottok/mariadb Thanks! 2014-09-01 11:25 GMT+03:00 Otto Kekäläinen <otto(a)seravo.fi>: > Hello! > > 2014-03-14 12:59 GMT+02:00 Kristian Nielsen <knielsen(a)knielsen-hq.org>: >> You can see the two builders here: >> >> http://buildbot.askmonty.org/buildbot/builders/debpkg-trusty >> http://buildbot.askmonty.org/buildbot/builders/debpkg-sid/ > > Thanks Kristian and Daniel for creating these in March. > > The trusty build testing is less relevant nowadays while testing 10.0 > has become relevant. > > Could you please delete the buildbot debpkg-trusty and create a copy > of debpkg-sid called debpkg-10.0-sid which would be exactly like > debpkg-sid, but follow commits in this repo: > https://github.com/ottok/mariadb-10.0 ? > > Thanks! > > PS. I already configured the webhook in Github to be called in the > mariadb-10.0 repo as it is done in the mariadb-5.5 repo.

1 2

Re: [Maria-developers] [Commits] b2f45ba: MDEV-7324 - Lock-free hash for table definition cache
by Sergei Golubchik 28 Dec '14

28 Dec '14

Hi, Sergey! On Dec 22, svoj(a)mariadb.org wrote: > revision-id: b2f45ba8280c97f8dc8b1adb3234cf2e28e48982 > parent(s): 4a469b0ed68bb341e1a1e1ba540a45a3b6ff3ef9 > committer: Sergey Vojtovich > branch nick: 10.1 > timestamp: 2014-12-22 19:56:48 +0400 > message: > > MDEV-7324 - Lock-free hash for table definition cache That looked good to me. I didn't find anything obviously wrong. There are mostly questions below. Some are style suggestions (like, renaming of methods, etc). The only place I'm not sure about is early lf_hash_search_unpin() in tdc_acquire_share(). > diff --git a/mysys/lf_hash.c b/mysys/lf_hash.c > --- a/mysys/lf_hash.c > +++ b/mysys/lf_hash.c > @@ -122,7 +122,7 @@ static int lfind(LF_SLIST * volatile *head, CHARSET_INFO *cs, uint32 hashnr, > { > if (unlikely(callback)) > { > - if (callback(cursor->curr + 1, (void*)key)) > + if (cur_hashnr & 1 && callback(cursor->curr + 1, (void*)key)) Sorry for this. > return 1; > } > else if (cur_hashnr >= hashnr) > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -912,17 +912,17 @@ bool Drop_table_error_handler::handle_condition(THD *thd, > #endif /* defined(ENABLED_DEBUG_SYNC) */ > wait_for_commit_ptr(0), > main_da(0, false, false), > - m_stmt_da(&main_da) > + m_stmt_da(&main_da), > #ifdef WITH_WSREP > - , > wsrep_applier(is_wsrep_applier), > wsrep_applier_closing(false), > wsrep_client_thread(false), > wsrep_apply_toi(false), > wsrep_po_handle(WSREP_PO_INITIALIZER), > wsrep_po_cnt(0), > - wsrep_apply_format(0) > + wsrep_apply_format(0), > #endif > + tdc_hash_pins(0) Eh... I actually wanted wsrep related data to the at the end of the THD. Just to keep the rest of the THD layout identical for wsrep and non-wsrep builds. In case a wsrep-enabled plugin is loaded into non-wsrep server or visa versa. That was an early change, though, I've done more cleanups after that and moved wsrep into a service, so perhaps (even, most probably) this is not needed anymore. But I'd better stay on the safe side and keep wsrep at the end of THD. > { > ulong tmp; > > @@ -1698,6 +1698,8 @@ void THD::cleanup(void) > > free_root(&main_mem_root, MYF(0)); > main_da.free_memory(); > + if (tdc_hash_pins) > + lf_hash_put_pins(tdc_hash_pins); no tdc_hash_pins=0 here? > if (status_var.memory_used != 0) > { > DBUG_PRINT("error", ("memory_used: %lld", status_var.memory_used)); > diff --git a/sql/table.h b/sql/table.h > --- a/sql/table.h > +++ b/sql/table.h > @@ -611,32 +612,7 @@ struct TABLE_SHARE > mysql_mutex_t LOCK_ha_data; /* To protect access to ha_data */ > mysql_mutex_t LOCK_share; /* To protect TABLE_SHARE */ > > - typedef I_P_List <TABLE, TABLE_share> TABLE_list; > - typedef I_P_List <TABLE, All_share_tables> All_share_tables_list; > - struct > - { > - /** > - Protects ref_count, m_flush_tickets, all_tables, free_tables, flushed, > - all_tables_refs. > - */ > - mysql_mutex_t LOCK_table_share; > - mysql_cond_t COND_release; > - TABLE_SHARE *next, **prev; /* Link to unused shares */ > - uint ref_count; /* How many TABLE objects uses this */ > - uint all_tables_refs; /* Number of refs to all_tables */ > - /** > - List of tickets representing threads waiting for the share to be flushed. > - */ > - Wait_for_flush_list m_flush_tickets; > - /* > - Doubly-linked (back-linked) lists of used and unused TABLE objects > - for this share. > - */ > - All_share_tables_list all_tables; > - TABLE_list free_tables; > - ulong version; > - bool flushed; > - } tdc; > + TDC_element *tdc; Hmm. Ok. I think I like this change - TABLE_SHARE now contains table metadata and everything related to tdc is moved to TDC_element. Right? > > LEX_CUSTRING tabledef_version; > > diff --git a/sql/table.cc b/sql/table.cc > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -3845,11 +3844,11 @@ bool TABLE_SHARE::visit_subgraph(Wait_for_flush *wait_for_flush, > because we won't try to acquire tdc.LOCK_table_share while > holding a write-lock on MDL_lock::m_rwlock. > */ > - mysql_mutex_lock(&tdc.LOCK_table_share); > - tdc.all_tables_refs++; > - mysql_mutex_unlock(&tdc.LOCK_table_share); > + mysql_mutex_lock(&tdc->LOCK_table_share); > + tdc->all_tables_refs++; > + mysql_mutex_unlock(&tdc->LOCK_table_share); Do I understand it correctly that a TDC_element can never be deleted from LF_HASH as long as at least one TABLE_SHARE references it? > > - All_share_tables_list::Iterator tables_it(tdc.all_tables); > + TDC_element::All_share_tables_list::Iterator tables_it(tdc->all_tables); > > /* > In case of multiple searches running in parallel, avoid going > @@ -3946,21 +3945,10 @@ bool TABLE_SHARE::wait_for_old_version(THD *thd, struct timespec *abstime, > > mdl_context->done_waiting_for(); > > - mysql_mutex_lock(&tdc.LOCK_table_share); > - > - tdc.m_flush_tickets.remove(&ticket); > - > - if (tdc.m_flush_tickets.is_empty() && tdc.ref_count == 0) > - { > - /* > - If our thread was the last one using the share, > - we must destroy it here. > - */ > - mysql_mutex_unlock(&tdc.LOCK_table_share); > - destroy(); > - } > - else > - mysql_mutex_unlock(&tdc.LOCK_table_share); > + mysql_mutex_lock(&tdc->LOCK_table_share); > + tdc->m_flush_tickets.remove(&ticket); > + mysql_cond_broadcast(&tdc->COND_release); > + mysql_mutex_unlock(&tdc->LOCK_table_share); Why is the logic here different? > > > /* > diff --git a/sql/table_cache.h b/sql/table_cache.h > --- a/sql/table_cache.h > +++ b/sql/table_cache.h > @@ -16,6 +16,171 @@ > Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ > > > +extern PSI_mutex_key key_TABLE_SHARE_LOCK_table_share; > +extern PSI_cond_key key_TABLE_SHARE_COND_release; > + > +class TDC_element > +{ > +public: > + uchar m_key[NAME_LEN + 1 + NAME_LEN + 1]; > + uint m_key_length; > + ulong version; > + bool flushed; > + TABLE_SHARE *share; > + > + typedef I_P_List <TABLE, TABLE_share> TABLE_list; > + typedef I_P_List <TABLE, All_share_tables> All_share_tables_list; > + /** > + Protects ref_count, m_flush_tickets, all_tables, free_tables, flushed, > + all_tables_refs. > + */ > + mysql_mutex_t LOCK_table_share; > + mysql_cond_t COND_release; > + TDC_element *next, **prev; /* Link to unused shares */ > + uint ref_count; /* How many TABLE objects uses this */ > + uint all_tables_refs; /* Number of refs to all_tables */ > + /** > + List of tickets representing threads waiting for the share to be flushed. > + */ > + Wait_for_flush_list m_flush_tickets; > + /* > + Doubly-linked (back-linked) lists of used and unused TABLE objects > + for this share. > + */ > + All_share_tables_list all_tables; > + TABLE_list free_tables; > + > + TDC_element() {} > + > + TDC_element(const char *key, uint key_length) : m_key_length(key_length) > + { > + memcpy(m_key, key, key_length); > + } > + > + > + void dbug_release_assertions() Somewhat confusing name. If I hadn't see the code below I'd thought this function releases some assertions (how? why?) may be assert_correctness_at_release() would be less ambiguous? > + { > + DBUG_ASSERT(share == 0); > + DBUG_ASSERT(ref_count == 0); > + DBUG_ASSERT(m_flush_tickets.is_empty()); > + DBUG_ASSERT(all_tables.is_empty()); > + DBUG_ASSERT(free_tables.is_empty()); > + DBUG_ASSERT(all_tables_refs == 0); > + DBUG_ASSERT(next == 0); > + DBUG_ASSERT(prev == 0); > + } > + > + > + /** > + Acquire TABLE object from table cache. > + > + @pre share must be protected against removal. > + > + Acquired object cannot be evicted or acquired again. > + > + While locked: > + - pop object from TABLE_SHARE::tdc.free_tables > + > + While unlocked: > + - mark object used by thd "While locked" / "While unlocked" - what does that mean? > + > + @return TABLE object, or NULL if no unused objects. > + */ > + > + TABLE *acquire_table(THD *thd) > + { > + TABLE *table; > + > + mysql_mutex_lock(&LOCK_table_share); > + table= free_tables.pop_front(); > + if (table) > + { > + DBUG_ASSERT(!table->in_use); lots of asserts - good! > + table->in_use= thd; > + /* The ex-unused table must be fully functional. */ > + DBUG_ASSERT(table->db_stat && table->file); > + /* The children must be detached from the table. */ > + DBUG_ASSERT(!table->file->extra(HA_EXTRA_IS_ATTACHED_CHILDREN)); I don't even think of asking why it's HA_EXTRA_IS_ATTACHED_CHILDREN and not HA_EXTRA_HAS_ATTACHED_CHILDREN or, at least, HA_EXTRA_CHILDREN_ARE_ATTACHED. Ah, whatever... Horrors of the English language hidden in old MySQL code :) > + } > + mysql_mutex_unlock(&LOCK_table_share); > + return table; > + } > + > + > + /** > + Get last element of free_tables. > + */ > + > + TABLE *free_tables_back() Eh, I've thought for a few seconds what could it mean to "free tables back" until I read the function comment. You see, "free" is so often used as a verb that one absolutely doesn't see it that here it means "free_tables' back" even though it's also a common naming pattern (xxx_back for the list xxx). Rename the method, may be? > + { > + TABLE_list::Iterator it(share->tdc->free_tables); > + TABLE *entry, *last= 0; > + while ((entry= it++)) > + last= entry; > + return last; > + } > + > + > + /** > + Wait for MDL deadlock detector to complete traversing tdc.all_tables. > + > + Must be called before updating TABLE_SHARE::tdc.all_tables. > + */ > + > + void wait_for_mdl_deadlock_detector() > + { > + while (all_tables_refs) > + mysql_cond_wait(&COND_release, &LOCK_table_share); > + } > + > + > + /** > + Prepeare table share for use with table definition cache. > + */ > + > + static void lf_alloc_constructor(uchar *arg) > + { > + TDC_element *element= (TDC_element*) (arg + LF_HASH_OVERHEAD); > + DBUG_ENTER("tdc_init_share"); intentionally? or forgot to update DBUG_ENTER? > + mysql_mutex_init(key_TABLE_SHARE_LOCK_table_share, > + &element->LOCK_table_share, MY_MUTEX_INIT_FAST); > + mysql_cond_init(key_TABLE_SHARE_COND_release, &element->COND_release, 0); > + element->m_flush_tickets.empty(); > + element->all_tables.empty(); > + element->free_tables.empty(); > + element->all_tables_refs= 0; > + element->share= 0; > + element->ref_count= 0; > + element->next= 0; > + element->prev= 0; > + DBUG_VOID_RETURN; > + } > + > + > + /** > + Release table definition cache specific resources of table share. > + */ > + > + static void lf_alloc_destructor(uchar *arg) > + { > + TDC_element *element= (TDC_element*) (arg + LF_HASH_OVERHEAD); > + DBUG_ENTER("tdc_deinit_share"); intentionally? or forgot to update DBUG_ENTER? > + element->dbug_release_assertions(); > + mysql_cond_destroy(&element->COND_release); > + mysql_mutex_destroy(&element->LOCK_table_share); > + DBUG_VOID_RETURN; > + } > + > + > + static uchar *key(const TDC_element *element, size_t *length, > + my_bool not_used __attribute__((unused))) > + { > + *length= element->m_key_length; > + return (uchar*) element->m_key; > + } > +}; > + > + > enum enum_tdc_remove_table_type > { > TDC_RT_REMOVE_ALL, > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -270,58 +270,75 @@ uint get_table_def_key(const TABLE_LIST *table_list, const char **key) > # Pointer to list of names of open tables. > */ > > -OPEN_TABLE_LIST *list_open_tables(THD *thd, const char *db, const char *wild) > +struct list_open_tables_arg > { > + THD *thd; > + const char *db; > + const char *wild; > TABLE_LIST table_list; > OPEN_TABLE_LIST **start_list, *open_list; > - TABLE_SHARE *share; > - TDC_iterator tdc_it; > - DBUG_ENTER("list_open_tables"); > +}; > > - bzero((char*) &table_list,sizeof(table_list)); > - start_list= &open_list; > - open_list=0; > > - tdc_it.init(); > - while ((share= tdc_it.next())) > +static my_bool list_open_tables_callback(TDC_element *element, > + list_open_tables_arg *arg) > { > - if (db && my_strcasecmp(system_charset_info, db, share->db.str)) > - continue; > - if (wild && wild_compare(share->table_name.str, wild, 0)) > - continue; > + char *db= (char*) element->m_key; > + char *table_name= (char*) element->m_key + strlen((char*) element->m_key) + 1; > > + if (arg->db && my_strcasecmp(system_charset_info, arg->db, db)) > + return FALSE; > + if (arg->wild && wild_compare(table_name, arg->wild, 0)) > + return FALSE; > + > /* Check if user has SELECT privilege for any column in the table */ > - table_list.db= share->db.str; > - table_list.table_name= share->table_name.str; > - table_list.grant.privilege=0; > + arg->table_list.db= db; > + arg->table_list.table_name= table_name; > + arg->table_list.grant.privilege= 0; > > - if (check_table_access(thd,SELECT_ACL,&table_list, TRUE, 1, TRUE)) > - continue; > + if (check_table_access(arg->thd, SELECT_ACL, &arg->table_list, TRUE, 1, TRUE)) > + return FALSE; > > - if (!(*start_list = (OPEN_TABLE_LIST *) > - sql_alloc(sizeof(**start_list)+share->table_cache_key.length))) > - { > - open_list=0; // Out of memory > - break; > - } > + if (!(*arg->start_list= (OPEN_TABLE_LIST *) sql_alloc( when you know the thd, prefer thd->alloc() over sql_alloc() (which is, basically, current_thd->alloc()) > + sizeof(**arg->start_list) + element->m_key_length))) > + return TRUE; > + > - strmov((*start_list)->table= > - strmov(((*start_list)->db= (char*) ((*start_list)+1)), > - share->db.str)+1, > - share->table_name.str); > + strmov((*arg->start_list)->table= > + strmov(((*arg->start_list)->db= (char*) ((*arg->start_list) + 1)), > + db) + 1, table_name); > - (*start_list)->in_use= 0; > + (*arg->start_list)->in_use= 0; > - mysql_mutex_lock(&share->tdc.LOCK_table_share); > - TABLE_SHARE::All_share_tables_list::Iterator it(share->tdc.all_tables); > + > + mysql_mutex_lock(&element->LOCK_table_share); > + TDC_element::All_share_tables_list::Iterator it(element->all_tables); > TABLE *table; > while ((table= it++)) > if (table->in_use) > - ++(*start_list)->in_use; > + ++(*arg->start_list)->in_use; > - mysql_mutex_unlock(&share->tdc.LOCK_table_share); > + mysql_mutex_unlock(&element->LOCK_table_share); > - (*start_list)->locked= 0; /* Obsolete. */ > + (*arg->start_list)->locked= 0; /* Obsolete. */ > - start_list= &(*start_list)->next; > + arg->start_list= &(*arg->start_list)->next; > - *start_list=0; > + *arg->start_list= 0; > + return FALSE; > } > - tdc_it.deinit(); > - DBUG_RETURN(open_list); > + > + > +OPEN_TABLE_LIST *list_open_tables(THD *thd, const char *db, const char *wild) > +{ > + list_open_tables_arg argument; > + DBUG_ENTER("list_open_tables"); > + > + argument.thd= thd; > + argument.db= db; > + argument.wild= wild; > + bzero((char*) &argument.table_list, sizeof(argument.table_list)); > + argument.start_list= &argument.open_list; > + argument.open_list= 0; > + > + if (tdc_iterate(thd, (my_hash_walk_action) list_open_tables_callback, > + &argument, true)) > + DBUG_RETURN(0); > + > + DBUG_RETURN(argument.open_list); > } > > /***************************************************************************** > diff --git a/sql/table_cache.cc b/sql/table_cache.cc > --- a/sql/table_cache.cc > +++ b/sql/table_cache.cc > @@ -72,32 +76,23 @@ > /** > Protects unused shares list. > > - TABLE_SHARE::tdc.prev > - TABLE_SHARE::tdc.next > - oldest_unused_share > - end_of_unused_share > + TDC_element::prev > + TDC_element::next > + unused_shares > */ > > static mysql_mutex_t LOCK_unused_shares; > -static mysql_rwlock_t LOCK_tdc; /**< Protects tdc_hash. */ > my_atomic_rwlock_t LOCK_tdc_atomics; /**< Protects tdc_version. */ > > #ifdef HAVE_PSI_INTERFACE > -static PSI_mutex_key key_LOCK_unused_shares, key_TABLE_SHARE_LOCK_table_share; > +PSI_mutex_key key_LOCK_unused_shares, key_TABLE_SHARE_LOCK_table_share; really? not static? > static PSI_mutex_info all_tc_mutexes[]= > { > { &key_LOCK_unused_shares, "LOCK_unused_shares", PSI_FLAG_GLOBAL }, > { &key_TABLE_SHARE_LOCK_table_share, "TABLE_SHARE::tdc.LOCK_table_share", 0 } > }; > > -static PSI_rwlock_key key_rwlock_LOCK_tdc; > -static PSI_rwlock_info all_tc_rwlocks[]= > -{ > - { &key_rwlock_LOCK_tdc, "LOCK_tdc", PSI_FLAG_GLOBAL } > -}; > - > - > -static PSI_cond_key key_TABLE_SHARE_COND_release; > +PSI_cond_key key_TABLE_SHARE_COND_release; and here > static PSI_cond_info all_tc_conds[]= > { > { &key_TABLE_SHARE_COND_release, "TABLE_SHARE::tdc.COND_release", 0 } > @@ -265,85 +269,41 @@ void tc_add_table(THD *thd, TABLE *table) > > if (need_purge) > { > - TABLE_SHARE *purge_share= 0; > - TABLE_SHARE *share; > - TABLE *entry; > - ulonglong UNINIT_VAR(purge_time); > - TDC_iterator tdc_it; > + tc_add_table_arg argument; > + argument.purge_time= ULONGLONG_MAX; > + tdc_iterate(thd, (my_hash_walk_action) tc_add_table_callback, &argument); > > - tdc_it.init(); > - while ((share= tdc_it.next())) > + if (argument.purge_time != ULONGLONG_MAX) > { > - mysql_mutex_lock(&share->tdc.LOCK_table_share); > - if ((entry= tc_free_tables_back(share)) && > - (!purge_share || entry->tc_time < purge_time)) > + TDC_element *element= (TDC_element*) lf_hash_search(&tdc_hash, > + thd->tdc_hash_pins, > + argument.key, > + argument.key_length); > + if (element) > { > - purge_share= share; > - purge_time= entry->tc_time; > - } > - mysql_mutex_unlock(&share->tdc.LOCK_table_share); > - } > + TABLE *entry; > + mysql_mutex_lock(&element->LOCK_table_share); > + lf_hash_search_unpin(thd->tdc_hash_pins); > + element->wait_for_mdl_deadlock_detector(); you wait under a mutex? hmm... > > - if (purge_share) > - { > - mysql_mutex_lock(&purge_share->tdc.LOCK_table_share); > - tc_wait_for_mdl_deadlock_detector(purge_share); > - tdc_it.deinit(); > /* > It may happen that oldest table was acquired meanwhile. In this case > just go ahead, number of objects in table cache will normalize > eventually. > */ > - if ((entry= tc_free_tables_back(purge_share)) && > - entry->tc_time == purge_time) > + if ((entry= element->free_tables_back()) && > + entry->tc_time == argument.purge_time) > { > - entry->s->tdc.free_tables.remove(entry); > + element->free_tables.remove(entry); > tc_remove_table(entry); > - mysql_mutex_unlock(&purge_share->tdc.LOCK_table_share); > + mysql_mutex_unlock(&element->LOCK_table_share); > intern_close_table(entry); > } > else > - mysql_mutex_unlock(&purge_share->tdc.LOCK_table_share); > + mysql_mutex_unlock(&element->LOCK_table_share); > } > - else > - tdc_it.deinit(); > } > } > - > - > -/** > - Acquire TABLE object from table cache. > - > - @pre share must be protected against removal. > - > - Acquired object cannot be evicted or acquired again. > - > - While locked: > - - pop object from TABLE_SHARE::tdc.free_tables > - > - While unlocked: > - - mark object used by thd > - > - @return TABLE object, or NULL if no unused objects. > -*/ > - > -static TABLE *tc_acquire_table(THD *thd, TABLE_SHARE *share) > -{ > - TABLE *table; > - > - mysql_mutex_lock(&share->tdc.LOCK_table_share); > - table= share->tdc.free_tables.pop_front(); > - if (table) > - { > - DBUG_ASSERT(!table->in_use); > - table->in_use= thd; > - /* The ex-unused table must be fully functional. */ > - DBUG_ASSERT(table->db_stat && table->file); > - /* The children must be detached from the table. */ > - DBUG_ASSERT(!table->file->extra(HA_EXTRA_IS_ATTACHED_CHILDREN)); > - } ok, you've moved it to table_cache.h... > - mysql_mutex_unlock(&share->tdc.LOCK_table_share); > - return table; > } > > > @@ -397,88 +357,76 @@ bool tc_release_table(TABLE *table) > */ > table->in_use= 0; > /* Add table to the list of unused TABLE objects for this share. */ > - table->s->tdc.free_tables.push_front(table); > - mysql_mutex_unlock(&table->s->tdc.LOCK_table_share); > + table->s->tdc->free_tables.push_front(table); > + mysql_mutex_unlock(&table->s->tdc->LOCK_table_share); > return false; > > purge: > - tc_wait_for_mdl_deadlock_detector(table->s); > + table->s->tdc->wait_for_mdl_deadlock_detector(); > tc_remove_table(table); > - mysql_mutex_unlock(&table->s->tdc.LOCK_table_share); > + mysql_mutex_unlock(&table->s->tdc->LOCK_table_share); > table->in_use= 0; > intern_close_table(table); > return true; > } > > > -extern "C" uchar *tdc_key(const uchar *record, size_t *length, > - my_bool not_used __attribute__((unused))) > -{ > - TABLE_SHARE *entry= (TABLE_SHARE*) record; > - *length= entry->table_cache_key.length; > - return (uchar*) entry->table_cache_key.str; > -} > - > - > /** > Delete share from hash and free share object. > - > - @return > - @retval 0 Success > - @retval 1 Share is referenced > */ > > -static int tdc_delete_share_from_hash(TABLE_SHARE *share) > +static void tdc_delete_share_from_hash(TDC_element *element) > { > + THD *thd= current_thd; > + LF_PINS *pins; > + TABLE_SHARE *share; > DBUG_ENTER("tdc_delete_share_from_hash"); > - mysql_rwlock_wrlock(&LOCK_tdc); > - mysql_mutex_lock(&share->tdc.LOCK_table_share); > - if (--share->tdc.ref_count) > - { > - mysql_cond_broadcast(&share->tdc.COND_release); > - mysql_mutex_unlock(&share->tdc.LOCK_table_share); > - mysql_rwlock_unlock(&LOCK_tdc); > - DBUG_RETURN(1); > - } > - my_hash_delete(&tdc_hash, (uchar*) share); > + > + share= element->share; > + DBUG_ASSERT(share); > + element->share= 0; > /* Notify PFS early, while still locked. */ "while still locked" what? > PSI_CALL_release_table_share(share->m_psi); > share->m_psi= 0; > - mysql_rwlock_unlock(&LOCK_tdc); > > - if (share->tdc.m_flush_tickets.is_empty()) > + if (!element->m_flush_tickets.is_empty()) > { > - /* No threads are waiting for this share to be flushed, destroy it. */ > - mysql_mutex_unlock(&share->tdc.LOCK_table_share); > - free_table_share(share); > - } > - else > - { > - Wait_for_flush_list::Iterator it(share->tdc.m_flush_tickets); > + Wait_for_flush_list::Iterator it(element->m_flush_tickets); > Wait_for_flush *ticket; > while ((ticket= it++)) > (void) ticket->get_ctx()->m_wait.set_status(MDL_wait::GRANTED); > - /* > - If there are threads waiting for this share to be flushed, > - the last one to receive the notification will destroy the > - share. At this point the share is removed from the table > - definition cache, so is OK to proceed here without waiting > - for this thread to do the work. > - */ > - mysql_mutex_unlock(&share->tdc.LOCK_table_share); > + > + do > + { > + mysql_cond_wait(&element->COND_release, &element->LOCK_table_share); > + } while (!element->m_flush_tickets.is_empty()); > } > - DBUG_RETURN(0); > + > + mysql_mutex_unlock(&element->LOCK_table_share); Ok, so the caller must lock LOCK_table_share? please put mysql_mutex_assert_owner() in the beginning of the function > + > + if (thd) When thd is NULL here? On shutdown? > + { > + fix_thd_pins(thd); > + pins= thd->tdc_hash_pins; > } > + else > + pins= lf_hash_get_pins(&tdc_hash); > > + DBUG_ASSERT(pins); // What can we do about it? > + element->dbug_release_assertions(); > + lf_hash_delete(&tdc_hash, pins, element->m_key, element->m_key_length); > + if (!thd) > + lf_hash_put_pins(pins); > + free_table_share(share); > + DBUG_VOID_RETURN; > +} > > + > /** > Initialize table definition cache. > - > - @retval 0 Success > - @retval !0 Error > */ > > -int tdc_init(void) > +void tdc_init(void) > { > DBUG_ENTER("tdc_init"); > #ifdef HAVE_PSI_INTERFACE > @@ -642,26 +548,34 @@ void tdc_deinit_share(TABLE_SHARE *share) > Caller is expected to unlock table share with tdc_unlock_share(). > > @retval 0 Share not found > - @retval !0 Pointer to locked table share > + @retval MY_ERRPTR OOM > + @retval ptr Pointer to locked table share > */ > > -TABLE_SHARE *tdc_lock_share(const char *db, const char *table_name) > +TDC_element *tdc_lock_share(THD *thd, const char *db, const char *table_name) > { > + TDC_element *element; > char key[MAX_DBKEY_LENGTH]; > - uint key_length; > > DBUG_ENTER("tdc_lock_share"); > - key_length= tdc_create_key(key, db, table_name); > + if (fix_thd_pins(thd)) > + DBUG_RETURN((TDC_element*) MY_ERRPTR); I suppose you can do it in THD:init, if you'd like. A THD almost always will need to access table definition cache, so it'll need these pins. Then there will be no need to return MY_ERRPTR here and handle it in the caller > > - mysql_rwlock_rdlock(&LOCK_tdc); > - TABLE_SHARE* share= (TABLE_SHARE*) my_hash_search(&tdc_hash, > - (uchar*) key, key_length); > - if (share && !share->error) > - mysql_mutex_lock(&share->tdc.LOCK_table_share); > - else > - share= 0; > - mysql_rwlock_unlock(&LOCK_tdc); > - DBUG_RETURN(share); > + element= (TDC_element *) lf_hash_search(&tdc_hash, thd->tdc_hash_pins, > + (uchar*) key, > + tdc_create_key(key, db, table_name)); > + if (element) > + { > + mysql_mutex_lock(&element->LOCK_table_share); > + if (!element->share || element->share->error) > + { > + mysql_mutex_unlock(&element->LOCK_table_share); > + element= 0; > + } > + lf_hash_search_unpin(thd->tdc_hash_pins); > + } > + > + DBUG_RETURN(element); Looks good to me > } > > > @@ -702,60 +616,60 @@ TABLE_SHARE *tdc_acquire_share(THD *thd, const char *db, const char *table_name, > TABLE **out_table) > { > TABLE_SHARE *share; > + TDC_element *element; > bool was_unused; > DBUG_ENTER("tdc_acquire_share"); > > - mysql_rwlock_rdlock(&LOCK_tdc); > - share= (TABLE_SHARE*) my_hash_search_using_hash_value(&tdc_hash, hash_value, > - (uchar*) key, > - key_length); > - if (!share) > + if (fix_thd_pins(thd)) > + DBUG_RETURN(0); > + > +retry: > + while (!(element= (TDC_element*) lf_hash_search_using_hash_value(&tdc_hash, Was lf_hash_search_using_hash_value added in a separate patch? > + thd->tdc_hash_pins, hash_value, (uchar*) key, key_length))) > { > - TABLE_SHARE *new_share; > - mysql_rwlock_unlock(&LOCK_tdc); > + TDC_element tmp(key, key_length); > + int res= lf_hash_insert(&tdc_hash, thd->tdc_hash_pins, (uchar*) &tmp); > > - if (!(new_share= alloc_table_share(db, table_name, key, key_length))) > + if (res == -1) > DBUG_RETURN(0); > - new_share->error= OPEN_FRM_OPEN_ERROR; > + else if (res == 1) > + continue; > > - mysql_rwlock_wrlock(&LOCK_tdc); > - share= (TABLE_SHARE*) my_hash_search_using_hash_value(&tdc_hash, hash_value, > - (uchar*) key, > - key_length); > - if (!share) > - { > - bool need_purge; > + element= (TDC_element*) lf_hash_search_using_hash_value(&tdc_hash, > + thd->tdc_hash_pins, hash_value, (uchar*) key, key_length); > + lf_hash_search_unpin(thd->tdc_hash_pins); How can you be sure that nobody frees your element after you unpin it? > + DBUG_ASSERT(element); > + element->dbug_release_assertions(); > > - share= new_share; > - mysql_mutex_lock(&share->tdc.LOCK_table_share); > - if (my_hash_insert(&tdc_hash, (uchar*) share)) > + if (!(share= alloc_table_share(db, table_name, key, key_length))) > { > - mysql_mutex_unlock(&share->tdc.LOCK_table_share); > - mysql_rwlock_unlock(&LOCK_tdc); > - free_table_share(share); > + lf_hash_delete(&tdc_hash, thd->tdc_hash_pins, key, key_length); > DBUG_RETURN(0); > } > - need_purge= tdc_hash.records > tdc_size; > - mysql_rwlock_unlock(&LOCK_tdc); > > /* note that tdc_acquire_share() *always* uses discovery */ > open_table_def(thd, share, flags | GTS_USE_DISCOVERY); > - share->tdc.ref_count++; > - mysql_mutex_unlock(&share->tdc.LOCK_table_share); > > if (share->error) > { > - tdc_delete_share_from_hash(share); > + free_table_share(share); > + lf_hash_delete(&tdc_hash, thd->tdc_hash_pins, key, key_length); > DBUG_RETURN(0); > } > - else if (need_purge) > + > + mysql_mutex_lock(&element->LOCK_table_share); > + element->share= share; > + share->tdc= element; > + element->ref_count++; > + element->version= tdc_refresh_version(); > + element->flushed= false; > + mysql_mutex_unlock(&element->LOCK_table_share); > + > tdc_purge(false); > if (out_table) > *out_table= 0; > share->m_psi= PSI_CALL_get_table_share(false, share); > goto end; > - } > - free_table_share(new_share); > } > > /* cannot force discovery of a cached share */ > @@ -763,18 +677,25 @@ TABLE_SHARE *tdc_acquire_share(THD *thd, const char *db, const char *table_name, > > if (out_table && (flags & GTS_TABLE)) > { > - if ((*out_table= tc_acquire_table(thd, share))) > + if ((*out_table= element->acquire_table(thd))) > { > - mysql_rwlock_unlock(&LOCK_tdc); > + lf_hash_search_unpin(thd->tdc_hash_pins); what this lf_hash_search_unpin corresponds to? > DBUG_ASSERT(!(flags & GTS_NOLOCK)); > - DBUG_ASSERT(!share->error); > - DBUG_ASSERT(!share->is_view); > - DBUG_RETURN(share); > + DBUG_ASSERT(element->share); > + DBUG_ASSERT(!element->share->error); > + DBUG_ASSERT(!element->share->is_view); > + DBUG_RETURN(element->share); > } > } > > - mysql_mutex_lock(&share->tdc.LOCK_table_share); > - mysql_rwlock_unlock(&LOCK_tdc); > + mysql_mutex_lock(&element->LOCK_table_share); > + if (!(share= element->share)) > + { > + mysql_mutex_unlock(&element->LOCK_table_share); > + lf_hash_search_unpin(thd->tdc_hash_pins); and here > + goto retry; > + } > + lf_hash_search_unpin(thd->tdc_hash_pins); may be that first lf_hash_search_unpin was wrong? the one that goes directly after lf_hash_search. > > /* > We found an existing table definition. Return it if we didn't get > @@ -983,13 +864,36 @@ bool tdc_remove_table(THD *thd, enum_tdc_remove_table_type remove_type, > thd->mdl_context.is_lock_owner(MDL_key::TABLE, db, table_name, > MDL_EXCLUSIVE)); > > - if ((share= tdc_delete_share(db, table_name))) > + > + mysql_mutex_lock(&LOCK_unused_shares); > + if (!(element= tdc_lock_share(thd, db, table_name))) > { > - I_P_List <TABLE, TABLE_share> purge_tables; > - uint my_refs= 1; > + mysql_mutex_unlock(&LOCK_unused_shares); > + DBUG_ASSERT(remove_type != TDC_RT_REMOVE_NOT_OWN_KEEP_SHARE); > + DBUG_RETURN(false); > + } > > - mysql_mutex_lock(&share->tdc.LOCK_table_share); > - tc_wait_for_mdl_deadlock_detector(share); > + DBUG_ASSERT(element != MY_ERRPTR); // What can we do about it? allocate thd->tdc_hash_pins in THD::init > + > + if (!element->ref_count) > + { > + if (element->prev) > + { > + unused_shares.remove(element); > + /* Concurrent thread may start using share again, reset prev and next. */ What concurrent thread? element->ref_count is 0, you removed the the share from the unused_shares list. There can be no concurrent threads, as far as I understand. And indeed, you do remove the share from the hash (which does free_table_share) immediately below, which also proves that there can be no concurrent threads. > + element->prev= 0; > + element->next= 0; > + } > + mysql_mutex_unlock(&LOCK_unused_shares); > + > + tdc_delete_share_from_hash(element); > + DBUG_RETURN(true); > + } > + mysql_mutex_unlock(&LOCK_unused_shares); > + > + element->ref_count++; > + > + element->wait_for_mdl_deadlock_detector(); > /* > Mark share flushed in order to ensure that it gets > automatically deleted once it is no longer referenced. Regards, Sergei

2 4

[Maria-developers] ccache with mariadb?
by Otto Kekäläinen 28 Dec '14

28 Dec '14

Hello! Has somebody managed to configure ccache correctly so that it would work with MariaDB compilation? https://ccache.samba.org/ I don't seem to get it working as the build always seems to pick up the real gcc binary instead of any of the ccache symlink wrappers I've manually put in place to replace all of gcc, cc1, cc1plus etc.

2 3

Re: [Maria-developers] MDEV-7273 - 10.1 fails to start up during tc_log initializations on PPC64
by Sergei Golubchik 26 Dec '14

26 Dec '14

Hi, Sergey! On Dec 26, svoj(a)mariadb.org wrote: > revision-id: f65901eef28728212a350f61569dc884c747bffc > parent(s): 8c616cd347aa7b3fcc2d8defb5e172ad316f432c > committer: Sergey Vojtovich > branch nick: 10.1 > timestamp: 2014-12-26 23:38:45 +0400 > message: > > MDEV-7273 - 10.1 fails to start up during tc_log initializations on PPC64 > > log-tc-size is 24K by default. Page size is 64K on PPC64. But log-tc-size > must be at least 3 x page size. This is enforced by TC_LOG_MMAP::open() > with a comment: to guarantee non-empty pool. > > This all makes server not startable in default configuration on PPC64. > > Autosize log-tc-size, so that it's min value= page size * 3, default > value= page size * 6, block size= page size. > > diff --git a/sql/log.cc b/sql/log.cc > index 66e1426..52c85d6 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -8780,6 +8779,7 @@ int TC_LOG_MMAP::sync() > int TC_LOG_MMAP::unlog(ulong cookie, my_xid xid) > { > pending_cookies *full_buffer= NULL; > + uint32 ncookies= tc_log_page_size / sizeof(my_xid); > DBUG_ASSERT(*(my_xid *)(data+cookie) == xid); > > /* > @@ -8793,7 +8793,7 @@ int TC_LOG_MMAP::unlog(ulong cookie, my_xid xid) > mysql_mutex_lock(&LOCK_pending_checkpoint); > if (pending_checkpoint == NULL) > { > - uint32 size= sizeof(*pending_checkpoint); > + uint32 size= sizeof(*pending_checkpoint) + sizeof(ulong) * (ncookies - 1); > if (!(pending_checkpoint= > (pending_cookies *)my_malloc(size, MYF(MY_ZEROFILL)))) > { > @@ -8804,8 +8804,7 @@ int TC_LOG_MMAP::unlog(ulong cookie, my_xid xid) > } > > pending_checkpoint->cookies[pending_checkpoint->count++]= cookie; > - if (pending_checkpoint->count == sizeof(pending_checkpoint->cookies) / > - sizeof(pending_checkpoint->cookies[0])) > + if (pending_checkpoint->count == ncookies) > { > full_buffer= pending_checkpoint; > pending_checkpoint= NULL; > @@ -8839,7 +8838,7 @@ int TC_LOG_MMAP::unlog(ulong cookie, my_xid xid) > if (count == 0) > { > uint i; > - for (i= 0; i < sizeof(pending->cookies)/sizeof(pending->cookies[0]); ++i) > + for (i= 0; i < tc_log_page_size / sizeof(my_xid); ++i) I wonder why you didn't use ncookies here :) > delete_entry(pending->cookies[i]); > my_free(pending); > } Regards, Sergei

2 1

Re: [Maria-developers] MDEV-7273 - 10.1 fails to start up during tc_log initializations on PPC64
by Sergei Golubchik 26 Dec '14

26 Dec '14

Hi, Sergey! Yes, that's what I had in mind, thanks! There are few rough edges, see below. Ok to push after you fix that, there's no need for another review round. On Dec 26, svoj(a)mariadb.org wrote: > revision-id: 72e58dc2b1a0bb859e918de0ff820d5a2a92dc32 > parent(s): db89dd3a8f7b0d868946d25ba98c6f88612d309a > committer: Sergey Vojtovich > branch nick: 10.1 > timestamp: 2014-12-26 17:03:45 +0400 > message: > > MDEV-7273 - 10.1 fails to start up during tc_log initializations on PPC64 > > log-tc-size is 24K by default. Page size is 64K on PPC64. But log-tc-size > must be at least 3 x page size. This is enforced by TC_LOG_MMAP::open() > with a comment: to guarantee non-empty pool. > > This all makes server not startable in default configuration on PPC64. > > Autosize log-tc-size, so that it's min value= page size * 6, default > value= page size * 6, block size= page size. > > --- > mysql-test/r/mysqld--help.result | 1 - > mysql-test/suite/sys_vars/r/log_tc_size_basic.result | 4 ++++ > .../suite/sys_vars/r/sysvars_server_notembedded.result | 14 ++++++++++++++ > mysql-test/suite/sys_vars/t/log_tc_size_basic.test | 5 +++++ > mysql-test/t/mysqld--help.test | 2 +- > sql/log.cc | 5 ++--- > sql/log.h | 3 +-- > sql/mysqld.cc | 7 +------ > sql/sys_vars.cc | 11 +++++++++++ > 9 files changed, 39 insertions(+), 13 deletions(-) > > diff --git a/mysql-test/suite/sys_vars/t/log_tc_size_basic.test b/mysql-test/suite/sys_vars/t/log_tc_size_basic.test > new file mode 100644 > index 0000000..1fce212 > --- /dev/null > +++ b/mysql-test/suite/sys_vars/t/log_tc_size_basic.test > @@ -0,0 +1,5 @@ > +--error ER_INCORRECT_GLOBAL_LOCAL_VAR,ER_UNKNOWN_SYSTEM_VARIABLE > +SET GLOBAL log_tc_size=1; > + > +--error ER_INCORRECT_GLOBAL_LOCAL_VAR,ER_UNKNOWN_SYSTEM_VARIABLE do we ever build on platforms without mmap? if the answer is "no" - you don't need ER_UNKNOWN_SYSTEM_VARIABLE here > +SET SESSION log_tc_size=1; > 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 006b9a9..5491169 100644 > --- a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > +++ b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > @@ -1757,6 +1757,20 @@ NUMERIC_BLOCK_SIZE NULL > ENUM_VALUE_LIST innodb,query_plan,explain > READ_ONLY NO > COMMAND_LINE_ARGUMENT REQUIRED > +VARIABLE_NAME LOG_TC_SIZE > +SESSION_VALUE NULL > +GLOBAL_VALUE 24576 > +GLOBAL_VALUE_ORIGIN AUTO > +DEFAULT_VALUE 24576 > +VARIABLE_SCOPE GLOBAL > +VARIABLE_TYPE BIGINT UNSIGNED > +VARIABLE_COMMENT Size of transaction coordinator log. > +NUMERIC_MIN_VALUE 24576 > +NUMERIC_MAX_VALUE 18446744073709551615 > +NUMERIC_BLOCK_SIZE 4096 > +ENUM_VALUE_LIST NULL > +READ_ONLY YES > +COMMAND_LINE_ARGUMENT REQUIRED if the answer is "yes" - this test will fail there. even if the answer is "no", the variable value is architecture dependent. But it's good to see all stable variable metadata (e.g. that GLOBAL_VALUE_ORIGIN is AUTO). > VARIABLE_NAME LOG_WARNINGS > SESSION_VALUE 1 > GLOBAL_VALUE 1 > diff --git a/sql/log.cc b/sql/log.cc > index 66e1426..6d6512e 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -8793,7 +8792,7 @@ int TC_LOG_MMAP::unlog(ulong cookie, my_xid xid) > mysql_mutex_lock(&LOCK_pending_checkpoint); > if (pending_checkpoint == NULL) > { > - uint32 size= sizeof(*pending_checkpoint); > + uint32 size= sizeof(*pending_checkpoint) + tc_log_page_size / sizeof(my_xid); that's not enough. There are further sizeof's, like if (pending_checkpoint->count == sizeof(pending_checkpoint->cookies) / sizeof(pending_checkpoint->cookies[0])) and for (i= 0; i < sizeof(pending->cookies)/sizeof(pending->cookies[0]); ++i) I've found these two by looking for "sizeof" in the Kristian's patch that introduced pending_checkpoint. There can be more sizeofs added later. > if (!(pending_checkpoint= > (pending_cookies *)my_malloc(size, MYF(MY_ZEROFILL)))) > { > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index fb11377..bfaf7d7 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -5202,3 +5202,14 @@ static Sys_var_mybool Sys_strict_password_validation( > "that cannot be validated (passwords specified as a hash)", > GLOBAL_VAR(strict_password_validation), > CMD_LINE(OPT_ARG), DEFAULT(TRUE), NO_MUTEX_GUARD, NOT_IN_BINLOG); > + > +#ifdef HAVE_MMAP > +static Sys_var_ulong Sys_log_tc_size( > + "log_tc_size", > + "Size of transaction coordinator log.", > + READ_ONLY GLOBAL_VAR(opt_tc_log_size), > + CMD_LINE(REQUIRED_ARG), > + VALID_RANGE(my_getpagesize() * 6, ULONG_MAX), > + DEFAULT(my_getpagesize() * 6), > + BLOCK_SIZE(my_getpagesize())); I think, min value could be my_getpagesize()*3 ? > +#endif Regards, Sergei

2 1

Re: [Maria-developers] MDEV-7273 - 10.1 fails to start up during tc_log initializations on PPC64
by Sergei Golubchik 25 Dec '14

25 Dec '14

Hi, Sergey! On Dec 17, svoj(a)mariadb.org wrote: > revision-id: 4a469b0ed68bb341e1a1e1ba540a45a3b6ff3ef9 > parent(s): ea8862aa9ffa1cfac9a99bbe0fe9b539f12b9a19 > committer: Sergey Vojtovich > branch nick: 10.1 > timestamp: 2014-12-17 13:55:01 +0400 > message: > > MDEV-7273 - 10.1 fails to start up during tc_log initializations on PPC64 > > log-tc-size is 24K by default. Page size is 64K on PPC64. But log-tc-size > must be at least 3 x page size. This is enforced by TC_LOG_MMAP::open() > with a comment: to guarantee non-empty pool. > > This all makes server not startable in default configuration on PPC64. > > When creating tc log, adjust it's size to max(log-tc-size, page size * 6). > > --- > sql/log.cc | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sql/log.cc b/sql/log.cc > index 24e5283..a2fd70f 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -8316,7 +8316,7 @@ int TC_LOG_MMAP::open(const char *opt_name) > O_RDWR, MYF(MY_WME))) < 0) > goto err; > inited=1; > - file_length= opt_tc_log_size; > + file_length= MY_MAX(tc_log_page_size * 2 * 3, opt_tc_log_size); > if (mysql_file_chsize(fd, file_length, 0, MYF(MY_WME))) > goto err; > } I don't particularly like silent adjustments of user-specified values. And anyway, as you found out, this fix is not sufficient. I'd suggest the following: 1. Create a sysvar for log-tc-size 2. invoke my_getpagesize() not from log.cc, but from mysqld.cc, and set min and default values for log-tc-size appropriately, don't forget to use SYSVAR_AUTOSIZE() 3. change the code not to depend on the TC_LOG_PAGE_SIZE constant. - it's basically used in only two places now: to set a default for TC_LOG_MIN_SIZE (but you'll fix it step 2) and in kristian's code for group commit, cookies[] array. - don't mind TC_LOG_PAGE_SIZE being used in TC_LOG_BINLOG, it means nothing there, any reasonable number will do, I suppose. - so basically you only need to remove TC_LOG_PAGE_SIZE from cookies[] array. That is, you can define it as 'ulong cookies[1]' and take care to fix all sizeof() in the code to add the real size there. 4. Then you can remove that assert. The tc log page will be of my_getpagesize() size and log-tc-size will be N*my_getpagesize() where N>3. Regards, Sergei

1 0

[Maria-developers] JIRA ping
by Jonas Oreland 23 Dec '14

23 Dec '14

Hi Kristian, I light of recent "I did see any of your comments until by accident just now", I now mail you directly that I updated both MDEV-162 and MDEV-7257. I want (review) comments. /Jonas

2 9

[Maria-developers] Review of Debian packaging file listing?
by Otto Kekäläinen 22 Dec '14

22 Dec '14

Hello! I recently updated my build scripts to automatically generate a file listing for each commit in in the Debian packaging "pool" I have. Here is a recent one: http://labs.seravo.fi/~otto/mariadb-repo/mariadb-10.0-sid-amd64/filelist-ec… It is now very easy to review and track what files are in what package. Could somebody please quickly review that file and tell me if you spot: - any files in wrong package and should be moved? - any package that has too few files and maybe something missing? - any file attributes that might be wrong or insecure? - should any of the MariaDB plugins deserve to be shipped as a separate package? (apart from current connect and oqgraph) -- Check out our blog at http://seravo.fi/blog and follow @ottokekalainen

4 5

[Maria-developers] Report from Debian/Ubuntu MySQL packaging sprint in London in December
by Otto Kekäläinen 21 Dec '14

21 Dec '14

Hello! I thought I'd brief the readers of this mailing list about the packaging sprint in London last week. It was organized by Canonical and in addition to Canonical staff and me there where also people from Oracle and Percona. We mostly worked on syncing the MySQL/MariaDB/Percona server packaging and a little bit also on creating a unified Galera package. We made good progress and all results have been pushed to public repos. I also recently updated the pages https://wiki.debian.org/Teams/MySQL/MariaDB and https://wiki.debian.org/Teams/MySQL/Galera to reflect the current status. One big technical change that was discussed and planned is the decoupling of the config and data directories. The plan is that in the future no MySQL variant in particular would anymore own the /etc/mysql/my.cnf file but instead each variant would register their own config file at installation time by using the Debian update-alternatives mechanism. So in future there would be /etc/mysql/mariadb.conf, mysql.conf, percona.conf etc and some more snippets in /etc/mysql/mariadb.conf.d/.. etc. The plan is not complete nor tested, but there are preliminary changes in the mysql-common package to provide the config file registration hook and in a preliminary Percona Debian package to use the new config file registration facility. As the config file defines the data directory, successful decoupling of config files would make it easy to also decouple the data directories so that if the user installs/removes/upgrades any of the MySQL variants they would not mess up their data accidentally and the installation scripts would help users to dump/migrate/copy/move their data in the common scenarios. -- Check out our blog at http://seravo.fi/blog and follow @ottokekalainen

2 1

[Maria-developers] CREATE USER gotcha
by Alexander Barkov 19 Dec '14

19 Dec '14

Hi Ian, I think there is a CREATE USER related gotcha which is not described in the manual. CREATE USER allows to create multiple users at the same time: CREATE USER user1, user2; A reader of https://mariadb.com/kb/en/mariadb/documentation/sql-commands/account-manage… might think that the above query is a short synonym for: CREATE USER user1; CREATE USER user2; But in fact it is not always true. The difference can happen during replication. A "CREATE USER" statement is binlogged as a single shot, but only if at least one user was really created. Suppose (on master) user1 already exists, while user2 does not. "CREATE USER user1, user2" will be binary logged as is, while two independent "CREATE USER user1" and "CREARE USER user2" statements will be logged only as "CREATE USER user2". The CREATE statement for user1 won't be logged, because it failed on master! Now, suppose non of the users exist on slave. In case of "CREATE USER user1, user2;" both will be created on slave. In case of two independent queries, only "user2" will be created on slave.

4 6

[Maria-developers] Syntax for parallel replication
by Kristian Nielsen 17 Dec '14

17 Dec '14

Hi Serg, Can you help me with suggestions/comments for the following proposal for how to do configuration for MDEV-6676, speculative parallel replication? I am not too happy about how things are in 10.0. There, there is a single option --slave-parallel-threads=N. If N>0, then parallel replication is enabled, else not. One problem is that this makes it not configurable per multi-source master connection. Another problem is that there are two possible mechanisms for parallelisation, group-commit based and domain_id based; currently, one can enable none of them or both, but not just one of them. MDEV-6676 will introduce at least one other mechanism, which seems to make it essential to make a better way to configure this. Now, ideally, there would not be any configuration at all. The server would just run things in parallel when possible and to the degree desirable. However, I think there are some reasons that we need to allow the user this configurability: - Parallel replication is still a somewhat experimental feature, so it seems too risky to enable it by default. Also, it doesn't really seem possible for the server to automatically set the best number of threads to use, with current implementation (or possibly any implementation). - When replicating with non-transactional updates, or in non-gtid mode, slave state is not crash safe. This is true in non-parallel replication also, but in parallel replication, the problem seems amplified, as there may be multiple transactions in progress at the time of a crash, complicating possible manual recovery. This also suggests that parallel replication must be configurable. - When using domain-based parallel replication, the user is responsible for ensuring that independent domains are non-conflicting and can be replicated out-of-order wrt. each other. So if replication domains are used, but this property is not guaranteed, then domain-based parallel replication need to be configurable, or parallel replication cannot be used at all. - The new speculative replication feature in MDEV-6676 is not always guaranteed to be a win - in some workloads, where there are many conflicts between successive transactions, excessive rollback could cause it to be less efficient than not using it. Again, this suggests it needs to be configurable. So given this, I came up with the following idea for syntax: CHANGE MASTER TO PARALLEL_MODE=(domain,groupcommit,transactional,waiting) Each of the four keywords in the parenthesis is optional. "domain" enables domain-based parallelisation, where each replication domain is treated independently. "groupcommit" enables the non-speculative mode, where only transactions that group-committed together on the master are applied in parallel on the slave. "transactional" enables the speculative mode, where all transactional DML is optimistically tried in parallel, and then in case of conflict a rollback and retry is done. "groupcommit" and "transactional" are mutually exclusive, at most one of them can be specified. The default would be (domain,groupcommit) to be back-wards compatible with 10.0. If slave_parallel_thread=0, then no parallel apply will happen even if PARALLEL_MODE is non-empty. If slave_parallel_thread>0 but PARALLEL_MODE is empty (PARALLEL_MODE=()), then again no parallel apply will be done. The "waiting" option is not essential to add, we could remove it, I put it in because there were already a number of options so it seemed to cause no harm. The idea is that on the master, we detect if transaction T2 had to do a row lock wait on transaction T1. If so, it seems likely that a similar conflict could occur on the slave, so we will not run T2 in parallel with T1; instead we will let T2 wait for T1 to commit before T2 is started. The "waiting" option could be enabled by a user to disable this check, enabling even more aggressive parallelisation. I am not sure if it is worth it to have this configurable though, comments welcome. I have not checked how hard it will be to implement the new syntax. We do not have any similar multi-option CHANGE MASTER elements, as far as I know, but it is similar to ENUM system variables. And we already have a IGNORE_SERVER_IDS syntax with comma-separated list within parenthesis. So hopefully not too hard to do, and somewhat consistent with existing syntax. So what do you think? Is it a reasonable syntax? Any comments, or suggestions for better way to do it? Thanks, - Kristian.

5 23

Re: [Maria-developers] Rev 4383: MDEV-6916:Upgrade from MySQL to MariaDB breaks already created views
by Sergei Golubchik 17 Dec '14

17 Dec '14

Hi, Sanja! Comments about the code, see below. Comments about the whole approach - see my other email. On Dec 07, sanja(a)askmonty.org wrote: > At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916/ > > ------------------------------------------------------------ > revno: 4383 > revision-id: sanja(a)askmonty.org-20141207094825-tx163unk3mm4gkql > parent: sanja(a)askmonty.org-20141203234414-63zkpeq9eb0n9lc1 > committer: sanja(a)askmonty.org > branch nick: work-maria-5.5-MDEV-6916 > timestamp: Sun 2014-12-07 10:48:25 +0100 > message: > MDEV-6916:Upgrade from MySQL to MariaDB breaks already created views > (SQL-level commands to support view update) > === modified file 'mysql-test/r/view.result' > --- a/mysql-test/r/view.result 2014-12-03 22:37:59 +0000 > +++ b/mysql-test/r/view.result 2014-12-07 09:48:25 +0000 > @@ -5398,6 +5398,29 @@ DROP VIEW v1; > DROP TABLE t1, t2; > create view v1 as select 1; > drop view v1; > +# > +#MDEV-6916:Upgrade from MySQL to MariaDB breaks already created views > +# (SQL command for upgrade check) > +# > +create table t1 (a int); > +create algorithm=temptable view v1 as select a from t1; > +show create view v1; > +View Create View character_set_client collation_connection > +v1 CREATE ALGORITHM=TEMPTABLE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select `t1`.`a` AS `a` from `t1` latin1 latin1_swedish_ci > +check table v1 force mariadb upgrade; > +Table Op Msg_type Msg_text > +test.v1 check note View algorithm swap > +show create view v1; > +View Create View character_set_client collation_connection > +v1 CREATE ALGORITHM=MERGE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select `t1`.`a` AS `a` from `t1` latin1 latin1_swedish_ci No, I don't like that. FORCE or not, upgrading of a view should not blindly change the alorithm. This functionality is created to solve a specific problem. So when a view has mariadb-version field we know with certainty that the algorithm is correct and it should *not* be changed with or without FORCE. In the test you'll need to copy mysql-created view frm (from std_data). Addendum: ok, I've read till the end. See my comment in view_check() function. > +check table v1 force mariadb upgrade; Also, I don't like that CHECK changes the algorithm of the view. For tables CHECK only performs the *check* (and updates the version in the frm). It's REPAIR that does the change. To be consistent with the behavior for tables, CHECK ... FOR UPGRADE should only report an error, like "view needs upgrading" and a user should then use REPAIR to actually fix the view. > +Table Op Msg_type Msg_text > +test.v1 check note View algorithm swap > +show create view v1; > +View Create View character_set_client collation_connection > +v1 CREATE ALGORITHM=TEMPTABLE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select `t1`.`a` AS `a` from `t1` latin1 latin1_swedish_ci > +drop view v1; > +drop table t1; > # ----------------------------------------------------------------- > # -- End of 5.5 tests. > # ----------------------------------------------------------------- > > === modified file 'sql/share/errmsg-utf8.txt' > --- a/sql/share/errmsg-utf8.txt 2014-05-27 06:45:01 +0000 > +++ b/sql/share/errmsg-utf8.txt 2014-12-07 09:48:25 +0000 > @@ -6565,3 +6565,5 @@ ER_QUERY_EXCEEDED_ROWS_EXAMINED_LIMIT > ER_NO_SUCH_TABLE_IN_ENGINE 42S02 > eng "Table '%-.192s.%-.192s' doesn't exist in engine" > swe "Det finns ingen tabell som heter '%-.192s.%-.192s' i handlern" > +ER_VIEW_UPGRADE_IS_DONE > + eng "View algorithm swap" No way, you cannot add a new error message in 5.5 because the next major branch (10.0) is already GA. > === modified file 'sql/sql_admin.cc' > --- a/sql/sql_admin.cc 2014-05-03 16:12:17 +0000 > +++ b/sql/sql_admin.cc 2014-12-07 09:48:25 +0000 > @@ -314,7 +314,8 @@ static bool mysql_admin_table(THD* thd, > HA_CHECK_OPT *), > int (handler::*operator_func)(THD *, > HA_CHECK_OPT *), > - int (view_operator_func)(THD *, TABLE_LIST*)) > + int (view_operator_func)(THD *, TABLE_LIST*, > + HA_CHECK_OPT *)) > { > TABLE_LIST *table; > SELECT_LEX *select= &thd->lex->select_lex; > @@ -521,7 +522,7 @@ static bool mysql_admin_table(THD* thd, > ER_CHECK_NO_SUCH_TABLE, ER(ER_CHECK_NO_SUCH_TABLE)); > /* if it was a view will check md5 sum */ > if (table->view && > - view_checksum(thd, table) == HA_ADMIN_WRONG_CHECKSUM) > + view_check(thd, table, check_opt) == HA_ADMIN_WRONG_CHECKSUM) > push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN, > ER_VIEW_CHECKSUM, ER(ER_VIEW_CHECKSUM)); > if (thd->stmt_da->is_error() && > @@ -536,7 +537,7 @@ static bool mysql_admin_table(THD* thd, > if (table->view) > { > DBUG_PRINT("admin", ("calling view_operator_func")); > - result_code= (*view_operator_func)(thd, table); > + result_code= (*view_operator_func)(thd, table, check_opt); > goto send_result; > } > > @@ -849,6 +850,14 @@ send_result_message: > system_charset_info); > break; > } > + case HA_ADMIN_VIEW_UPGRADE_IS_DONE: > + { > + protocol->store(STRING_WITH_LEN("note"), system_charset_info); > + protocol->store(ER(ER_VIEW_UPGRADE_IS_DONE), > + strlen(ER(ER_VIEW_UPGRADE_IS_DONE)), > + system_charset_info); > + break; > + } Don't do that. First, you cannot have ER_VIEW_UPGRADE_IS_DONE. Second, it's simply unnecessary. Return HA_ADMIN_OK from the view_operator_func() and use push_warning inside view_operator_func() to say that the algorithm was changed. > case HA_ADMIN_NEEDS_UPGRADE: > case HA_ADMIN_NEEDS_ALTER: > @@ -1071,7 +1080,7 @@ bool Check_table_statement::execute(THD > > res= mysql_admin_table(thd, first_table, &m_lex->check_opt, "check", > lock_type, 0, 0, HA_OPEN_FOR_REPAIR, 0, > - &handler::ha_check, &view_checksum); > + &handler::ha_check, &view_check); > > m_lex->select_lex.table_list.first= first_table; > m_lex->query_tables= first_table; > > === modified file 'sql/sql_view.cc' > --- a/sql/sql_view.cc 2014-12-03 23:44:14 +0000 > +++ b/sql/sql_view.cc 2014-12-07 09:48:25 +0000 > @@ -729,9 +729,29 @@ err: > DBUG_RETURN(res || thd->is_error()); > } > > +static void make_view_filename(LEX_STRING *dir, char *dir_buff, > + size_t dir_buff_len, > + LEX_STRING *path, char *path_buff, > + size_t path_buff_len, > + LEX_STRING *file, > + TABLE_LIST *view) > +{ > + /* print file name */ > + dir->length= build_table_filename(dir_buff, dir_buff_len - 1, > + view->db, "", "", 0); > + dir->str= dir_buff; > + > + path->length= build_table_filename(path_buff, path_buff_len - 1, > + view->db, view->table_name, reg_ext, 0); > + path->str= path_buff; > + > + file->str= path->str + dir->length; > + file->length= path->length - dir->length; > +} > + > > /* number of required parameters for making view */ > -static const int required_view_parameters= 15; > +static const int required_view_parameters= 16; eh? why? > /* > table of VIEW .frm field descriptors > @@ -795,6 +815,65 @@ static File_option view_parameters[]= > static LEX_STRING view_file_type[]= {{(char*) STRING_WITH_LEN("VIEW") }}; > > > +int mariadb_fix_view(THD *thd, TABLE_LIST *view) > +{ > + char dir_buff[FN_REFLEN + 1], path_buff[FN_REFLEN + 1]; > + LEX_STRING dir, file, path; > + DBUG_ENTER("mariadb_fix_view"); > + > + if (view->algorithm == VIEW_ALGORITHM_UNDEFINED) > + DBUG_RETURN(HA_ADMIN_OK); > + > + make_view_filename(&dir, dir_buff, sizeof(dir_buff), > + &path, path_buff, sizeof(path_buff), > + &file, view); > + /* init timestamp */ > + if (!view->timestamp.str) > + view->timestamp.str= view->timestamp_buffer; > + > + /* check old .frm */ > + { > + char path_buff[FN_REFLEN]; > + LEX_STRING path; > + File_parser *parser; > + > + path.str= path_buff; > + fn_format(path_buff, file.str, dir.str, "", MY_UNPACK_FILENAME); > + path.length= strlen(path_buff); > + if (access(path.str, F_OK)) > + DBUG_RETURN(HA_ADMIN_INVALID); > + > + if (!(parser= sql_parse_prepare(&path, thd->mem_root, 0))) > + DBUG_RETURN(HA_ADMIN_INTERNAL_ERROR); > + > + if (!parser->ok() || !is_equal(&view_type, parser->type())) > + DBUG_RETURN(HA_ADMIN_INVALID); > + } > + > + DBUG_ASSERT(view->algorithm == VIEW_ALGORITHM_MERGE || > + view->algorithm == VIEW_ALGORITHM_TMPTABLE); if() instead of an assert, please. while unlikely, it's possible that the file would be changed between two opens. > + if (view->algorithm == VIEW_ALGORITHM_MERGE) > + view->algorithm= VIEW_ALGORITHM_TMPTABLE; > + else > + view->algorithm= VIEW_ALGORITHM_MERGE; > + > + if (sql_create_definition_file(&dir, &file, view_file_type, > + (uchar*)view, view_parameters)) > + { > + sql_print_error("View '%-.192s'.'%-.192s': algorithm swap error.", "View %`s.%`s: cannot change the algorithm from %s to %s" And push_warning_printf() with the same. Or, rather, only push_warning_printf without sql_print_information. > + view->db, view->table_name); > + DBUG_RETURN(HA_ADMIN_INTERNAL_ERROR); > + } > + sql_print_information("View '%-.192s'.'%-.192s': algorithm swapped to '%s'", "View %`s.%`s: algorithm changed from %s to %s" And push_warning_printf() with the same. Or, rather, only push_warning_printf without sql_print_information. > + view->db, view->table_name, > + (view->algorithm == VIEW_ALGORITHM_MERGE)? > + "MERGE":"TEMPTABLE"); > + > + > + DBUG_RETURN(HA_ADMIN_VIEW_UPGRADE_IS_DONE); > +} > + > + > /* > Register VIEW (write .frm & process .frm's history backups) > > @@ -931,16 +1010,9 @@ static int mysql_register_view(THD *thd, > } > loop_out: > /* print file name */ > - dir.length= build_table_filename(dir_buff, sizeof(dir_buff) - 1, > - view->db, "", "", 0); > - dir.str= dir_buff; > - > - path.length= build_table_filename(path_buff, sizeof(path_buff) - 1, > - view->db, view->table_name, reg_ext, 0); > - path.str= path_buff; > - > - file.str= path.str + dir.length; > - file.length= path.length - dir.length; > + make_view_filename(&dir, dir_buff, sizeof(dir_buff), > + &path, path_buff, sizeof(path_buff), > + &file, view); > > /* init timestamp */ > if (!view->timestamp.str) > @@ -1953,6 +2025,33 @@ int view_checksum(THD *thd, TABLE_LIST * > HA_ADMIN_OK); > } you can make view_checksum() static now. > > +/** > + Check view > + > + @param thd thread handle > + @param view view for check > + @param check_opt check options > + > + @retval HA_ADMIN_OK OK > + @retval HA_ADMIN_NOT_IMPLEMENTED it is not VIEW > + @retval HA_ADMIN_WRONG_CHECKSUM check sum is wrong > +*/ > +int view_check(THD *thd, TABLE_LIST *view, HA_CHECK_OPT *check_opt) > +{ > + int res; > + DBUG_ENTER("view_check"); > + if ((res= view_checksum(thd, view)) != HA_ADMIN_OK) > + DBUG_RETURN(res); > + if (((check_opt->sql_flags & TT_MARIADB_UPGRADE) && > + !view->mariadb_version) || > + ((check_opt->sql_flags & (TT_MARIADB_UPGRADE | TT_FORCE)))) What is the point? Why would one ever want to change the algorithm of the provably created-in-mariadb view and pretend that it's an "upgrade"? To change the algorithm one should use ALTER. Fixing the view after an upgrade is not altering it, it's *fixing after an upgrade*. And views that were created in mariadb don't need to be fixed after an upgrade. No FORCE please. > + { > + DBUG_PRINT("XXX", ("Upgrade")); please either remove this DBUG_PRINT or change it to use a reasonable keyword. > + DBUG_RETURN(mariadb_fix_view(thd, view)); > + } > + DBUG_RETURN(HA_ADMIN_OK); > +} > + > === modified file 'sql/sql_yacc.yy' > --- a/sql/sql_yacc.yy 2014-11-08 18:54:42 +0000 > +++ b/sql/sql_yacc.yy 2014-12-07 09:48:25 +0000 > @@ -7299,6 +7300,10 @@ mi_check_type: > | EXTENDED_SYM { Lex->check_opt.flags|= T_EXTEND; } > | CHANGED { Lex->check_opt.flags|= T_CHECK_ONLY_CHANGED; } > | FOR_SYM UPGRADE_SYM { Lex->check_opt.sql_flags|= TT_FOR_UPGRADE; } > + | MARIADB_SYM UPGRADE_SYM > + { Lex->check_opt.sql_flags|= TT_MARIADB_UPGRADE; } > + | FORCE_SYM MARIADB_SYM UPGRADE_SYM > + { Lex->check_opt.sql_flags|= (TT_MARIADB_UPGRADE | TT_FORCE); } > ; > > optimize: Regards, Sergei

1 0

Re: [Maria-developers] Rev 4384: MDEV-6916: Upgrade from MySQL to MariaDB breaks already created views
by Sergei Golubchik 17 Dec '14

17 Dec '14

Hi, Sanja! Ok, I've seen all four patches. Specific comments about the code are below and in other emails. But the main question is - why are you doing it, what problem does it solve. As you said, the use case is - a user upgrades *from MySQL* and *runs mysql_upgrade*. To cover this use case mysql_upgrade needs to 1) detect whether it's an upgrade from MySQL, and to 2) run a command to change the algorithm of all views. You've done a lot of changes that aren't related to that. If this is, indeed, the use case, than you only need to: 1. add MySQL detection in mysql_upgrade. You have it, is_mysql() function. 2. add the code to toggle the algorithm of all views. See? No command line options, no FORCE, no CHECK, nothing. Now, let's consider extensions to this minimal functionality. 3. mariadb-version in the view frm file. this is a good idea, makes views a lot more future-proof. ok. I don't see why other extensions might be needed. Now, how to toggle the algorithm. The most natural solution would be to use ALTER VIEW. Like ALTER VIEW v1 ALGORITHM=MERGE. Unfortunately, ALTER VIEW doesn't work that way, I don't know why it was implemented in that crazy way in MySQL, but it's mostly useless. CHECK should not change its arguments. REPAIR? May be. I'd still prefer ALTER, but I don't know whether it can be extended in 5.5 in a safe way. If not then REPAIR ... AFTER UPGRADE is probably the second best. Or even REPAIR VIEW ... AFTER UPGRADE. It won't conflict with anything, doesn't add new keywords. Drawback - we'll be stuck with that hack for quite a while :( Even better would be to make mysql_upgrade to scan the data directory and rewrite view frms. No hacks or new grammar in the server. No new features in 5.5 GA. Nothing to maintain for years. Almost perfect. But it requires mysql_upgrade to have write access to the datadir. It kind of needs it already, but it's optional. With this approach it'll be less optional than before. So, I don't know... The most reasonable and future-proof solution would be to use ALTER TABLE. But I don't know whether we'll be able to do it in 5.5, looks a bit risky. Then there are reasonably safe hacks like REPAIR VIEW. And most safe and quite clean mysql_upgrade only approach, that needs write access to the datadir. Opinions? Anyway, here are comments about the code, below: On Dec 11, sanja(a)askmonty.org wrote: > At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916/ > > ------------------------------------------------------------ > revno: 4384 > revision-id: sanja(a)askmonty.org-20141211180458-j8ajlibnvvydmj40 > parent: sanja(a)askmonty.org-20141207094825-tx163unk3mm4gkql > committer: sanja(a)askmonty.org > branch nick: work-maria-5.5-MDEV-6916 > timestamp: Thu 2014-12-11 19:04:58 +0100 > message: > MDEV-6916: Upgrade from MySQL to MariaDB breaks already created views > > mysql_upgrade now fix views from mysql. > > (+ fix new view detection) > === modified file 'client/mysql_upgrade.c' > --- a/client/mysql_upgrade.c 2014-03-27 21:26:58 +0000 > +++ b/client/mysql_upgrade.c 2014-12-11 18:04:58 +0000 > @@ -40,7 +40,8 @@ static char mysql_path[FN_REFLEN]; > static char mysqlcheck_path[FN_REFLEN]; > > static my_bool opt_force, opt_verbose, debug_info_flag, debug_check_flag, > - opt_systables_only, opt_version_check; > + opt_systables_only, opt_version_check, > + opt_mysql_upgrade= 0, opt_skip_mysql_upgrade= 0; > static my_bool opt_not_used, opt_silent; > static uint my_end_arg= 0; > static char *opt_user= (char*)"root"; > @@ -150,6 +151,14 @@ static struct my_option my_long_options[ > &opt_not_used, &opt_not_used, 0, GET_BOOL, NO_ARG, 1, 0, 0, 0, 0, 0}, > {"version", 'V', "Output version information and exit.", 0, 0, 0, > GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}, > + {"mysql-upgrade", 'y', > + "Skip automatic detection MySQL and assume that we upgrade it", > + &opt_mysql_upgrade, &opt_mysql_upgrade, 0, GET_BOOL, NO_ARG, > + 0, 0, 0, 0, 0, 0}, > + {"skip-mysql-upgrade", 'Y', > + "Skip view algorithm upgrade from MySQL", > + &opt_skip_mysql_upgrade, &opt_skip_mysql_upgrade, 0, GET_BOOL, NO_ARG, > + 0, 0, 0, 0, 0, 0}, Those two are very bad option names. First, $ mysql_upgrade --mysql-upgrade looks plain weird. $ mysql_upgrade --skip-mysql-upgrade is even worse. And what about $ mysql_upgrade --mysql-upgrade --skip-mysql-upgrade Second, "skip" is the prefix automatically recognized by my_getopt. Normally, --skip-xxx is the same as --xxx=0. But now you're introducing --skip-mysql-upgrade which is very different from --skip-mysql-upgrade. While simultaneously adding support for --skip-skip-mysql-upgrade. A better option would be --fix-view-algorithm = { AUTO | ALWAYS | NEVER } > {"version-check", 'k', "Run this program only if its \'server version\' " > "matches the version of the server to which it's connecting, (enabled by " > "default); use --skip-version-check to avoid this check. Note: the \'server " > @@ -754,6 +771,23 @@ static int run_mysqlcheck_upgrade(void) > NULL); > } > > +static int run_mysqlcheck_views(void) > +{ > + if (!opt_mysql_upgrade) > + return 0; > + verbose("Phase 0: Fixing views"); No "Phase 0" please, renumber all phases to start from 1. > + print_conn_args("mysqlcheck"); > + return run_tool(mysqlcheck_path, > + NULL, /* Send output from mysqlcheck directly to screen */ > + "--no-defaults", > + ds_args.str, > + "--all-databases", > + "--view-upgrade", > + opt_verbose ? "--verbose": "", > + opt_silent ? "--silent": "", > + "2>&1", > + NULL); > +} > > === modified file 'client/mysqlcheck.c' > --- a/client/mysqlcheck.c 2014-02-17 10:00:51 +0000 > +++ b/client/mysqlcheck.c 2014-12-11 18:04:58 +0000 > @@ -42,7 +42,8 @@ static my_bool opt_alldbs = 0, opt_check > opt_medium_check = 0, opt_quick = 0, opt_all_in_1 = 0, > opt_silent = 0, opt_auto_repair = 0, ignore_errors = 0, > tty_password= 0, opt_frm= 0, debug_info_flag= 0, debug_check_flag= 0, > - opt_fix_table_names= 0, opt_fix_db_names= 0, opt_upgrade= 0; > + opt_fix_table_names= 0, opt_fix_db_names= 0, opt_upgrade= 0, > + opt_mariadb_upgrade= 0, opt_f_mariadb_upgrade= 0; > static my_bool opt_write_binlog= 1, opt_flush_tables= 0; > static uint verbose = 0, opt_mysql_port=0; > static int my_end_arg; > @@ -196,6 +197,12 @@ static struct my_option my_long_options[ > NO_ARG, 0, 0, 0, 0, 0, 0}, > {"version", 'V', "Output version information and exit.", 0, 0, 0, GET_NO_ARG, > NO_ARG, 0, 0, 0, 0, 0, 0}, > + {"view-upgrade", 'f', > + "Fix view algorithm view field if it is not new MariaDB view.", "Repair algorithm value for views created by MySQL (or MariaDB before 5.5.41)" > + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}, > + {"force-view-upgrade", 'F', > + "Swap view algorithm view field.", "Always change view algorithm" > + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}, > {0, 0, 0, 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0} > }; Regards, Sergei

1 0

[Maria-developers] Invisible Indexes and .frm Change?
by James Briggs 16 Dec '14

16 Dec '14

Hi list. I'm looking into adding invisible indexes. There's 3 "scopes" I've thought of: 1) global and persistent (create table, alter table, create index) 2) global temporary (alter table t1 change index invisible global) 3) session temporary (alter table t1 change index invisible session) #1 likely means changing the .frm format, which would cause backwardcompatibility issues. Are there any upcoming changes to .frm planned? Any comments or suggestions? Thanks, James Briggs. -- Cassandra/MySQL DBA. Available in San Jose area or remote. cass_top: https://github.com/jamesbriggs/cassandra-top

2 1

Re: [Maria-developers] Rev 4350: MDEV-6985: MariaDB crashes on stored procedure call
by Sergei Golubchik 16 Dec '14

16 Dec '14

Hi, Sanja! On Nov 17, sanja(a)askmonty.org wrote: > ------------------------------------------------------------ > revno: 4350 > revision-id: sanja(a)askmonty.org-20141117132635-gf281ipiomifw1oc > parent: psergey(a)askmonty.org-20141113111559-cuhsyfhor141qo6a > committer: sanja(a)askmonty.org > branch nick: work-maria-5.5-MDEV-6985 > timestamp: Mon 2014-11-17 14:26:35 +0100 > message: > MDEV-6985: MariaDB crashes on stored procedure call > > Item_ident fixed to allow double cleanup(). > === modified file 'sql/item.cc' > --- a/sql/item.cc 2014-10-06 17:53:55 +0000 > +++ b/sql/item.cc 2014-11-17 13:26:35 +0000 > @@ -862,12 +862,20 @@ void Item_ident::cleanup() > field_name ? field_name : "(null)", > orig_field_name ? orig_field_name : "(null)")); > #endif Before pushing, please remove the above #ifdef, in a separate commit. thanks! > + bool was_fixed= fixed; > Item::cleanup(); > db_name= orig_db_name; > table_name= orig_table_name; > field_name= orig_field_name; > /* Store if this Item was depended */ > - can_be_depended= test(depended_from); > + if (was_fixed) > + { > + /* > + We can trust that depended_from set correctly only if this item > + was fixed > + */ > + can_be_depended= test(depended_from); > + } Are double cleanups generally normal? Why this item is cleaned up twice? > DBUG_VOID_RETURN; > } Regards, Sergei

2 2

Re: [Maria-developers] Rev 4382: Added creation time to the view frm
by Sergei Golubchik 16 Dec '14

16 Dec '14

Hi, Sanja! On Dec 03, sanja(a)askmonty.org wrote: > At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916/ > > ------------------------------------------------------------ > revno: 4382 > revision-id: sanja(a)askmonty.org-20141203234414-63zkpeq9eb0n9lc1 > parent: sanja(a)askmonty.org-20141203223759-316k8y35qdb4cgyq > committer: sanja(a)askmonty.org > branch nick: work-maria-5.5-MDEV-6916 > timestamp: Thu 2014-12-04 00:44:14 +0100 > message: > Added creation time to the view frm. > ALTER VIEW saves it. CREATE VIEW & CREATE OR REPLACE VIEW reset it. Please no. This has nothing to do with MDEV-6916, it's a separate task and should be reviewed and discussed separately, not sneaked in under the MDEV-6916 cover. Regards, Sergei

1 0

Re: [Maria-developers] Rev 4381: Adding mariadb-version on the view creation to view frm. (MDEV-6916 followup)
by Sergei Golubchik 16 Dec '14

16 Dec '14

Hi, Sanja! On Dec 03, sanja(a)askmonty.org wrote: > ------------------------------------------------------------ > revno: 4381 > revision-id: sanja(a)askmonty.org-20141203223759-316k8y35qdb4cgyq > parent: jplindst(a)mariadb.org-20141203113839-xkxdy39mq77vu99y > committer: sanja(a)askmonty.org > branch nick: work-maria-5.5-MDEV-6916 > timestamp: Wed 2014-12-03 23:37:59 +0100 > message: > Adding mariadb-version on the view creation to view frm. (MDEV-6916 followup) Looks ok, thanks Regards, Sergei

1 0

[Maria-developers] Review of MDEV-7112 Split HA_CREATE_INFO
by Sergei Golubchik 04 Dec '14

04 Dec '14

Hi. It think the split is good, I liked it. I didn't like that you've tried to put the refactoring of sql_db.cc and sql_lang.h in the same commit, please move it to a separate patch. MDEV-5359 should not depend on that. Anyway, see all my comments below: > diff --git a/sql/structs.h b/sql/structs.h > index 99561c5..b38685c 100644 > --- a/sql/structs.h > +++ b/sql/structs.h > @@ -471,4 +471,67 @@ class Discrete_intervals_list { > Discrete_interval* get_current() const { return current; }; > }; > > + > +struct DDL_options_st > +{ > +public: > + enum Options > + { > + OPT_NONE= 0, > + OPT_IF_NOT_EXISTS= 2, // CREATE TABLE IF NOT EXISTS > + OPT_LIKE= 4, // CREATE TABLE LIKE > + OPT_OR_REPLACE= 16, // CREATE OR REPLACE TABLE > + OPT_OR_REPLACE_SLAVE_GENERATED= 32,// REPLACE was added on slave, it was > + // not in the original query on master. > + OPT_IF_EXISTS= 64 > + }; > + > +private: > + Options m_options; > + > +protected: > + > +public: > + Options create_like_options() const > + { > + return (DDL_options_st::Options) > + (((uint) m_options) & (OPT_IF_NOT_EXISTS | OPT_OR_REPLACE)); > + } > + void init() { m_options= OPT_NONE; } > + void init(Options options) { m_options= options; } > + void set(Options other) > + { > + m_options= other; > + } > + void set(const DDL_options_st other) > + { > + m_options= other.m_options; > + } > + bool if_not_exists() const { return m_options & OPT_IF_NOT_EXISTS; } > + bool or_replace() const { return m_options & OPT_OR_REPLACE; } > + bool or_replace_slave_generated() const > + { return m_options & OPT_OR_REPLACE_SLAVE_GENERATED; } > + bool like() const { return m_options & OPT_LIKE; } > + bool if_exists() const { return m_options & OPT_IF_EXISTS; } > + void add(const DDL_options_st::Options other) > + { > + m_options= (Options) ((uint) m_options | (uint) other); > + } > + void add(const DDL_options_st &other) > + { > + add(other.m_options); > + } > + DDL_options_st operator|(const DDL_options_st &other) > + { > + add(other.m_options); > + return *this; > + } > + DDL_options_st operator|=(DDL_options_st::Options other) > + { > + add(other); > + return *this; > + } > +}; I don't particularly like this style of C++ing... It's way too verbose. but ok > + > + > #endif /* STRUCTS_INCLUDED */ > diff --git a/sql/handler.h b/sql/handler.h > index 0044556..7b6a5f4 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -376,10 +376,7 @@ enum enum_alter_inplace_result { > #define HA_KEY_BLOB_LENGTH 2 > > #define HA_LEX_CREATE_TMP_TABLE 1 > -#define HA_LEX_CREATE_IF_NOT_EXISTS 2 > -#define HA_LEX_CREATE_TABLE_LIKE 4 > #define HA_CREATE_TMP_ALTER 8 > -#define HA_LEX_CREATE_REPLACE 16 One empty line here, please, HA_MAX_REC_LENGTH is not a flag > #define HA_MAX_REC_LENGTH 65535 > > /* Table caching type */ > @@ -1571,9 +1568,9 @@ enum enum_stats_auto_recalc { HA_STATS_AUTO_RECALC_DEFAULT= 0, > HA_STATS_AUTO_RECALC_ON, > HA_STATS_AUTO_RECALC_OFF }; > Could you add structure comments here? Like what you have in MDEV, "This will be passed to mysql_create_db, mysql_drop_db, mysqld_show_create_db", etc To explain what every structure is for, what kind of data it should store, and expected users (internal DB DDL, internal table DDL, handler, etc) > -struct HA_CREATE_INFO > +struct Table_contents_source_st > { > - CHARSET_INFO *table_charset, *default_table_charset; > + CHARSET_INFO *table_charset; > LEX_CUSTRING tabledef_version; > LEX_STRING connect_string; > const char *password, *tablespace; > @@ -1635,6 +1635,55 @@ struct HA_CREATE_INFO > }; > comments for all new structures, please > +struct Schema_specification_st > +{ > + CHARSET_INFO *default_table_charset; > + void init() > + { > + bzero(this, sizeof(*this)); > + } > +}; > + > + > +struct HA_CREATE_INFO: public Table_contents_source_st, > + public Schema_specification_st > +{ > + void init() > + { > + Table_contents_source_st::init(); > + Schema_specification_st::init(); > + } > +}; > + > + > +struct Table_specification_st: public HA_CREATE_INFO, > + public DDL_options_st > +{ > + // Deep initialization > + void init() > + { > + HA_CREATE_INFO::init(); > + DDL_options_st::init(); > + } > + void init(DDL_options_st::Options options) > + { > + HA_CREATE_INFO::init(); > + DDL_options_st::init(options); > + } > + /* > + Quick initialization, for parser. > + Most of the HA_CREATE_INFO is left uninitialized. > + It gets fully initialized in sql_yacc.yy, only when the parser > + scans a related keyword (e.g. CREATE, ALTER). > + */ > + void lex_start() > + { > + HA_CREATE_INFO::options= 0; > + DDL_options_st::init(); > + } > +}; > + > + > /** > In-place alter handler context. > > diff --git a/sql/sql_lang.h b/sql/sql_lang.h > new file mode 100644 > index 0000000..76a1340 > --- /dev/null > +++ b/sql/sql_lang.h > @@ -0,0 +1,79 @@ > +/* Copyright (c) 2014, MariaDB Foundation > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; version 2 of the License. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program; if not, write to the Free Software > + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ > + > +#ifndef SQL_LANG_INCLUDED > +#define SQL_LANG_INCLUDED > + > +namespace SQL { > + > +class Identifier: protected LEX_CSTRING > +{ > + void set(const LEX_STRING &other) > + { > + str= other.str; > + length= other.length; > + DBUG_ASSERT(str[length] == 0); > + } > +public: > + Identifier(const LEX_STRING &other) > + { > + set(other); > + } > + Identifier(const LEX *); > + const char *name() const { return str; } > + const size_t name_length() const { return length; } > +}; Eh? What all this has to do with MDEV-7112 "Split HA_CREATE_INFO" ? it's not mentioned there. If it's unrelated refactoring, please, put it in a separate patch. > + > + > +class Identifier_normalized: public Identifier > +{ > + char m_buf[NAME_LEN + 1 + MYSQL50_TABLE_NAME_PREFIX_LENGTH + 10]; > + void normalize(); > +public: > + Identifier_normalized(const LEX_STRING &other) > + :Identifier(other) { normalize(); } > + Identifier_normalized(const LEX *lex) > + :Identifier(lex) { normalize(); } > +}; > + > + > +namespace Statement { > + > + > +/* > + All SQL statements that can be executed directly. > + Statement instances are created using the data in thd->lex, > + previously populated by the SQL parser. > +*/ > +class Directly_executable > +{ > +public: > + /* > + Perform all stages of a direct SQL execution: > + - Check for validity (e.g. name in "CREATE SCHEMA name" is valid). > + - Check access > + - Apply filters (e.g. Rpl_filters on slave) > + - Perform WSREP_TO_ISOLATION > + - Execute the actual action > + */ > + virtual int exec_direct(THD *thd) const = 0; > +}; > + > +} // End of namescpace Statement > + > + > +} // End of namespace SQL > + > +#endif /* SQL_LANG_INCLUDED */ > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 0bbcca5..0edda65 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -1923,7 +1923,9 @@ bool MDL_deadlock_handler::handle_condition(THD *, > */ > > static bool > -open_table_get_mdl_lock(THD *thd, Open_table_context *ot_ctx, > +open_table_get_mdl_lock(THD *thd, > + const DDL_options_st &options, > + Open_table_context *ot_ctx, What's the point - you don't seem to be using options in this function? And that recursively means that you don't need 'options' in all other functions that only pass 'options' argument down to open_table_get_mdl_lock() but otherwise don't use it. > MDL_request *mdl_request, > uint flags, > MDL_ticket **mdl_ticket) > @@ -4176,8 +4182,8 @@ thr_lock_type read_lock_type_for_table(THD *thd, > DBUG_RETURN(FALSE); > > /* Check if CREATE TABLE without REPLACE was used */ > - create_table= (thd->lex->sql_command == SQLCOM_CREATE_TABLE && > - !(thd->lex->create_info.options & HA_LEX_CREATE_REPLACE)); > + create_table= thd->lex->sql_command == SQLCOM_CREATE_TABLE && > + !options.or_replace(); why, options are still in thd->lex, aren't they? > > if (!(flags & MYSQL_OPEN_SKIP_SCOPED_MDL_LOCK)) > { > diff --git a/sql/sql_db.h b/sql/sql_db.h > index 62d379c..dc62e6b 100644 > --- a/sql/sql_db.h > +++ b/sql/sql_db.h > @@ -35,13 +33,178 @@ bool mysql_opt_change_db(THD *thd, > bool my_dboptions_cache_init(void); > void my_dboptions_cache_free(void); > bool check_db_dir_existence(const char *db_name); > -bool load_db_opt(THD *thd, const char *path, HA_CREATE_INFO *create); > +bool load_db_opt(THD *thd, const char *path, Schema_specification_st *create); > bool load_db_opt_by_name(THD *thd, const char *db_name, > - HA_CREATE_INFO *db_create_info); > + Schema_specification_st *db_create_info); > CHARSET_INFO *get_default_db_collation(THD *thd, const char *db_name); > bool my_dbopt_init(void); > void my_dbopt_cleanup(void); > > + > +namespace SQL { > + > + > +namespace Statement { > + > + > +namespace Schema { > + > + > +// Schema name > +class Name: public Identifier_normalized > +{ > +public: > + Name(const LEX_STRING &other) :Identifier_normalized(other) { } > + Name(const LEX *lex) :Identifier_normalized(lex) { } > +#ifdef HAVE_REPLICATION > + bool check_if_slave_ignored(THD *thd) const; > +#endif > + bool check() const; > + bool check_with_error() const; > +}; > + > + > +// Schema definition options: IF EXISTS, IF NOT EXISTS > +class DDL_options: public DDL_options_st > +{ > +public: > + DDL_options() { init(); } > + DDL_options(Options options) > + { > + set(options); > + } > + DDL_options(const DDL_options_st options) > + { > + set(options); > + } > + DDL_options(const LEX *lex); > +}; > + > + > +// Schema specification, e.g. CHARACTER SET xxx > +class Specification: public Schema_specification_st > +{ > +public: > + Specification(const Schema_specification_st &info) > + :Schema_specification_st(info) > + { } > + Specification(const LEX *lex); > +}; > + > + > +/** > + A common class for all Schema DDL options: > + - CREATE SCHEMA > + - DROP SCHEMA > + - ALTER SCHEMA > + - ALTER SCHEMA UPGRADE DATA DIRECTORY NAME > +*/ > +class DDL: public Directly_executable > +{ > +protected: > + bool rm_db(THD *thd, const Name &name, bool if_exists, bool silent) const; > + > + int create_db(THD *thd, > + const Name &name, > + const DDL_options_st options, > + const Schema_specification_st &info, > + bool silent) const; > +}; > + > + > +class DDL_with_name: public DDL, public Name > +{ > + /* > + Execute a DDL statement. > + Assumes that Name has already been checked for validity. > + */ > + virtual bool exec(THD *thd) const = 0; > + virtual bool check_access(THD *) const = 0; > +protected: > + DDL_with_name(const Name &name): Name(name) { } > + DDL_with_name(const LEX *lex): Name(lex) { } > +public: > + int exec_direct(THD *thd) const; > +}; > + > + > +class DDL_with_name_and_options: public DDL_with_name, public DDL_options > +{ > +protected: > + DDL_with_name_and_options(const LEX *lex) > + :DDL_with_name(lex), DDL_options(lex) { } > + DDL_with_name_and_options(const Name &name) > + :DDL_with_name(name), DDL_options() { } > +}; > + > + > +class Create: public DDL_with_name_and_options, public Specification > +{ > + bool exec(THD *thd) const > + { > + return DDL::create_db(thd, *this, *this, *this, false); > + } > + bool check_access(THD *thd) const > + { > + return ::check_access(thd, CREATE_ACL, name(), NULL, NULL, 1, 0); > + } > +public: > + Create(const LEX *lex) > + :DDL_with_name_and_options(lex), Specification(lex) > + { } > + Create(const Name &name, const Schema_specification_st &info) > + :DDL_with_name_and_options(name), Specification(info) > + { } > +}; > + > + > +class Drop: public DDL_with_name_and_options > +{ > + bool exec(THD *thd) const > + { > + return rm_db(thd, *this, if_exists(), false); > + } > + bool check_access(THD *thd) const > + { > + return ::check_access(thd, DROP_ACL, name(), NULL, NULL, 1, 0); > + } > +public: > + Drop(const Name &name) :DDL_with_name_and_options(name) { } > + Drop(const LEX *lex) :DDL_with_name_and_options(lex) { } > +}; > + > + > +class Alter: public DDL_with_name, public Specification > +{ > + bool exec(THD *thd) const; > + bool check_access(THD *thd) const > + { > + return ::check_access(thd, ALTER_ACL, name(), NULL, NULL, 1, 0); > + } > +public: > + Alter(const LEX *lex) :DDL_with_name(lex), Specification(lex) { } > +}; > + > + > +class Upgrade: public DDL_with_name > +{ > + bool exec(THD *thd) const; > + bool check_access(THD *thd) const > + { > + return ::check_access(thd, ALTER_ACL, name(), NULL, NULL, 1, 0) || > + ::check_access(thd, DROP_ACL, name(), NULL, NULL, 1, 0) || > + ::check_access(thd, CREATE_ACL, name(), NULL, NULL, 1, 0); > + } > +public: > + Upgrade(const LEX *lex) :DDL_with_name(lex) { } > + int exec_direct(THD *thd) const; > +}; > + > + > +} // End of namespace SQL::Statement::Schema > +} // End of namespace SQL::Statement > +} // End of namespace SQL Please, no. This and sql_lang.h - make it a separate patch This one should be *only* about splitting HA_CREATE_INFO > + > #define MY_DB_OPT_FILE "db.opt" > > #endif /* SQL_DB_INCLUDED */ > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index b905d8b..c59332e 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -4181,143 +4176,21 @@ static bool do_execute_sp(THD *thd, sp_head *sp) > } > break; > case SQLCOM_CREATE_DB: > - { > - /* > - As mysql_create_db() may modify HA_CREATE_INFO structure passed to > - it, we need to use a copy of LEX::create_info to make execution > - prepared statement- safe. > - */ > - HA_CREATE_INFO create_info(lex->create_info); > - if (check_db_name(&lex->name)) > - { > - my_error(ER_WRONG_DB_NAME, MYF(0), lex->name.str); > - break; > - } > - /* > - If in a slave thread : > - CREATE DATABASE DB was certainly not preceded by USE DB. > - For that reason, db_ok() in sql/slave.cc did not check the > - do_db/ignore_db. And as this query involves no tables, tables_ok() > - above was not called. So we have to check rules again here. > - */ > -#ifdef HAVE_REPLICATION > - if (thd->slave_thread) > - { > - rpl_filter= thd->system_thread_info.rpl_sql_info->rpl_filter; > - if (!rpl_filter->db_ok(lex->name.str) || > - !rpl_filter->db_ok_with_wild_table(lex->name.str)) > - { > - my_message(ER_SLAVE_IGNORED_TABLE, ER(ER_SLAVE_IGNORED_TABLE), MYF(0)); > - break; > - } > - } > -#endif > - if (check_access(thd, CREATE_ACL, lex->name.str, NULL, NULL, 1, 0)) > - break; > - WSREP_TO_ISOLATION_BEGIN(lex->name.str, NULL, NULL) > - res= mysql_create_db(thd, lex->name.str, &create_info, 0); separate refactoring patch, please > + if ((res= SQL::Statement::Schema::Create(lex).exec_direct(thd)) < 0) > + goto error; > break; > - } > case SQLCOM_DROP_DB: > - { > - if (check_db_name(&lex->name)) > - { > - my_error(ER_WRONG_DB_NAME, MYF(0), lex->name.str); > - break; > - } > - /* > - If in a slave thread : > - DROP DATABASE DB may not be preceded by USE DB. > - For that reason, maybe db_ok() in sql/slave.cc did not check the > - do_db/ignore_db. And as this query involves no tables, tables_ok() > - above was not called. So we have to check rules again here. > - */ > -#ifdef HAVE_REPLICATION > - if (thd->slave_thread) > - { > - rpl_filter= thd->system_thread_info.rpl_sql_info->rpl_filter; > - if (!rpl_filter->db_ok(lex->name.str) || > - !rpl_filter->db_ok_with_wild_table(lex->name.str)) > - { > - my_message(ER_SLAVE_IGNORED_TABLE, ER(ER_SLAVE_IGNORED_TABLE), MYF(0)); > - break; > - } > - } > -#endif > - if (check_access(thd, DROP_ACL, lex->name.str, NULL, NULL, 1, 0)) > - break; > - WSREP_TO_ISOLATION_BEGIN(lex->name.str, NULL, NULL) > - res= mysql_rm_db(thd, lex->name.str, lex->check_exists, 0); > + if ((res= SQL::Statement::Schema::Drop(lex).exec_direct(thd)) < 0) > + goto error; > break; > - } > case SQLCOM_ALTER_DB_UPGRADE: > - { > - LEX_STRING *db= & lex->name; > -#ifdef HAVE_REPLICATION > - if (thd->slave_thread) > - { > - rpl_filter= thd->system_thread_info.rpl_sql_info->rpl_filter; > - if (!rpl_filter->db_ok(db->str) || > - !rpl_filter->db_ok_with_wild_table(db->str)) > - { > - res= 1; > - my_message(ER_SLAVE_IGNORED_TABLE, ER(ER_SLAVE_IGNORED_TABLE), MYF(0)); > - break; > - } > - } > -#endif > - if (check_db_name(db)) > - { > - my_error(ER_WRONG_DB_NAME, MYF(0), db->str); > - break; > - } > - if (check_access(thd, ALTER_ACL, db->str, NULL, NULL, 1, 0) || > - check_access(thd, DROP_ACL, db->str, NULL, NULL, 1, 0) || > - check_access(thd, CREATE_ACL, db->str, NULL, NULL, 1, 0)) > - { > - res= 1; > - break; > - } > - WSREP_TO_ISOLATION_BEGIN(db->str, NULL, NULL) > - res= mysql_upgrade_db(thd, db); > - if (!res) > - my_ok(thd); > + if ((res= SQL::Statement::Schema::Upgrade(lex).exec_direct(thd)) < 0) > + goto error; > break; > - } > case SQLCOM_ALTER_DB: > - { > - LEX_STRING *db= &lex->name; > - HA_CREATE_INFO create_info(lex->create_info); > - if (check_db_name(db)) > - { > - my_error(ER_WRONG_DB_NAME, MYF(0), db->str); > - break; > - } > - /* > - If in a slave thread : > - ALTER DATABASE DB may not be preceded by USE DB. > - For that reason, maybe db_ok() in sql/slave.cc did not check the > - do_db/ignore_db. And as this query involves no tables, tables_ok() > - above was not called. So we have to check rules again here. > - */ > -#ifdef HAVE_REPLICATION > - if (thd->slave_thread) > - { > - rpl_filter= thd->system_thread_info.rpl_sql_info->rpl_filter; > - if (!rpl_filter->db_ok(db->str) || > - !rpl_filter->db_ok_with_wild_table(db->str)) > - { > - my_message(ER_SLAVE_IGNORED_TABLE, ER(ER_SLAVE_IGNORED_TABLE), MYF(0)); > - break; > - } > - } > -#endif > - if (check_access(thd, ALTER_ACL, db->str, NULL, NULL, 1, 0)) > - break; > - WSREP_TO_ISOLATION_BEGIN(db->str, NULL, NULL) > - res= mysql_alter_db(thd, db->str, &create_info); > + if ((res= SQL::Statement::Schema::Alter(lex).exec_direct(thd)) < 0) > + goto error; > break; > - } > case SQLCOM_SHOW_CREATE_DB: > { > char db_name_buff[NAME_LEN+1]; > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 0541cbc..2536255 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -1696,14 +1695,14 @@ int show_create_table(THD *thd, TABLE_LIST *table_list, String *packet, > > packet->append(STRING_WITH_LEN("CREATE ")); > if (create_info_arg && > - (create_info_arg->org_options & HA_LEX_CREATE_REPLACE || > + ((create_info_arg->or_replace() && > + !create_info_arg->or_replace_slave_generated()) || that's a bit strange. Could you explain this? why org_options & HA_LEX_CREATE_REPLACE is translated to or_replace() && !or_replace_slave_generated() > create_info_arg->table_was_deleted)) > packet->append(STRING_WITH_LEN("OR REPLACE ")); > if (share->tmp_table) > packet->append(STRING_WITH_LEN("TEMPORARY ")); > packet->append(STRING_WITH_LEN("TABLE ")); > - if (create_info_arg && > - (create_info_arg->options & HA_LEX_CREATE_IF_NOT_EXISTS)) > + if (create_info_arg && create_info_arg->if_not_exists()) > packet->append(STRING_WITH_LEN("IF NOT EXISTS ")); > if (table_list->schema_table) > alias= table_list->schema_table->table_name; > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index b991215..7bb293b 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -4628,7 +4628,8 @@ int create_table_impl(THD *thd, > const char *orig_db, const char *orig_table_name, > const char *db, const char *table_name, > const char *path, > - HA_CREATE_INFO *create_info, > + const DDL_options_st options, > + HA_CREATE_INFO &create_info, why? I generally prefer pointers over C++ references - one can immediately see what variables can be modified by the function. is there a compelling reason to use references here? > Alter_info *alter_info, > int create_table_mode, > bool *is_trans, > @@ -5273,9 +5275,10 @@ bool mysql_create_like_table(THD* thd, TABLE_LIST* table, > */ > > /* Copy temporarily the statement flags to thd for lock_table_names() */ > + // QQ: is this really needed??? may be not anymore, as you pass flags separately > uint save_thd_create_info_options= thd->lex->create_info.options; > - thd->lex->create_info.options|= create_info->options; > - res= open_tables(thd, &thd->lex->query_tables, &not_used, 0); > + thd->lex->create_info.options|= create_info.options; > + res= open_tables(thd, create_info, &thd->lex->query_tables, &not_used, 0); > thd->lex->create_info.options= save_thd_create_info_options; > > if (res) > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 632bdd2..0f2c0e0 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -1621,7 +1623,8 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize); > IDENT_sys TEXT_STRING_sys TEXT_STRING_literal > NCHAR_STRING opt_component key_cache_name > sp_opt_label BIN_NUM label_ident TEXT_STRING_filesystem ident_or_empty > - opt_constraint constraint opt_ident opt_if_not_exists_ident > + opt_constraint constraint opt_ident > + opt_if_not_exists_opt_table_element_name can you please try to come up with shorter identifier names? 40 characters - that's a bit too long :) > > %type <lex_str_ptr> > opt_table_alias > diff --git a/storage/xtradb/handler/ha_innodb.cc b/storage/xtradb/handler/ha_innodb.cc > index f3b1167..6b1942e 100644 > --- a/storage/xtradb/handler/ha_innodb.cc > +++ b/storage/xtradb/handler/ha_innodb.cc > @@ -11349,7 +11349,7 @@ static __attribute__((nonnull, warn_unused_result)) > > /* Do not use DATA DIRECTORY with TEMPORARY TABLE. */ > if (create_info->data_file_name > - && create_info->options & HA_LEX_CREATE_TMP_TABLE) { > + && create_info->tmp_table()) { nope, you should not change storage engine API here. tmp_table() was a convenience wrapper, old way should still work > push_warning( > thd, Sql_condition::WARN_LEVEL_WARN, > ER_ILLEGAL_HA_CREATE_OPTION, Regards, Sergei

2 4

[Maria-developers] better execution plan possible with INNER JOIN of mroonga (or whatever storage engine) ?
by Kazuhiko Shiozaki 03 Dec '14

03 Dec '14

Hello ! I am investigating a slow query and have a question about execution plan with mroonga (possibly any storage engine?). (with MariaDB 10.0.15.) Here are tables : CREATE TABLE catalog ( uid bigint(20) unsigned NOT NULL, path varchar(255) COLLATE utf8_unicode_ci NOT NULL DEFAULT '', PRIMARY KEY (uid) ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci; CREATE TABLE catalog_full_text ( uid bigint(20) unsigned NOT NULL, title varchar(255) COLLATE utf8_unicode_ci DEFAULT '', PRIMARY KEY (uid), FULLTEXT KEY title (title) COMMENT 'parser "TokenBigramSplitSymbolAlphaDigit"' ) ENGINE=Mroonga DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci; CREATE TABLE category ( uid bigint(20) unsigned NOT NULL, category_uid bigint(20) unsigned NOT NULL DEFAULT '0', base_category_uid bigint(20) unsigned NOT NULL DEFAULT '0', PRIMARY KEY (uid,category_uid,base_category_uid) ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci; Here is the query : SELECT DISTINCT catalog.path, catalog.uid FROM ( ( catalog AS catalog INNER JOIN ( category AS category INNER JOIN catalog_full_text AS source_title_full_text ON category.base_category_uid = 18482 AND source_title_full_text.uid = category.category_uid ) ON category.uid = catalog.uid ) INNER JOIN catalog_full_text AS title_full_text ON title_full_text.uid = catalog.uid ) WHERE MATCH (source_title_full_text.title) AGAINST ('bar' IN BOOLEAN MODE) AND MATCH (title_full_text.title) AGAINST ('foo' IN BOOLEAN MODE) Here is the output of EXPLAIN : *************************** 1. row *************************** id: 1 select_type: SIMPLE table: source_title_full_text type: fulltext possible_keys: PRIMARY,title key: title key_len: 0 ref: rows: 1 Extra: Using where with pushed condition; Using temporary *************************** 2. row *************************** id: 1 select_type: SIMPLE table: title_full_text type: fulltext possible_keys: PRIMARY,title key: title key_len: 0 ref: rows: 1 Extra: Using where with pushed condition *************************** 3. row *************************** id: 1 select_type: SIMPLE table: catalog type: eq_ref possible_keys: PRIMARY key: PRIMARY key_len: 8 ref: test.title_full_text.uid rows: 1 Extra: *************************** 4. row *************************** id: 1 select_type: SIMPLE table: category type: eq_ref possible_keys: PRIMARY key: PRIMARY key_len: 24 ref: test.title_full_text.uid,test.source_title_full_text.uid,const rows: 1 Extra: Using index; Distinct As you see, we have two full-text queries at the beginning and both have 'rows = 1'. In a slow query case, 25k records mathces with the first full-text query and 1.6k records matches with the second full-text query. When I manually modify the order of INNER JOIN so that the 'lighter' full-text query appears first, EXPLAIN output is different as well and the performance is much improved. Accoding to Kohei, a Mroonga developer, we always have 'rows = 1' for mroonga full-text query because MariaDB does not request such value to mroonga (JOIN::get_examined_rows() in sql/sql_select.cc), and it is possible to modify mroonga to return an estimation of rows if MariaDB has such an API. So here are my questions : * can MariaDB request an 'estimation of rows' to mroonga (or whatever storage engine) ? * can such information help to decide a better execution plan with INNER JOIN like above ? Thanks in advance ! -- Kazuhiko Shiozaki, Nexedi SA Senior Consultant Nexedi: Consulting and Development of Free / Open Source Software http://www.nexedi.com/ ERP5: Full Featured High End Open Source ERP http://www.erp5.com/

3 3

[Maria-developers] Google's patches to MariaDB are available in the opensource repo
by Pavel Ivanov 03 Dec '14

03 Dec '14

Hi all, Effective immediately the full opensource tree of changes made to MariaDB by Google to make it work for our internal applications is available at https://code.google.com/p/google-mysql/. Note: I didn't update the main page on the site to reflect the changes yet, but the sources are there and the master branch points to the tip of MariaDB tree. Note #2: this tree is not an exact copy of our internal tree, and it's not intended to reflect verbatim our internal development process. It's just a set of changes split roughly one feature per commit with a grouping of several commits related to the same feature set into branches. That means that whenever we find a bug in some of our commits we'll squash the fix in the appropriate place rebuilding the whole git tree on top of that. As a consequence of that links to specific commits will not last forever, though links to specific branches likely will. Note #3: unfortunately code.google.com doesn't have a good visualization of branches in the git tree, so it's not easy to see which commits belong to which branch in our structure of branches stacked on top of each other. So the easiest way to see that is to clone the tree on your local disk and run gitk on it (or other tool that you find more convenient). Besides raising a general awareness of existence of our opensource tree, I'd like to draw attention of MariaDB developers especially to the following branches. I think many commits from there can be considered for inclusion into MariaDB proper. 16-encryption https://code.google.com/p/google-mysql/source/list?name=mariadb-10.0.12%2F1… Encryption of different subsystems writing to disk (not all subsystems included yet, but will be soon). 09-rpl https://code.google.com/p/google-mysql/source/list?name=mariadb-10.0.12%2F0… Replication-related fixes and improvements. 01-base https://code.google.com/p/google-mysql/source/list?name=mariadb-10.0.12%2F0… Mostly basic compilation and testing fixes. 03-privileges https://code.google.com/p/google-mysql/source/list?name=mariadb-10.0.12%2F0… Fixes for some quirks in privilege subsystem (aka sql_acl.cc). If you want to discuss any changes that you see in the tree feel free to reply to this email or start a separate email thread. Thank you, Pavel

2 1

[Maria-developers] Some patches for the C client library
by Daniel Schepler 02 Dec '14

02 Dec '14

While I was creating Debian packages for the "MariaDB Client Library for C" I came across several minor issues that I corrected. I thought I'd post the patches here in case you might be interested in integrating them; I'd have no problem with contributing them under the MCA if that's necessary (i.e. if they're even big enough to require such a thing). Each one has a fairly complete description of its purpose at the start. I've also noticed that mariadb_config --libs prints out something like ... -l-lz if WITH_EXTERNAL_ZLIB=ON. However, for the Debian packages I instead patched mariadb_config to remove the indirect dependencies altogether, as we generally do, so I didn't write a fix for the -l-lz issue. Also, it seems that if I try to build with WITH_SQLITE=ON then I get a build error, so I haven't enabled that in the Debian packages. -- Daniel Schepler

2 2