- developers - lists.mariadb.org

[Maria-developers] Fixes for an issue with my_atomic
by Kristian Nielsen 09 Jun '10

09 Jun '10

Hi, I had a problem with my group commit patch, and tracked it down to a problem in my_atomic. The issue is that my_atomic_cas*(val, cmp, new) accesses *cmp after successful CAS operation (in one place reading it, in another place writing it). Here is the fix: Index: work-5.1-groupcommit/include/atomic/gcc_builtins.h =================================================================== --- work-5.1-groupcommit.orig/include/atomic/gcc_builtins.h 2010-06-09 11:53:59.000000000 +0200 +++ work-5.1-groupcommit/include/atomic/gcc_builtins.h 2010-06-09 11:54:06.000000000 +0200 @@ -19,8 +19,9 @@ v= __sync_lock_test_and_set(a, v); #define make_atomic_cas_body(S) \ int ## S sav; \ - sav= __sync_val_compare_and_swap(a, *cmp, set); \ - if (!(ret= (sav == *cmp))) *cmp= sav; + int ## S cmp_val= *cmp; \ + sav= __sync_val_compare_and_swap(a, cmp_val, set);\ + if (!(ret= (sav == cmp_val))) *cmp= sav #ifdef MY_ATOMIC_MODE_DUMMY #define make_atomic_load_body(S) ret= *a Index: work-5.1-groupcommit/include/atomic/x86-gcc.h =================================================================== --- work-5.1-groupcommit.orig/include/atomic/x86-gcc.h 2010-06-09 11:53:59.000000000 +0200 +++ work-5.1-groupcommit/include/atomic/x86-gcc.h 2010-06-09 11:54:06.000000000 +0200 @@ -45,8 +45,12 @@ #define make_atomic_fas_body(S) \ asm volatile ("xchg %0, %1;" : "+r" (v) , "+m" (*a)) #define make_atomic_cas_body(S) \ + int ## S sav; \ asm volatile (LOCK_prefix "; cmpxchg %3, %0; setz %2;" \ - : "+m" (*a), "+a" (*cmp), "=q" (ret): "r" (set)) + : "+m" (*a), "=a" (sav), "=q" (ret) \ + : "r" (set), "a" (*cmp)); \ + if (!ret) \ + *cmp= sav #ifdef MY_ATOMIC_MODE_DUMMY #define make_atomic_load_body(S) ret=*a This makes the behaviour consistent with the other implementations, see for example generic-msvc.h. (It is also pretty important for correct operation. In my code, I use my_atomic_casptr() to atomically enqueue a struct from one thread, and as soon as it is enqueued another thread may grab the struct and change it, including the field pointed to by cmp. So it is essential that my_atomic_casptr() neither reads nor writes *cmp after successful CAS, as the above patch ensures.) It was btw. a bit funny how I tracked this down. After digging for a few hours in my own code without success I got the idea to check the CAS implementation, and spotted the problem in gcc_builtins.h. After fixing I was baffled as to why my code stil failed, until I realised I was not using gcc_builtins.h but x86-gcc.h, and found and fixed the similar problem there ;-) Now the question is, where should I push this (if at all)? Any opinions? ----------------------------------------------------------------------- While I was there, I also noticed another potential problem in gcc_builtins.h, suggesting this patch: Index: work-5.1-groupcommit/include/atomic/x86-gcc.h =================================================================== --- work-5.1-groupcommit.orig/include/atomic/x86-gcc.h 2010-06-09 11:37:12.000000000 +0200 +++ work-5.1-groupcommit/include/atomic/x86-gcc.h 2010-06-09 11:52:47.000000000 +0200 @@ -38,17 +38,31 @@ #define asm __asm__ #endif +/* + The atomic operations imply a memory barrier for the CPU, to ensure that all + prior writes are flushed from cache, and all subsequent reads reloaded into + cache. + + We need to imply a similar memory barrier for the compiler, so that all + pending stores (to memory that may be aliased in other parts of the code) + will be flushed to memory before the operation, and all reads from such + memory be re-loaded. This is achieved by adding the "memory" pseudo-register + to the clobber list, see GCC documentation for more explanation. + + The compiler and CPU memory barriers are needed to make sure changes in one + thread are made visible in another by the atomic operation. +*/ #ifndef MY_ATOMIC_NO_XADD #define make_atomic_add_body(S) \ - asm volatile (LOCK_prefix "; xadd %0, %1;" : "+r" (v) , "+m" (*a)) + asm volatile (LOCK_prefix "; xadd %0, %1;" : "+r" (v) , "+m" (*a): : "memory") #endif #define make_atomic_fas_body(S) \ - asm volatile ("xchg %0, %1;" : "+r" (v) , "+m" (*a)) + asm volatile ("xchg %0, %1;" : "+r" (v) , "+m" (*a) : : "memory") #define make_atomic_cas_body(S) \ int ## S sav; \ asm volatile (LOCK_prefix "; cmpxchg %3, %0; setz %2;" \ : "+m" (*a), "=a" (sav), "=q" (ret) \ - : "r" (set), "a" (*cmp)); \ + : "r" (set), "a" (*cmp) : "memory"); \ if (!ret) \ *cmp= sav @@ -63,9 +77,9 @@ #define make_atomic_load_body(S) \ ret=0; \ asm volatile (LOCK_prefix "; cmpxchg %2, %0" \ - : "+m" (*a), "+a" (ret): "r" (ret)) + : "+m" (*a), "+a" (ret) : "r" (ret) : "memory") #define make_atomic_store_body(S) \ - asm volatile ("; xchg %0, %1;" : "+m" (*a), "+r" (v)) + asm volatile ("; xchg %0, %1;" : "+m" (*a), "+r" (v) : : "memory") #endif /* TODO test on intel whether the below helps. on AMD it makes no difference */ The comment in the patch explains the idea I think. Basically, these memory barrier operations need to be a compiler barrier also. Otherwise there is nothing that prevents GCC from moving unrelated stores across the memory barrier operation. This is a problem for example when filling in a structure and then atomically linking it into a shared list. The CPU memory barrier ensures that the fields in the structure will be visible to other threads on the CPU level, but it is also necessary to tell GCC to keep the stores into the struct fields prior to the store linking into the shared list. (It also makes the volatile declaration on the updated memory location unnecessary, but maybe it is needed for other implementations (though it shouldn't be, volatile is almost always wrong)). - Kristian.

