- developers - lists.mariadb.org

Re: [Maria-developers] Bug in trx_group_commit_leader()
by Kristian Nielsen 21 Jan '13

21 Jan '13

Michael Widenius <monty(a)askmonty.org> writes: > I noticed this compiler error on one of the build machines for 10.0-base: > > /home/buildbot/buildbot/build/mariadb-10.0.0/sql/log.cc:6564:40: > warning: ‘last_in_queue’ may be used uninitialized in this function > [-Wuninitialized] Thanks. I've pushed the fix to 10.0-base. > Looking at the code, this looks like a real potential bug. Yes (though likely dead code, as there's a DBUG_ASSERT before the if () asserting that it is always true). - Kristian.

1 0

[Maria-developers] review of MDEV-4011 per thread memory counting and usage
by Sergei Golubchik 18 Jan '13

18 Jan '13

Hi. Here it is: > === modified file 'client/completion_hash.cc' > --- client/completion_hash.cc 2011-06-30 15:46:53 +0000 > +++ client/completion_hash.cc 2013-01-15 21:00:33 +0000 > @@ -49,7 +49,7 @@ int completion_hash_init(HashTable *ht, > ht->initialized = 0; > return FAILURE; > } > - init_alloc_root(&ht->mem_root, 8192, 0); > + init_alloc_root(&ht->mem_root, 8192, 0, 0); very often you write 0 or MY_THREAD_SPECIFIC instead of MYF(0) and MYF(MY_THREAD_SPECIFIC) > ht->pHashFunction = hashpjw; > ht->nTableSize = nSize; > ht->initialized = 1; > === modified file 'include/my_sys.h' > --- include/my_sys.h 2012-12-23 21:37:11 +0000 > +++ include/my_sys.h 2013-01-15 21:00:33 +0000 > @@ -86,6 +86,8 @@ typedef struct my_aio_result { > #define MY_SYNC 4096 /* my_copy(): sync dst file */ > #define MY_SYNC_DIR 32768 /* my_create/delete/rename: sync directory */ > #define MY_SYNC_FILESIZE 65536 /* my_sync(): safe sync when file is extended */ > +#define MY_THREAD_SPECIFIC 0x10000 /* my_malloc(): thread specific */ > +#define MY_THREAD_MOVE 0x20000 /* realloc(); Memory can move */ may be, better to remove this, as it's unused? it's easier to add it later (just a couple of lines of code), than to keep the dead code around. > #define MY_CHECK_ERROR 1 /* Params to my_end; Check open-close */ > #define MY_GIVE_INFO 2 /* Give time info about process*/ > @@ -148,6 +150,42 @@ typedef struct my_aio_result { > /* Extra length needed for filename if one calls my_create_backup_name */ > #define MY_BACKUP_NAME_EXTRA_LENGTH 17 > > +/* If we have our own safemalloc (for debugging) */ > +#if defined(SAFEMALLOC) > +#define MALLOC_SIZE_AND_FLAG(p,b) sf_malloc_usable_size(p,b) > +#define MALLOC_PREFIX_SIZE 0 > +#define MALLOC_STORE_SIZE(a,b,c,d) > +#define MALLOC_FIX_POINTER_FOR_FREE(a) a > +#define SAFEMALLOC_REPORT_MEMORY(X) sf_report_leaked_memory(X) > +void sf_report_leaked_memory(my_thread_id id); > +extern my_thread_id (*sf_malloc_dbug_id)(void); > +#else > +/* > + * We use double as prefix size as this guarantees the correct > + * alignment on all platforms and will optimize things for > + * memcpy(), memcmp() etc. > + */ > +#define MALLOC_PREFIX_SIZE (sizeof(double)) > +#define MALLOC_SIZE(p) (*(size_t*) ((char*)(p) - MALLOC_PREFIX_SIZE)) > +#define MALLOC_STORE_SIZE(p, type_of_p, size, flag) \ > +{\ > + *(size_t*) p= (size) | (flag); \ > + (p)= (type_of_p) (((char*) (p)) + MALLOC_PREFIX_SIZE); \ > +} > +static inline size_t malloc_size_and_flag(void *p, myf *flags) > +{ > + size_t size= MALLOC_SIZE(p); > + *flags= (size & 1); > + return size & ~ (ulonglong) 1; > +} > +#define MALLOC_SIZE_AND_FLAG(p,b) malloc_size_and_flag(p, b); > +#define MALLOC_FIX_POINTER_FOR_FREE(p) (((char*) (p)) - MALLOC_PREFIX_SIZE) > +#define SAFEMALLOC_REPORT_MEMORY(X) do {} while(0) > +#endif /* SAFEMALLOC */ > + > +typedef void (*MALLOC_SIZE_CB) (long long size, myf my_flags); > +extern void set_malloc_size_cb(MALLOC_SIZE_CB func); I don't think you need many of these defines here. my_sys.h is the interface header. MALLOC_STORE_SIZE and MALLOC_FIX_POINTER_FOR_FREE (for example, I didn't check the others) are only used in my_malloc.c - they should not be used elsewhere, and thus, I think, they should be defined in my_malloc.c And, frankly speaking, I wouldn't bother doing two different ways of storing the size and the flags. my_malloc can always allocate one sizeof(double) more and store its data there. Who cares that in the debug builds safemalloc duplicates that... > /* defines when allocating data */ > extern void *my_malloc(size_t Size,myf MyFlags); > extern void *my_multi_malloc(myf MyFlags, ...); > @@ -323,6 +361,7 @@ typedef struct st_dynamic_array > uint elements,max_element; > uint alloc_increment; > uint size_of_element; > + myf malloc_flags; hm. you've preserved the MEM_ROOT structure for compatibiity reasons, but you've changed DYNAMIC_ARRAY. Why? I'd expect you either to change everything or to change nothing. Besides, what's the point of preserving MEM_ROOT, if you change prototypes of functions that work with it? > } DYNAMIC_ARRAY; > > typedef struct st_my_tmpdir > @@ -768,16 +807,13 @@ extern my_bool real_open_cached_file(IO_ > extern void close_cached_file(IO_CACHE *cache); > File create_temp_file(char *to, const char *dir, const char *pfx, > int mode, myf MyFlags); > -#define my_init_dynamic_array(A,B,C,D) init_dynamic_array2(A,B,NULL,C,D) > -#define my_init_dynamic_array_ci(A,B,C,D) init_dynamic_array2(A,B,NULL,C,D) > -#define my_init_dynamic_array2(A,B,C,D,E) init_dynamic_array2(A,B,C,D,E) > -#define my_init_dynamic_array2_ci(A,B,C,D,E) init_dynamic_array2(A,B,C,D,E) > +#define my_init_dynamic_array(A,B,C,D,E) init_dynamic_array2(A,B,NULL,C,D,E) > +#define my_init_dynamic_array_ci(A,B,C,D,E) init_dynamic_array2(A,B,NULL,C,D,E) > +#define my_init_dynamic_array2(A,B,C,D,E,F) init_dynamic_array2(A,B,C,D,E,F) > +#define my_init_dynamic_array2_ci(A,B,C,D,E,F) init_dynamic_array2(A,B,C,D,E,F) as you're changing function prototypes anyway, you can remove the _ci functions and all other "preserve the ABI" garbage, > extern my_bool init_dynamic_array2(DYNAMIC_ARRAY *array, uint element_size, > void *init_buffer, uint init_alloc, > - uint alloc_increment); > -/* init_dynamic_array() function is deprecated */ > -extern my_bool init_dynamic_array(DYNAMIC_ARRAY *array, uint element_size, > - uint init_alloc, uint alloc_increment); > + uint alloc_increment, myf my_flags); > extern my_bool insert_dynamic(DYNAMIC_ARRAY *array, const uchar * element); > extern uchar *alloc_dynamic(DYNAMIC_ARRAY *array); > extern uchar *pop_dynamic(DYNAMIC_ARRAY*); > @@ -829,7 +865,8 @@ extern void my_free_lock(void *ptr); > #define ALLOC_ROOT_MIN_BLOCK_SIZE (MALLOC_OVERHEAD + sizeof(USED_MEM) + 8) > #define clear_alloc_root(A) do { (A)->free= (A)->used= (A)->pre_alloc= 0; (A)->min_malloc=0;} while(0) > extern void init_alloc_root(MEM_ROOT *mem_root, size_t block_size, > - size_t pre_alloc_size); > + size_t pre_alloc_size, myf my_flags); > +extern void alloc_root_set_min_malloc(MEM_ROOT *mem_root, size_t min_malloc); if you'd store the flag in the lowest bit of the block_size or if you wouldn't try to keep the old MEM_ROOT structure, this function wouldn't be needed > extern void *alloc_root(MEM_ROOT *mem_root, size_t Size); > extern void *multi_alloc_root(MEM_ROOT *mem_root, ...); > extern void free_root(MEM_ROOT *root, myf MyFLAGS); > === modified file 'include/my_tree.h' > --- include/my_tree.h 2011-11-03 18:17:05 +0000 > +++ include/my_tree.h 2013-01-15 21:00:33 +0000 > @@ -68,13 +68,15 @@ typedef struct st_tree { > MEM_ROOT mem_root; > my_bool with_delete; > tree_element_free free; > + myf malloc_flags; > uint flag; > } TREE; > > /* Functions on whole tree */ > void init_tree(TREE *tree, size_t default_alloc_size, size_t memory_limit, > int size, qsort_cmp2 compare, my_bool with_delete, > - tree_element_free free_element, void *custom_arg); > + tree_element_free free_element, void *custom_arg, > + myf my_flags_for_malloc); I'd just call it my_flags. it doesn't matter what tree is doing with them. and there may be other flags too. E.g. you could've removed 'with_delete' and added MY_TREE_WITH_DELETE > void delete_tree(TREE*); > void reset_tree(TREE*); > > === modified file 'include/mysql.h' > --- include/mysql.h 2012-11-20 14:24:39 +0000 > +++ include/mysql.h 2013-01-15 21:00:33 +0000 > @@ -167,7 +167,7 @@ enum mysql_option > MYSQL_OPT_GUESS_CONNECTION, MYSQL_SET_CLIENT_IP, MYSQL_SECURE_AUTH, > MYSQL_REPORT_DATA_TRUNCATION, MYSQL_OPT_RECONNECT, > MYSQL_OPT_SSL_VERIFY_SERVER_CERT, MYSQL_PLUGIN_DIR, MYSQL_DEFAULT_AUTH, > - MYSQL_PROGRESS_CALLBACK, > + MYSQL_PROGRESS_CALLBACK, MYSQL_THREAD_SPECIFIC_MALLOC, I don't think you need this on the client side. (the only use case is federated, and I don't think you need to extend the client API specifically for it, federated can set whatever it needs directliy). > /* MariaDB options */ > MYSQL_OPT_NONBLOCK=6000 > }; > === modified file 'mysys/array.c' > --- mysys/array.c 2012-01-13 14:50:02 +0000 > +++ mysys/array.c 2013-01-15 21:00:33 +0000 > @@ -62,17 +64,19 @@ my_bool init_dynamic_array2(DYNAMIC_ARRA > should not throw an error > */ > if (init_alloc && > - !(array->buffer= (uchar*) my_malloc(element_size*init_alloc, MYF(0)))) > + !(array->buffer= (uchar*) my_malloc(element_size*init_alloc, > + MYF(my_flags)))) > array->max_element=0; > DBUG_RETURN(FALSE); > } > > my_bool init_dynamic_array(DYNAMIC_ARRAY *array, uint element_size, > - uint init_alloc, uint alloc_increment) > + uint init_alloc, uint alloc_increment, > + myf my_flags) > { > /* placeholder to preserve ABI */ > return my_init_dynamic_array_ci(array, element_size, init_alloc, You can remove this, as you're changing the ABI anyway > - alloc_increment); > + alloc_increment, my_flags); > } > /* > Insert element at the end of array. Allocate memory if needed. > === modified file 'mysys/my_alloc.c' > --- mysys/my_alloc.c 2012-01-13 14:50:02 +0000 > +++ mysys/my_alloc.c 2013-01-15 21:00:33 +0000 > @@ -41,16 +48,20 @@ > Altough error can happen during execution of this function if > pre_alloc_size is non-0 it won't be reported. Instead it will be > reported as error in first alloc_root() on this memory root. > + > + We don't want to change the api for MEMROOT. > + Becasue of this, we store in min_malloc of my_thread_specific is set. > */ > > void init_alloc_root(MEM_ROOT *mem_root, size_t block_size, > - size_t pre_alloc_size __attribute__((unused))) > + size_t pre_alloc_size __attribute__((unused)), > + myf my_flags) > { > DBUG_ENTER("init_alloc_root"); > DBUG_PRINT("enter",("root: 0x%lx", (long) mem_root)); > > mem_root->free= mem_root->used= mem_root->pre_alloc= 0; > - mem_root->min_malloc= 32; > + mem_root->min_malloc= 32 | (test(my_flags & MY_THREAD_SPECIFIC) << 16); it'd be safer to put it in the lowest bit. min_malloc or - particularly - block size, are always even numbers, the lowest bit is always 0. or to add a new field to the MEM_ROOT, as compatibility is already broken anyway. > mem_root->block_size= block_size - ALLOC_ROOT_MIN_BLOCK_SIZE; > mem_root->error_handler= 0; > mem_root->block_num= 4; /* We shift this with >>2 */ > === modified file 'mysys/my_malloc.c' > --- mysys/my_malloc.c 2012-01-13 14:50:02 +0000 > +++ mysys/my_malloc.c 2013-01-15 21:00:33 +0000 > @@ -18,6 +18,31 @@ > #include "mysys_err.h" > #include <m_string.h> > > +MALLOC_SIZE_CB malloc_size_cb_func= NULL; > + > +/** > + Inform appliction that memory usage has changed application > + > + @param size Size of memory segment allocated or freed > + @param flag 1 if thread specific (allocated by MY_THREAD_SPECIFIC), > + 0 if system specific. > + > + The type os size is long long, to be able to handle negative numbers to > + decrement the memory usage > +*/ > + > +static void update_malloc_size(long long size, myf my_flags) > +{ > + if (malloc_size_cb_func) > + malloc_size_cb_func(size, my_flags); > +} > + > +void set_malloc_size_cb(MALLOC_SIZE_CB func) > +{ > + malloc_size_cb_func= func; > +} > + > + > /** > Allocate a sized block of memory. > > === modified file 'mysys/mysys_priv.h' > --- mysys/mysys_priv.h 2011-12-12 21:58:24 +0000 > +++ mysys/mysys_priv.h 2013-01-15 21:00:33 +0000 > @@ -70,12 +70,13 @@ extern PSI_file_key key_file_charset, ke > #endif /* HAVE_PSI_INTERFACE */ > > #ifdef SAFEMALLOC > -void *sf_malloc(size_t size); > -void *sf_realloc(void *ptr, size_t size); > +void *sf_malloc(size_t size, myf my_flags); > +void *sf_realloc(void *ptr, size_t size, myf my_flags); > void sf_free(void *ptr); > +size_t sf_malloc_usable_size(void *ptr, myf *my_flags); this is used nowhere. dead code again. > #else > -#define sf_malloc(X) malloc(X) > -#define sf_realloc(X,Y) realloc(X,Y) > +#define sf_malloc(X,Y) malloc(X) > +#define sf_realloc(X,Y,Z) realloc(X,Y) > #define sf_free(X) free(X) > #endif > > === modified file 'mysys/safemalloc.c' > --- mysys/safemalloc.c 2012-04-18 01:29:26 +0000 > +++ mysys/safemalloc.c 2013-01-15 21:00:33 +0000 > @@ -79,11 +81,21 @@ static int bad_ptr(const char *where, vo > static void free_memory(void *ptr); > static void sf_terminate(); > > +/* Setup default call to get a thread id for the memory */ > + > +my_thread_id default_sf_malloc_dbug_id(void) > +{ > + return my_thread_dbug_id(); > +} > + > +my_thread_id (*sf_malloc_dbug_id)(void)= default_sf_malloc_dbug_id; Why do you need different functions for getting the thread id? > + > + > /** > allocates memory > */ > > -void *sf_malloc(size_t size) > +void *sf_malloc(size_t size, myf my_flags) > { > struct st_irem *irem; > uchar *data; > === modified file 'sql/event_scheduler.cc' > --- sql/event_scheduler.cc 2012-09-27 23:06:56 +0000 > +++ sql/event_scheduler.cc 2013-01-15 21:00:33 +0000 > @@ -182,12 +180,15 @@ deinit_event_thread(THD *thd) > void > pre_init_event_thread(THD* thd) > { > + THD *orig_thd= current_thd; > DBUG_ENTER("pre_init_event_thread"); > + > + my_pthread_setspecific_ptr(THR_THD, thd); I'd rather see set_current_thd(thd) here: #define set_current_thd(X) my_pthread_setspecific_ptr(THR_THD, (X)) > thd->client_capabilities= 0; > thd->security_ctx->master_access= 0; > thd->security_ctx->db_access= 0; > thd->security_ctx->host_or_ip= (char*)my_localhost; > - my_net_init(&thd->net, NULL); > + my_net_init(&thd->net, NULL, MY_THREAD_SPECIFIC); > thd->security_ctx->set_user((char*)"event_scheduler"); > thd->net.read_timeout= slave_net_timeout; > thd->variables.option_bits|= OPTION_AUTO_IS_NULL; > === modified file 'sql/handler.cc' > --- sql/handler.cc 2012-12-17 00:49:19 +0000 > +++ sql/handler.cc 2013-01-15 21:00:33 +0000 > @@ -5395,6 +5395,7 @@ fl_log_iterator_buffer_init(struct handl > iterator->buffer= buff; > iterator->next= &fl_log_iterator_next; > iterator->destroy= &fl_log_iterator_destroy; > + my_dir_end(dirp); Why did we never see memory leaks because of that? > return HA_ITERATOR_OK; > } > > === modified file 'sql/mysqld.cc' > --- sql/mysqld.cc 2013-01-09 03:34:33 +0000 > +++ sql/mysqld.cc 2013-01-15 21:00:33 +0000 > @@ -2606,9 +2607,10 @@ bool one_thread_per_connection_end(THD * > > /* It's safe to broadcast outside a lock (COND... is not deleted here) */ > DBUG_PRINT("signal", ("Broadcasting COND_thread_count")); > + mysql_cond_broadcast(&COND_thread_count); > + > DBUG_LEAVE; // Must match DBUG_ENTER() > my_thread_end(); > - mysql_cond_broadcast(&COND_thread_count); why? > > pthread_exit(0); > return 0; // Avoid compiler warnings > @@ -7024,6 +7096,7 @@ SHOW_VAR status_vars[]= { > {"Key", (char*) &show_default_keycache, SHOW_FUNC}, > {"Last_query_cost", (char*) offsetof(STATUS_VAR, last_query_cost), SHOW_DOUBLE_STATUS}, > {"Max_used_connections", (char*) &max_used_connections, SHOW_LONG}, > + {"Memory_used", (char*) &total_memory_used, SHOW_LONGLONG}, May be this should rather be: SHOW SESSION STATUS -> thd->memory_used SHOW GLOBAL STATUS -> total_memory_used (which means that total_memory_used/memory_used should be in the status_var struct) > {"Not_flushed_delayed_rows", (char*) &delayed_rows_in_use, SHOW_LONG_NOFLUSH}, > {"Open_files", (char*) &my_file_opened, SHOW_LONG_NOFLUSH}, > {"Open_streams", (char*) &my_stream_opened, SHOW_LONG_NOFLUSH}, > === modified file 'sql/net_serv.cc' > --- sql/net_serv.cc 2012-02-28 17:53:05 +0000 > +++ sql/net_serv.cc 2013-01-15 21:00:33 +0000 > @@ -139,6 +140,7 @@ my_bool my_net_init(NET *net, Vio* vio) > net->net_skip_rest_factor= 0; > net->last_errno=0; > net->unused= 0; > + net->thread_specific_malloc= test(my_flags & MY_THREAD_SPECIFIC); why do you need that in NET ? > > if (vio != 0) /* If real connection */ > { > === modified file 'sql/sql_analyse.h' > --- sql/sql_analyse.h 2011-11-03 18:17:05 +0000 > +++ sql/sql_analyse.h 2013-01-15 21:00:33 +0000 > @@ -121,7 +121,8 @@ class field_str :public field_info > must_be_blob(0), was_zero_fill(0), > was_maybe_zerofill(0), can_be_still_num(1) > { init_tree(&tree, 0, 0, sizeof(String), (qsort_cmp2) sortcmp2, > - 0, (tree_element_free) free_string, NULL); }; > + 0, (tree_element_free) free_string, NULL, > + MY_THREAD_SPECIFIC); }; Now, when I'm thinking of it, I got the idea that it's better to have MY_THREAD_SPECIFIC as a default, and add MY_GLOBAL_ALLOC instead to mark global allocations explicitly. It's more future-proof. When one copies init_dynamic_array() from somewhere and zeros out unknown or unneeded parameters, the memory will be per-thread by default. And if that's wrong, the test suite will find it, you have an assert for this. To mark memory global as an explicit action, one needs to think about it. Currently, the memory is global by default, and if one doesn't think,# copy-pastes, and leaves new allocation global, it will not be noticed, the memory will simply be unaccounted for. > > void add(); > void get_opt_type(String*, ha_rows); > === modified file 'sql/sql_error.cc' > --- sql/sql_error.cc 2012-01-13 14:50:02 +0000 > +++ sql/sql_error.cc 2013-01-15 21:00:33 +0000 > @@ -457,25 +457,37 @@ Diagnostics_area::disable_status() > m_status= DA_DISABLED; > } > > -Warning_info::Warning_info(ulonglong warn_id_arg, bool allow_unlimited_warnings) > +Warning_info::Warning_info(ulonglong warn_id_arg, > + bool allow_unlimited_warnings, bool initialize) I'd rather add a special constructor for Warning_info that THD would use. Because there's only one place where you need initialize=false, and all other "normal" uses, should not suffer (be changed) because of that. > :m_statement_warn_count(0), > m_current_row_for_warning(1), > m_warn_id(warn_id_arg), > m_allow_unlimited_warnings(allow_unlimited_warnings), > m_read_only(FALSE) > { > - /* Initialize sub structures */ > - init_sql_alloc(&m_warn_root, WARN_ALLOC_BLOCK_SIZE, WARN_ALLOC_PREALLOC_SIZE); > m_warn_list.empty(); > bzero((char*) m_warn_count, sizeof(m_warn_count)); > + if (initialize) > + init(); > } > Regards, Sergei

2 2

Re: [Maria-developers] LP:1024058 - mysqld XA crash in replication slave
by Kristian Nielsen 18 Jan '13

18 Jan '13

"Sergei Golubchik (JIRA)" <jira(a)mariadb.atlassian.net> writes: > [ https://mariadb.atlassian.net/browse/MDEV-633?page=com.atlassian.jira.plugi… ] [Taking this to maria-developers@, which is better for discussions like this] > The problem here is very simple to explain. The server can use either > mmap-based transaction coordinator for 2PC or a binary log. 2PC always uses > binary log, if binary logging is enabled. But even if it is enabled > globally, it is usually disabled in the replication slave thread unless > --log-slave-updates is specified. > > One would probably get the same crash without replication, if one disables > binary log manually with SET SQL_LOG_BIN=0; > > Possible fixes: > * auto-enable binary log for 2PC transactions (bad, binlog will contain > unwanted DDL and DML events). > * abort 2PC transactions if binary log is locally disabled for this thread > (worse, too easy to break the replication) > * write Xid events to binlog even if binary log is locally disabled (best?) > > The last approach seems to be preferable. But in the future if we'll start > recovering transactions from the binary log (doing only one sync per 2PC > transaction), we'll have this problem again, because then we'll need the > actual changes to be logged, not just the Xid. > > [~knielsen] - opinion? First, I do not think SQL_LOG_BIN=0 or binlog enabled without --log-slave-updates are common or very useful. And multi-engine transactions are problematic in several ways. But we should not crash, of course. I would find it acceptable to simply call each engine's commit method separately, without 2PC, especially in stable 5.5 and below. Note that we now have @@skip_replication, which can be used to log a transaction to binlog without replicating it, similar to @@sql_log_bin but maintaining 2PC and recovery. If we really want, we could fix it 10.0 so that 2PC works even for binlog disabled. As you say, probably writing the Xid to the binlog without any associated DML events would be a reasonable way to do this. I would suggest logging a separate new event to not confuse things with XID_LOG_EVENT, which implies a commit of a prior transactions (there are facilities in 10.0 to rewrite such event to a dummy event so we will not break old slaves). With respect to a future recovering transactions from the binary log, it is actually in any case very hard to recover a multi-engine transaction in this way. The problem is when one engine is missing the transaction but the other engine has it, and the event is logged with statement-based replication. But we could simply not do this optimisation for multi-engine transactions, which are really not very commonly used (for multi-engine transactions we would keep the fsync() inside prepare()). Then things should work, for the normal case as well as for binlog locally disabled. So in summary, my immediate opinion would be: - In 5.5, commit each engine individually without 2PC when binlog is locally disabled. - In 10.0, optionally binlog just a new XID_RECOVERY_LOG_EVENT and use it in recovery if binlog is locally disabled. Hope this helps, - Kristian.

1 0

Re: [Maria-developers] Structure of Binary Log
by Kristian Nielsen 16 Jan '13

16 Jan '13

Nuno Rebocho <rebocho.nmg(a)portugalmail.pt> writes: > The attractive hypothesis, develop a data recovery tool, never left my > mind. I've been trying to learn together as a "Maria developers" team > can get information about the structure of the log file from MySql to > navigate this and obtain information needed for recovery. I am not aware of any definitive manual for the MySQL/MariaDB binary log, however there are various resources out there. For example, I wrote two articles that give an overview of the events in the log: http://kristiannielsen.livejournal.com/13382.html http://kristiannielsen.livejournal.com/13253.html My articles on group commit on the same blog would give you lots on information on how the binary log is written. Google can probably find other articles by other authors that might be useful. Otherwise, the source code in the definite source of information. The binary log code is in sql/log.cc. The code for creating and parsing events is in sql/log_event.cc. I noticed you are now on the maria-developers@ mailing list, this will be a good place to ask concrete questions about the binary log. Good luck with your project! - Kristian.

1 0

Re: [Maria-developers] 答复: in-order commit
by Kristian Nielsen 16 Jan '13

16 Jan '13

丁奇 <dingqi.lxb(a)taobao.com> writes: > Hi, Kristian > Ok. I have got the information from JIRA. > > I find you control the commit order inside the user thread. > > Will it be easier to let Trans_worker thread hold this logic? Yes, I think you are right. Of course, the user thread is the one that knows the ordering, but the logic for waiting needs to be in the Trans_worker thread. In fact this is a bug in my first patch: Transaction T3 could wait for the THD of worker thread 1 which has both T1 and T2 queued; then it will wake up too early, when T1 commits rather than when T2 does. I will try to implement the new idea today. > After they have done the execution of one transaction, "register the transaction and wait" if there are transactions from other workers should be commited ahead. > After commit in one worker, wake up another worker, the worker who wait for the next "head of commitee" should be woken up. Right, I'll need to look into this a bit deeper. Actually, in my patch the actual wait and wakeup happens inside ha_commit_trans(), and there is a reason for this. Because eventually I want to do it inside tc_log->log_and_order(), which is called from ha_commit_trans(). Here is how a commit happens: InnoDB prepare step fsync() InnoDB redo log (*A) TC_LOG_BINLOG::log_and_order Write transaction to binlog fsync() binlog (*B) InnoDB commit_ordered() (*C) Write commit record to InnoDB redo log InnoDB commit step The steps (*A) and (*B) are slow, typically around 1-10 milliseconds depending on disk system. So we need many threads to commit in parallel and reach points (*A) and (*B) at the same time, so we only need to do the fsync() once for many threads. This is group commit. Thus for in-order parallel replication, we must not do the wait for the previous commit before the (*B) step. Because if we do, then it becomes impossible for two transactions to be at point (*B) at the same time, and group commit is impossible. On the other hand, point (*C) is where the commit order is determined. So if we do the wait after point (*C), then we cannot enforce that T1 commits before T2. So therefore, the wait must happen exactly around point (B) and (C), inside TC_LOG_BINLOG::log_and_order(). That is why I invented all the register_wait_for_prior_commit() and so on: so that log_and_order() has somewhere to look for exactly who is waiting for who. Then if T2 is waiting for T1 to commit, we can do steps (*B) and (*C) for both of them together, achiving both group commit and in-order parallel replication. Anyway, I just wanted to mention this, I know it will be difficult to understand fully from just this description. This is something that I have been planning to have for years, but I still need to show some real code that actually works. If I manage that, hopefully things will be clearer. (If not - then I need to think again ;-) Thanks, - Kristian.

1 2

[Maria-developers] new timestamp fields
by Zardosht Kasheff 15 Jan '13

15 Jan '13

Hello all, In MySQL/MariaDB 5.1, when a timestamp field was used as a key, we used an integer compare to compare the fields. We assumed all integer fields (including date, time, and timestamp) could use an integer comparison to compare the fields, and that they were either 1, 2, 3, 4 or 8 bytes long. We now see in MariaDB 5.3 and beyond that changes have been made to the timestamp field, such that the length of the field may be something other than 1, 2, 3, 4,or 8 bytes. Given a timestamp field of N bytes, how are we meant to compare them? Are some meant to be integer comparisons? Are others supposed to be memory comparisons? Thanks -Zardosht

2 5

[Maria-developers] MDEV-520: consider parallel replication patch from taobao patches
by Kristian Nielsen 14 Jan '13

14 Jan '13

Hi xiaobin, I read through your patch found here: http://mysql.taobao.org/index.php/RLPR_for_MariaDB I am still a bit confused about the status of this patch. It would be useful if you could explain a bit about the status of the patch: - How have you tested the patch. - How are you currently using the patch? Or is it still just development/test? - What are your current plans for the patch? (You mentioned that you have some changes you are implementing at the moment). Anyway, I think I have a fair understanding of the overall idea, which seems very interesting! I will try to summarise here in a bit more detail, as well as give a few general comments on the patch. Please correct me if I misunderstood anything. ---- The basic idea is to build a set (implemented as a hash table) of every unique key in every row-event in each transaction received from the master. If two transactions have no overlapping keys, they can be applied in parallel (foreign_key_checks is set to OFF to make this work). There is a single thread that processes each event received in sequence. It checks the key set of the new transaction against the key sets of all transactions queued for each of N worker threads: - If there is no conflict with any already queued transaction, the new transaction can be queued to any worker thread (the one with fewest queued transaction is chosen). - If there is a conflict with just one thread, then the transaction can be queued for that thread - it will then execute in correct sequence with the conflicting transactions. - If there is a conflict with multiple threads, wait until the conflicting transactions have been processed. No new transactions are queued while waiting. Then there are N (default 16) threads that apply the queued transactions. Queued transactions are processsed in parallel within one thread, but transaction in different queues are processed in parallel with each other. If a transaction is received which is not using row events (statement-based replication or DDL), then we wait until all queued transactions are processed. Only after that is the new transaction queued, so that such events are processed correctly (but not in parallel, of course). Likewise, if the worker threads get too far behind, we wait with queuing any more transactions. ---- I think this is a very interesting approach, and I am happy that someone have actually tried implementing it. The biggest potential issue seems to be the performance of the thread that checks the transactions for conflicts and queues them for the appropriate thread: - Main issue will be huge transactions, if the size of hash tables of key values become too big for available memory. Maybe this could be solved by having some limit above which transactions are not tried in parallel. - There will be a lot of hash lookups of key values, all done in a single thread. This could become a limit I suppose - testing will be needed to understand this fully. But I think there will be many workloads where this will be useful. Overall, it seems to me this technique is very promising. It seems to be better at getting maximum parallelism on the slave than any of the other approaches that have been considered. As to the patch itself, it seems a bit immature in the present form, but I understood you are still working on it. Let us discuss what the plans could be for finishing it and get it ready for inclusion in MariaDB. The main criteria is that it must be reasonably complete - it must not crash or silently corrupt data in any usage. However, reasonable restrictions for what is supported is fine. (For example, we could say that LOAD DATA is not supported and give an error, but not crash or silently ignore such statement). But we can discuss this in more details. ---- Finally, a couple general comments on the code: 1. get_pk_value() does not seem to handle collations, eg. case insensitivity? It needs to understand that eg. "abab" and "ABAB" can conflict. I think this should be solved by copying in values with non-binary collations using the appropriate strxfrm() variant. 2. remote_query() retries the query in case of deadlock - but that seems wrrong, doesn't it need to retry the whole _transaction_, not just the query? 3. What about other events like User_var_event ? Will they be included in a later patch? 4. There is a my_sleep(10000) in the code when a transaction conflicts with two or more workers. It would be better to replace this with a condition variable so we can wake up as soon as the conflict is resolved. Same for some other sleeps like in wait_for_all_dml_done(). 5. Shouldn't there be some locking in dispatch_transaction()? To protect the hash lookups when checking conflicts against worker threads doing pk_hash_minus() ? 6. get_pk_value(). There seems to be a buffer overflow - st_hash_item::key is fixed size, but there seems no check against overrunning it. One solution is to just truncate if data does not fit - this can lead to false positives where potential parallelism is not exploited, but that is still safe and should be ok. Thanks! - Kristian.

5 21

Re: [Maria-developers] MDEV-318 IF (NOT) EXIST clauses for ALTER TABLE
by Sergei Golubchik 13 Jan '13

13 Jan '13

Hi, Holyfoot! On Jul 08, holyfoot(a)askmonty.org wrote: > At file:///home/hf/wmar/mdev-318/ > > ------------------------------------------------------------ > revno: 3552 > revision-id: holyfoot(a)askmonty.org-20120708152354-d7iu7yoqo3cohidz > parent: sanja(a)montyprogram.com-20120626184334-ptpg39ptq3t79dg5 > committer: Alexey Botchkov <holyfoot(a)askmonty.org> > branch nick: mdev-318 > timestamp: Sun 2012-07-08 20:23:54 +0500 > message: > MDEV-318 IF (NOT) EXIST clauses for ALTER TABLE (MWL #252). > ALTER TABLE syntax modified and related implementations added to allow: > ALTER TABLE ADD/DROP COLUMN > ALTER TABLE ADD/DROP INDEX > ALTER TABLE ADD/DROP FOREIGN KEY > ALTER TABLE ADD/DROP PARTITION > ALTER TABLE CHANGE COLUMN > ALTER TABLE MODIFY COLUMN Please, find my comments below: > === modified file 'mysql-test/r/alter_table.result' > --- mysql-test/r/alter_table.result 2011-11-21 17:13:14 +0000 > +++ mysql-test/r/alter_table.result 2012-07-11 14:24:32 +0000 > @@ -1282,3 +1282,30 @@ > # Clean-up. > drop table t1; > End of 5.1 tests > +CREATE TABLE t1 ( > +id INT(11) NOT NULL, > +x_param INT(11) DEFAULT NULL, > +PRIMARY KEY (id), > +KEY x_param (x_param) > +); > +ALTER TABLE t1 ADD COLUMN IF NOT EXISTS id INT, > +ADD COLUMN IF NOT EXISTS lol INT AFTER id; > +Warnings: > +Note 1060 Duplicate column name 'id' > +ALTER TABLE t1 ADD COLUMN IF NOT EXISTS lol INT AFTER id; > +Warnings: > +Note 1060 Duplicate column name 'lol' > +ALTER TABLE t1 DROP COLUMN IF EXISTS lol; > +ALTER TABLE t1 DROP COLUMN IF EXISTS lol; > +Warnings: > +Note 1091 Can't DROP 'lol'; check that column/key exists > +ALTER TABLE t1 ADD KEY IF NOT EXISTS x_param(x_param); > +Warnings: > +Note 1061 Duplicate key name 'x_param' > +ALTER TABLE t1 ADD KEY IF NOT EXISTS x_param(x_param); typo? you test ADD KEY IF NOT EXISTS twice. > +Warnings: > +Note 1061 Duplicate key name 'x_param' > +ALTER TABLE t1 MODIFY IF EXISTS lol INT; > +Warnings: > +Note 1054 Unknown column 'lol' in 't1' > +DROP TABLE t1; > === modified file 'sql/field.cc' > --- sql/field.cc 2012-03-13 14:38:43 +0000 > +++ sql/field.cc 2012-07-11 14:28:40 +0000 > @@ -9182,6 +9182,7 @@ void Create_field::init_for_tmp_table(en First: please, see the last section in http://kb.askmonty.org/en/how-to-get-more-out-of-bzr-when-working-on-mariad… and do as it suggests I had to apply your patch locally and re-run bzr diff to get the function names in the diff :( > (maybe_null ? FIELDFLAG_MAYBE_NULL : 0) | > (is_unsigned ? 0 : FIELDFLAG_DECIMAL)); > vcol_info= 0; > + create_if_not_exists= FALSE; > stored_in_db= TRUE; > } > > === modified file 'sql/sql_lex.h' > --- sql/sql_lex.h 2011-12-11 16:39:33 +0000 > +++ sql/sql_lex.h 2012-07-11 14:28:40 +0000 > @@ -1816,6 +1816,7 @@ typedef struct st_lex : public Query_tab > uint16 create_view_algorithm; > uint8 create_view_check; > bool drop_if_exists, drop_temporary, local_file, one_shot_set; > + bool check_exists; /* Enabled if the IF [NOT] EXISTS specified for ALTER */ I think, you can remove drop_if_exists and use check_exists instead > bool autocommit; > bool verbose, no_write_to_binlog; > > === modified file 'sql/sql_yacc.yy' > --- sql/sql_yacc.yy 2012-06-10 09:53:06 +0000 > +++ sql/sql_yacc.yy 2012-07-11 14:28:40 +0000 > @@ -4656,8 +4659,16 @@ table_option: > ; > > opt_if_not_exists: > - /* empty */ { $$= 0; } > - | IF not EXISTS { $$=HA_LEX_CREATE_IF_NOT_EXISTS; } > + /* empty */ > + { > + Lex->check_exists= FALSE; Let's initialize check_exists either before the grammar is parsed (like in the add_create_index_prepare and in the create rule) of in the grammar (here, in the opt_if_not_exists). But don't do it twice. > + $$= 0; > + } > + | IF not EXISTS > + { > + Lex->check_exists= TRUE; > + $$=HA_LEX_CREATE_IF_NOT_EXISTS; > + } > ; > > opt_create_table_options: > @@ -5879,6 +5891,18 @@ opt_ident: > | field_ident { $$=$1.str; } > ; > > +opt_if_not_exists_ident: > + opt_if_not_exists opt_ident > + { > + LEX *lex= Lex; > + if (lex->check_exists && lex->sql_command != SQLCOM_ALTER_TABLE) > + { > + my_parse_error(ER(ER_SYNTAX_ERROR)); > + MYSQL_YYABORT; > + } > + $$= $2; > + }; > + why did you create opt_if_not_exists_ident rule, instead of adding opt_if_not_exists before ident in the alter table grammar? > opt_component: > /* empty */ { $$= null_lex_str; } > | '.' ident { $$= $2; } > @@ -6455,6 +6482,11 @@ opt_column: > | COLUMN_SYM {} > ; > > +opt_column_if_exists: > + opt_column if_exists > + {} > + ; > + same as above - why a special rule? > opt_ignore: > /* empty */ { Lex->ignore= 0;} > | IGNORE_SYM { Lex->ignore= 1;} > @@ -10096,8 +10128,16 @@ table_alias_ref: > ; > > if_exists: please, rename this old rule to opt_if_exists > - /* empty */ { $$= 0; } > - | IF EXISTS { $$= 1; } > + /* empty */ > + { > + Lex->check_exists= FALSE; > + $$= 0; > + } > + | IF EXISTS > + { > + Lex->check_exists= TRUE; > + $$= 1; > + } > ; > > opt_temporary: you didn't change CREATE INDEX and DROP INDEX, did you? and ALTER TABLE DROP PRIMARY KEY isn't covered either > === modified file 'sql/sql_table.cc' > --- sql/sql_table.cc 2012-04-05 21:07:18 +0000 > +++ sql/sql_table.cc 2012-07-11 14:28:40 +0000 > @@ -5788,6 +5788,226 @@ is_index_maintenance_unique (TABLE *tabl > > > /* > + Preparation for table creation > + > + SYNOPSIS > + handle_if_exists_option() Why did you do that in a separate function? You have basically duplicated all checks. normal alter table code checks for duplicate columns/key names and for non-existant columns/keys for DROP. this code is still in place after your patch. so, basically, you'll check for duplicates and non-existant names twice. what's the point of that? I'd expect you to take check_exists flag into account in the existing checking code and convert errors into warnings as appropriate. > + thd Thread object. > + table The altered table. > + alter_info List of columns and indexes to create > + > + DESCRIPTION > + Looks for the IF [NOT] EXISTS options, checks the states and remove items > + from the list if existing found. > + > + RETURN VALUES > + NONE > +*/ > + > +static void > +handle_if_exists_options(THD *thd, TABLE *table, Alter_info *alter_info) > +{ > + Field **f_ptr; Regards, Sergei

2 9

[Maria-developers] Order by speed optimization
by Matthew Lagoe 13 Jan '13

13 Jan '13

I was talking with a few people about a query such as select * from table order by X, Y, Z limit G; > > As the "ORDER BY" for me could change to any number of 12 values in any order I am not able to use a multicolumn index as such I was looking at how to optimize in such case where sorting is dynamic. On my dataset the query takes ~7 seconds for a full table scan but only .3 seconds when using the column index and returns ~80,000 records for the query (before the LIMIT) I was wondering if there is any reason that the order by syntax doesn't use the following logic, I was thinking of looking at the code but was told such a simple optimization must have a reason that its not already in the code, so I am asking here before I spend hours being stupid. This logic block will be based on the above query. 1. If index exists for column X and there is a limit on the query then use the index and sort the list, then return the first G values, continue past G until you hit a different value for X then the value that was at G 2. Use this smaller subset of data for then ordering the data based on Y 3. Order the data again based on Z 4. Return the sorted data like normal If there is a reason this optimization isn't in the code please someone let me know, if not I think I will take a look at adding it. -Botanic

2 2

[Maria-developers] Kubuntu 12.04.1 and MariaDB
by Giacomo Picchiarelli 11 Jan '13

11 Jan '13

Hi! I'm having troubles installing mariadb 10.0 in Kubuntu 12.04.1. When I try to install it, dependencies are getting enraged and apt (through synaptic or muon) suggests to remove kde-plasma-desktop. This happens when I try to install mariadb-server-core 10.0. Of course if I remove kde-plasma-desktop I can't use my kde desktop, because it depends on mysql packages. I was trying to install it for educational purpose, and try to build and make some tests. Do you have any suggestions? Thank you -- Giacomo Picchiarelli <gpicchiarelli(a)gmail.com>

3 3