1 0

[Maria-developers] Patch to build xtradb and federatedx statically on Windows
by Bo Thorsen 09 Jun '10

09 Jun '10

Hi everyone, Recently, we found out that xtradb and federatedx were compiled as plugins on Windows. This is not correct because they are core storage engines and should be linked statically instead. On Unix, it's possible to build both types, but the CMake files doesn't really handle this. They just look in plug.in, and if there is a dynamic line, the storage engine is built dynamically. I'm hesitant to fix this properly, as it's probably already done in MySQL 5.5, or at least it will be. I see no reason to come up with a good solution for a temporary problem - there are many other issues on Windows that are more pressing. FederatedX couldn't compile as a statically linked plugin at all on Windows. I have fixed this now, but I *really* don't like the way we do these hacks on Windows. It's absurdly difficult to keep track of when a variable says xtradb and when it says innobase. And debugging CMakeLists.txt files is not too much fun. Anyway, I did the same thing on FederatedX as is done in XtraDB, and it works now. Finally, I removed a subdir that didn't have a CMakeLists.txt. Removed a warning during the cmake run. With this patch, the Windows zip file release build should work correctly and produce a MariaDB with both XtraDB and FederatedX in it. Should I check this into ~maria-captains/maria/5.1, or is there a branch more suitable at the moment? Bo Thorsen. Monty Program AB. -- MariaDB: MySQL replacement Community developed. Feature enhanced. Backward compatible.

2 2

[Maria-developers] Rev 2795: Uninitialized memory problem fixed. in file:///home/bell/maria/bzr/work-maria-5.3-scache2/
by sanja＠askmonty.org 08 Jun '10

08 Jun '10

At file:///home/bell/maria/bzr/work-maria-5.3-scache2/ ------------------------------------------------------------ revno: 2795 revision-id: sanja(a)askmonty.org-20100608122454-dtlvue8n2s55yi67 parent: sanja(a)askmonty.org-20100608120847-v4loj2gdqjflbarv committer: sanja(a)askmonty.org branch nick: work-maria-5.3-scache2 timestamp: Tue 2010-06-08 15:24:54 +0300 message: Uninitialized memory problem fixed. === modified file 'sql/table.cc' --- a/sql/table.cc 2010-06-07 07:58:45 +0000 +++ b/sql/table.cc 2010-06-08 12:24:54 +0000 @@ -5176,6 +5176,7 @@ key_part_info->offset= (*reg_field)->offset(record[0]); key_part_info->length= (uint16) (*reg_field)->pack_length(); keyinfo->key_length+= key_part_info->length; + key_part_info->key_part_flag= 0; /* TODO: The below method of computing the key format length of the key part is a copy/paste from opt_range.cc, and table.cc.

1 0

[Maria-developers] Rev 2794: Fixed uninitialized memory in file:///home/bell/maria/bzr/work-maria-5.3-scache2/
by sanja＠askmonty.org 08 Jun '10

08 Jun '10

At file:///home/bell/maria/bzr/work-maria-5.3-scache2/ ------------------------------------------------------------ revno: 2794 revision-id: sanja(a)askmonty.org-20100608120847-v4loj2gdqjflbarv parent: sanja(a)askmonty.org-20100608114156-36q3me04ecxh2by6 committer: sanja(a)askmonty.org branch nick: work-maria-5.3-scache2 timestamp: Tue 2010-06-08 15:08:47 +0300 message: Fixed uninitialized memory === modified file 'sql/sql_subquery_cache.cc' --- a/sql/sql_subquery_cache.cc 2010-06-08 11:41:56 +0000 +++ b/sql/sql_subquery_cache.cc 2010-06-08 12:08:47 +0000 @@ -50,6 +50,7 @@ tab_ref->null_rejecting= 1; tab_ref->disable_cache= FALSE; tab_ref->has_record= 0; + tab_ref->use_count= 0; KEY_PART_INFO *cur_key_part= tmp_key->key_part; store_key **ref_key= tab_ref->key_copy;

1 0

[Maria-developers] Rev 2793: Fixed uninitialized memory. in file:///home/bell/maria/bzr/work-maria-5.3-scache2/
by sanja＠askmonty.org 08 Jun '10

08 Jun '10

At file:///home/bell/maria/bzr/work-maria-5.3-scache2/ ------------------------------------------------------------ revno: 2793 revision-id: sanja(a)askmonty.org-20100608114156-36q3me04ecxh2by6 parent: sanja(a)askmonty.org-20100608093610-efg156vu4mi4u9pp committer: sanja(a)askmonty.org branch nick: work-maria-5.3-scache2 timestamp: Tue 2010-06-08 14:41:56 +0300 message: Fixed uninitialized memory. === modified file 'sql/mysql_priv.h' --- a/sql/mysql_priv.h 2010-05-31 21:25:54 +0000 +++ b/sql/mysql_priv.h 2010-06-08 11:41:56 +0000 @@ -1274,7 +1274,7 @@ select_result *result, SELECT_LEX_UNIT *unit, SELECT_LEX *select_lex); -struct st_join_table *create_index_lookup_join_tab(TABLE *table); +struct st_join_table *create_index_lookup_join_tab(TABLE *table, int key_no); int join_read_key2(THD *thd, struct st_join_table *tab, TABLE *table, struct st_table_ref *table_ref); void free_underlaid_joins(THD *thd, SELECT_LEX *select); === modified file 'sql/sql_select.cc' --- a/sql/sql_select.cc 2010-05-31 21:25:54 +0000 +++ b/sql/sql_select.cc 2010-06-08 11:41:56 +0000 @@ -7627,11 +7627,12 @@ Creates and fills JOIN_TAB for index look up in temporary table @param table The table where to look up + @param key_no Number of key @return JOIN_TAB object or NULL in case of error */ -JOIN_TAB *create_index_lookup_join_tab(TABLE *table) +JOIN_TAB *create_index_lookup_join_tab(TABLE *table, int key_no) { JOIN_TAB *tab; DBUG_ENTER("create_index_lookup_join_tab"); @@ -7640,13 +7641,12 @@ DBUG_RETURN(NULL); tab->read_record.table= table; tab->read_record.file=table->file; - /*tab->read_record.unlock_row= rr_unlock_row;*/ tab->next_select=0; tab->sorted= 1; + tab->ref.key= key_no; table->status= STATUS_NO_RECORD; tab->read_first_record= join_read_key; - /*tab->read_record.unlock_row= join_read_key_unlock_row;*/ tab->read_record.read_record= join_no_more_records; if (table->covering_keys.is_set(tab->ref.key) && !table->no_keyread) === modified file 'sql/sql_subquery_cache.cc' --- a/sql/sql_subquery_cache.cc 2010-05-31 21:25:54 +0000 +++ b/sql/sql_subquery_cache.cc 2010-06-08 11:41:56 +0000 @@ -225,7 +225,7 @@ (uchar*)&field_counter) < 0) || createtmp_table_search_structures(table_thd, cache_table, li_items, &tab_ref) || - !(tab= create_index_lookup_join_tab(cache_table))) + !(tab= create_index_lookup_join_tab(cache_table, 0))) { DBUG_PRINT("error", ("creating index failed")); goto error;

1 0

[Maria-developers] Rev 2792: Fixed memory management problem (Item can't contain other Item due to way of Items destruction). in file:///home/bell/maria/bzr/work-maria-5.3-scache2/
by sanja＠askmonty.org 08 Jun '10

08 Jun '10

At file:///home/bell/maria/bzr/work-maria-5.3-scache2/ ------------------------------------------------------------ revno: 2792 revision-id: sanja(a)askmonty.org-20100608093610-efg156vu4mi4u9pp parent: sanja(a)askmonty.org-20100608074734-1m60ib2tac7y9m33 committer: sanja(a)askmonty.org branch nick: work-maria-5.3-scache2 timestamp: Tue 2010-06-08 12:36:10 +0300 message: Fixed memory management problem (Item can't contain other Item due to way of Items destruction). === modified file 'sql/item_cmpfunc.cc' --- a/sql/item_cmpfunc.cc 2010-05-31 21:25:54 +0000 +++ b/sql/item_cmpfunc.cc 2010-06-08 09:36:10 +0000 @@ -1737,13 +1737,16 @@ not_null_tables_cache|= args[1]->not_null_tables(); const_item_cache&= args[1]->const_item(); DBUG_ASSERT(scache == NULL); + DBUG_ASSERT(value_for_scache == NULL); if (args[0]->cols() ==1 && thd->variables.optimizer_switch & OPTIMIZER_SWITCH_SUBQUERY_CACHE && !(sub->engine->uncacheable() & (UNCACHEABLE_RAND | UNCACHEABLE_SIDEEFFECT))) { sub->depends_on.push_front((Item**)&cache); - scache= new Subquery_cache_tmptable(thd, sub->depends_on, &result); + value_for_scache= new Item_bool_cache; + scache= new Subquery_cache_tmptable(thd, sub->depends_on, + value_for_scache); } fixed= 1; return FALSE; @@ -1851,8 +1854,8 @@ /* put result in the cache */ if (scache) { - result.set(tmp, null_value); - scache->put_value(&result); + value_for_scache->set(tmp, null_value); + scache->put_value(value_for_scache); } DBUG_RETURN(tmp); } @@ -1876,6 +1879,7 @@ delete scache; scache= 0; } + value_for_scache= 0; DBUG_VOID_RETURN; } === modified file 'sql/item_cmpfunc.h' --- a/sql/item_cmpfunc.h 2010-05-31 21:25:54 +0000 +++ b/sql/item_cmpfunc.h 2010-06-08 09:36:10 +0000 @@ -241,7 +241,7 @@ /* Subquery cache */ Subquery_cache *scache; /* result representation for the subquery cache */ - Item_bool_cache result; + Item_bool_cache *value_for_scache; bool save_cache; /* Stores the value of "NULL IN (SELECT ...)" for uncorrelated subqueries: @@ -252,7 +252,8 @@ my_bool result_for_null_param; public: Item_in_optimizer(Item *a, Item_in_subselect *b): - Item_bool_func(a, my_reinterpret_cast(Item *)(b)), cache(0), scache(NULL), + Item_bool_func(a, my_reinterpret_cast(Item *)(b)), cache(0), + scache(NULL), value_for_scache(NULL), save_cache(0), result_for_null_param(UNKNOWN) {} bool fix_fields(THD *, Item **); === modified file 'sql/item_subselect.cc' --- a/sql/item_subselect.cc 2010-05-31 21:25:54 +0000 +++ b/sql/item_subselect.cc 2010-06-08 09:36:10 +0000 @@ -34,10 +34,10 @@ Item_subselect::Item_subselect(): Item_result_field(), value_assigned(0), thd(0), substitution(0), - engine(0), old_engine(0), scache(0), used_tables_cache(0), - have_to_be_excluded(0), const_item_cache(1), inside_first_fix_fields(0), - done_first_fix_fields(FALSE), eliminated(FALSE), engine_changed(0), - changed(0), is_correlated(FALSE) + engine(0), old_engine(0), scache(0), value_for_scache(0), + used_tables_cache(0), have_to_be_excluded(0), const_item_cache(1), + inside_first_fix_fields(0), done_first_fix_fields(FALSE), + eliminated(FALSE), engine_changed(0), changed(0), is_correlated(FALSE) { with_subselect= 1; reset(); @@ -121,6 +121,7 @@ delete scache; scache= 0; } + value_for_scache= 0; reset(); value_assigned= 0; DBUG_VOID_RETURN; @@ -129,7 +130,7 @@ void Item_singlerow_subselect::cleanup() { DBUG_ENTER("Item_singlerow_subselect::cleanup"); - value= 0; row= 0; + value= 0; row= 0; null_value_item= 0; Item_subselect::cleanup(); DBUG_VOID_RETURN; } @@ -572,7 +573,7 @@ Item_singlerow_subselect::Item_singlerow_subselect(st_select_lex *select_lex) - :Item_subselect(), value(0) + :Item_subselect(), value(0), null_value_item(0) { DBUG_ENTER("Item_singlerow_subselect::Item_singlerow_subselect"); init(select_lex, new select_singlerow_subselect(this)); @@ -760,6 +761,7 @@ (uint)depends_on.elements, (uint)test(thd->variables.optimizer_switch & OPTIMIZER_SWITCH_SUBQUERY_CACHE))); engine->fix_length_and_dec(row= &value); + DBUG_ASSERT(scache == NULL); if (depends_on.elements && optimizer_flag(thd, OPTIMIZER_SWITCH_SUBQUERY_CACHE) && !(engine->uncacheable() & (UNCACHEABLE_RAND | @@ -839,6 +841,22 @@ DBUG_RETURN(NULL); } + +/** + Puts NULL value as result in the cache +*/ + +void Item_singlerow_subselect::put_null_value_in_scache() +{ + if (!value_for_scache) + { + value_for_scache= new Item_bool_cache; + value_for_scache->set(0, TRUE); // NULL + } + DBUG_ASSERT(value_for_scache->null_value); + scache->put_value(value_for_scache); +} + double Item_singlerow_subselect::val_real() { Item *cached_value; @@ -870,7 +888,7 @@ reset(); DBUG_PRINT("info", ("error: %u", (uint)err)); if (scache && !err) - scache->put_value(&const_null_value); + put_null_value_in_scache(); DBUG_RETURN(0); } } @@ -906,7 +924,7 @@ reset(); DBUG_PRINT("info", ("error: %u", (uint)err)); if (scache && !err) - scache->put_value(&const_null_value); + put_null_value_in_scache(); DBUG_RETURN(0); } } @@ -942,7 +960,7 @@ reset(); DBUG_PRINT("info", ("error: %u", (uint)err)); if (scache && !err) - scache->put_value(&const_null_value); + put_null_value_in_scache(); DBUG_RETURN(0); } } @@ -979,7 +997,7 @@ reset(); DBUG_PRINT("info", ("error: %u", (uint)err)); if (scache && !err) - scache->put_value(&const_null_value); + put_null_value_in_scache(); DBUG_RETURN(0); } } @@ -1016,7 +1034,7 @@ reset(); DBUG_PRINT("info", ("error: %u", (uint)err)); if (scache && !err) - scache->put_value(&const_null_value); + put_null_value_in_scache(); DBUG_RETURN(0); } } @@ -1108,13 +1126,16 @@ max_columns= engine->cols(); /* We need only 1 row to determine existence */ unit->global_parameters->select_limit= new Item_int((int32) 1); + + DBUG_ASSERT(scache == NULL); + DBUG_ASSERT(value_for_scache == NULL); if (substype() == EXISTS_SUBS && depends_on.elements && optimizer_flag(thd, OPTIMIZER_SWITCH_SUBQUERY_CACHE) && !(engine->uncacheable() & (UNCACHEABLE_RAND | UNCACHEABLE_SIDEEFFECT))) { - DBUG_ASSERT(scache == NULL); - scache= new Subquery_cache_tmptable(thd, depends_on, &result); + value_for_scache= new Item_bool_cache; + scache= new Subquery_cache_tmptable(thd, depends_on, value_for_scache); DBUG_PRINT("info", ("cache: 0x%lx", (ulong) scache)); } DBUG_VOID_RETURN; @@ -1141,8 +1162,8 @@ if (scache) { - result.set(value, FALSE); - scache->put_value(&result); + value_for_scache->set(value, FALSE); + scache->put_value(value_for_scache); } DBUG_RETURN((double) value); @@ -1170,8 +1191,8 @@ if (scache) { - result.set(value, FALSE); - scache->put_value(&result); + value_for_scache->set(value, FALSE); + scache->put_value(value_for_scache); } DBUG_RETURN(value); @@ -1213,8 +1234,8 @@ if (scache) { - result.set(value, FALSE); - scache->put_value(&result); + value_for_scache->set(value, FALSE); + scache->put_value(value_for_scache); } str->set((ulonglong)value,&my_charset_bin); @@ -1257,8 +1278,8 @@ if (scache) { - result.set(value, FALSE); - scache->put_value(&result); + value_for_scache->set(value, FALSE); + scache->put_value(value_for_scache); } int2my_decimal(E_DEC_FATAL_ERROR, value, 0, decimal_value); @@ -1287,8 +1308,8 @@ if (scache) { - result.set(value, FALSE); - scache->put_value(&result); + value_for_scache->set(value, FALSE); + scache->put_value(value_for_scache); } DBUG_RETURN(value != 0); === modified file 'sql/item_subselect.h' --- a/sql/item_subselect.h 2010-05-31 21:25:54 +0000 +++ b/sql/item_subselect.h 2010-06-08 09:36:10 +0000 @@ -60,8 +60,8 @@ subselect_engine *old_engine; /* subquery cache */ Subquery_cache *scache; - /* null consrtant for caching */ - Item_null const_null_value; + /* subquery cache value for NULL and TRUE/FALSE subqueries */ + Item_bool_cache *value_for_scache; /* cache of used external tables */ table_map used_tables_cache; /* allowed number of columns (1 for single value subqueries) */ @@ -217,10 +217,15 @@ { protected: Item_cache *value, **row; + /* null value for subquery cache value */ + Item_null *null_value_item; + + void put_null_value_in_scache(); public: Item_singlerow_subselect(st_select_lex *select_lex); - Item_singlerow_subselect() :Item_subselect(), value(0), row (0) {} + Item_singlerow_subselect() :Item_subselect(), value(0), row (0), + null_value_item(0) {} void cleanup(); subs_type substype() { return SINGLEROW_SUBS; } @@ -284,8 +289,6 @@ { protected: bool value; /* value of this item (boolean: exists/not-exists) */ - /* result representation for the subquery cache */ - Item_bool_cache result; public: Item_exists_subselect(st_select_lex *select_lex);

1 0

[Maria-developers] Build Slaves
by Adam M. Dutko 08 Jun '10

08 Jun '10

Is the adutko-ultrasparc3 buildbot too slow? would it be missed?

2 1

[Maria-developers] Rev 2791: bugfixes lost in moving between trees in file:///home/bell/maria/bzr/work-maria-5.3-scache2/
by sanja＠askmonty.org 08 Jun '10

08 Jun '10

At file:///home/bell/maria/bzr/work-maria-5.3-scache2/ ------------------------------------------------------------ revno: 2791 revision-id: sanja(a)askmonty.org-20100608074734-1m60ib2tac7y9m33 parent: sanja(a)askmonty.org-20100607075845-lo3tcaiuk54qqlw0 committer: sanja(a)askmonty.org branch nick: work-maria-5.3-scache2 timestamp: Tue 2010-06-08 10:47:34 +0300 message: bugfixes lost in moving between trees === modified file 'sql/item.cc' --- a/sql/item.cc 2010-05-31 21:25:54 +0000 +++ b/sql/item.cc 2010-06-08 07:47:34 +0000 @@ -5152,6 +5152,10 @@ int Item_field::save_in_field(Field *to, bool no_conversions) { + /* if it is external field */ + if (unlikely(depended_from)) + return save_field_in_field(field, &null_value, to, no_conversions); + return save_field_in_field(result_field, &null_value, to, no_conversions); } @@ -6359,7 +6363,7 @@ int Item_ref::save_in_field(Field *to, bool no_conversions) { int res; - if (result_field) + if (result_field && !depended_from) return save_field_in_field(result_field, &null_value, to, no_conversions); res= (*ref)->save_in_field(to, no_conversions); null_value= (*ref)->null_value;

1 0

[Maria-developers] Question about License...
by Mateusz Matan 08 Jun '10

08 Jun '10

Welcome, We designed a custom Storage Engine (CLDB) for MySQL/MariaDB which now pass all premature tests. I wonder, how can we solve license problem... We would not share our sources, but we will just use shared library (*.so)... Which licenses should our customer who bought MariaDB with CLDB use to stay legally... If it is possible, can we use in corporation use MariaDB on GPL and just sell CLDB (storage engine) on other license (which one) ?? Thank You for Your quick reply... -- ___________________________________________________________ Mateusz Matan IT Security R&D Department, C/C++ programmer ComArch S.A., Al. Jana Pawła II 41d, 31-864 Kraków tel: (+48 12) 684 8411 e-mail: Mateusz.Matan(a)comarch.pl

1 0

Re: [Maria-developers] [Commits] Rev 2797: MWL#90, code movearound to unify merged and non-merged semi-join materialization processing in file:///home/psergey/dev/maria-5.3-subqueries-r12/
by Sergey Petrunya 07 Jun '10

07 Jun '10

Hi Monty, On Fri, Jun 04, 2010 at 05:36:59PM +0300, Michael Widenius wrote: > Note that this is not a full review, but just a quick scan of some of > the things in the commit. (One suspicious thing found...) Thanks for the feedback! Reply summary: - This commit was primarily to get a buildbot run, hence the presence of loads of commented-out old code and lack of real comments. I'll address this a bit later. - The suspicious thing confirmed and fixed. - Style suggestions followed. > >>>>> "Sergey" == Sergey Petrunya <psergey(a)askmonty.org> writes: > > <cut> > > Sergey> === modified file 'sql/item_cmpfunc.cc' > Sergey> --- a/sql/item_cmpfunc.cc 2010-05-25 06:32:15 +0000 > Sergey> +++ b/sql/item_cmpfunc.cc 2010-06-04 13:40:57 +0000 > Sergey> @@ -5733,6 +5733,8 @@ > Sergey> It's a field from an materialized semi-join. We can substitute it only > Sergey> for a field from the same semi-join. > Sergey> */ > Sergey> +#if 0 > Sergey> + psergey3:remove: > > Please don't ever use #if 0; Instead use something like: > > #ifdef LEFT_FOR_TESTING_WILL_BE_REMOVED_BY_PSERGEY_SOON > > Even better to just remove the code (after all, we can always find it > in bzr) > > <cut> > Sergey> - if (item->field->table->reginfo.join_tab >= first) > Sergey> + //if (item->field->table->reginfo.join_tab >= first) > > Same here; Don't leave the old code around > > <cut> > > Sergey> +bool join_tab_execution_startup(JOIN_TAB *tab) > Sergey> { > Sergey> + DBUG_ENTER("join_tab_execution_startup"); > Sergey> Item_in_subselect *in_subs; > > Please put DBUG_ENTER after all declarations. > (So that we have same layout in C and C++) > > <cut> > > Sergey> +#if 0 > > Replace with proper #if or remove code > <cut> > > Sergey> +++ b/sql/sql_select.cc 2010-06-04 13:40:57 +0000 > Sergey> @@ -1008,15 +1006,26 @@ > Sergey> /* > Sergey> Permorm the the optimization on fields evaluation mentioned above > Sergey> for all on expressions. > Sergey> - */ > Sergey> - for (JOIN_TAB *tab= join_tab + const_tables; tab < join_tab + tables ; tab++) > Sergey> + */ > Sergey> + > Sergey> { > Sergey> - if (*tab->on_expr_ref) > Sergey> + List_iterator<JOIN_TAB_RANGE> it(join_tab_ranges); > Sergey> + JOIN_TAB_RANGE *jt_range; > Sergey> + bool first= TRUE; > > Wouldn't it be better to set first to const_tables and then to 0 ? > > Sergey> + while ((jt_range= it++)) > Sergey> { > Sergey> + for (JOIN_TAB *tab= jt_range->start + (first ? const_tables : 0); > > If you do the above, then you can just do 'jt_range->start + first' here > > Sergey> + tab < jt_range->end; tab++) > Sergey> + { > Sergey> + if (*tab->on_expr_ref) > Sergey> + { > Sergey> + *tab->on_expr_ref= substitute_for_best_equal_field(*tab->on_expr_ref, > Sergey> + tab->cond_equal, > Sergey> + map2table); > Sergey> + (*tab->on_expr_ref)->update_used_tables(); > Sergey> + } > Sergey> + } > Sergey> + first= FALSE; > Sergey> } > Sergey> } > > A comment for the above outer loop would be nice. > (It's not obvious why only the first element in join_tab_ranges has > const tables) > > Sergey> @@ -1026,6 +1035,7 @@ > Sergey> { > Sergey> conds=new Item_int((longlong) 0,1); // Always false > Sergey> } > Sergey> + > Sergey> if (make_join_select(this, select, conds)) > Sergey> { > Sergey> zero_result_cause= > Sergey> @@ -1289,7 +1299,8 @@ > Sergey> if (need_tmp || select_distinct || group_list || order) > Sergey> { > Sergey> for (uint i = const_tables; i < tables; i++) > Sergey> - join_tab[i].table->prepare_for_position(); > Sergey> + table[i]->prepare_for_position(); > Sergey> + > > Isn't table[] in other order than join_tab? > (I thought that only join_tab has the const tables first) Right. Will fix this. > <cut> > > Sergey> +JOIN_TAB *first_linear_tab(JOIN *join, bool after_const_tables) > Sergey> +{ > Sergey> + JOIN_TAB *first= join->join_tab; > Sergey> + if (after_const_tables) > Sergey> + first += join->const_tables; > > remove space before '+' > > Sergey> + if (first < join->join_tab + join->top_jtrange_tables) > Sergey> + return first; > Sergey> + else > Sergey> + return NULL; > Sergey> +} > > Better to do: > > return (first < join->join_tab + join->top_jtrange_tables) ? first : 0; > > Or: > > if (first < join->join_tab + join->top_jtrange_tables) > return first; > return NULL; > Changed. > <cut> > > Sergey> +JOIN_TAB *next_linear_tab(JOIN* join, JOIN_TAB* tab, bool include_bush_roots) //psergey2: added > > Remove comments; It's trival to see that the function was added :) > > Sergey> +{ > Sergey> + if (include_bush_roots && tab->bush_children) > Sergey> + return tab->bush_children->start; > Sergey> + > Sergey> + if (tab->last_leaf_in_bush) > Sergey> + tab= tab->bush_root_tab; > Sergey> + > Sergey> + if (tab->bush_root_tab) > Sergey> + return ++tab; > > Add an assert before if (tab->last_leaf_in_bush): > > DBUG_ASSERT(!tab->last_leaf_in_bush || tab->bush_root_tab); > Just to declare that the above code is safe! Done. > Sergey> + > Sergey> + if (++tab == join->join_tab + join->top_jtrange_tables /*join->join_tab_ranges.head()->end*/) > > Move the comment to previous row and make it more clear what you are testing > (The current comment doesn't tell me much) > > Sergey> + return NULL; > Sergey> + > Sergey> + if (!include_bush_roots && tab->bush_children) > Sergey> + { > Sergey> + tab= tab->bush_children->start; > Sergey> + } > Sergey> + return tab; > > Why not do: > > return ((!include_bush_roots && tab->bush_children) ? > tab->bush_children->start : tab); > > Sergey> + if ((start? tab: ++tab) == join->join_tab_ranges.head()->end) > Sergey> + return NULL; /* End */ > > I think the above code would be more clear if you would do: > > if (start) > tab++; > if (...) > > This makes it clear that the ++tab is not just for the test but also > for future usage of tab. Changed. > Sergey> + > Sergey> + if (tab->bush_children) > Sergey> + return tab->bush_children->start; > Sergey> + > Sergey> + return tab; > > Could be combined with ? > > Sergey> +} > Sergey> + > Sergey> + > Sergey> +static Item *null_ptr= NULL; > > Can we make this const, so that if anyone tried to change this memory > location we would get an exception? Done > <cut> > > Sergey> DBUG_RETURN(TRUE); /* purecov: inspected */ > > Sergey> join_tab= parent->join_tab_reexec; > Sergey> + //psergey2: hopefully this is ok: > Sergey> + // join_tab_ranges.head()->start= join_tab; > Sergey> + // join_tab_ranges.head()->end= join_tab + 1; > > Better to know than to hope :) It wasn't ok actually, already changed. > > (sorry, don't have time to look at the rest now) BR Sergey -- Sergey Petrunia, Software Developer Monty Program AB, http://askmonty.org Blog: http://s.petrunia.net/blog

1 0