- developers - lists.mariadb.org

[Maria-developers] GSoC 2016 Maxscale Filter Implementation.
by Jason Kravitz 07 Mar '16

07 Mar '16

Hi, I am Jay Kravitz an Informations systems student at Georgetown University and I would love to work on Maxscale during GSoC 2016. I am interested in the Filter projects specifically the filter to capture incoming operations for consumption in external sources. I have build Maxscale on Ubuntu 15 and I am currently looking at some filters in the codebase to understand the filter API. I will also submit a backup proposal for the query injection filter hoping I am given the opportunity to implement one. I look forward to hearing from anyone soon. Regards, JK.

1 0

[Maria-developers] main.delayed fails on timeout
by Alexander Barkov 03 Mar '16

03 Mar '16

Hello Monty, Sergei, Can you please have a look into: http://buildbot.askmonty.org/buildbot/builders/kvm-deb-precise-amd64/builds… Can you please give any suggestions about what we can do to fix the problem with main.delayed failing sporadically? This problem is blocking us badly in Debian. Otto can try to disable this test temporary. But it would be nice to have a real fix. Any tips would be very appreciated. Thanks!

2 1

[Maria-developers] GSoC 2015 mysqlbinlog Enhancement.
by Rebecca Tucker 03 Mar '16

03 Mar '16

Hello everyone, I am Rebecca Tucker a MS student at University of Maryland and I wish to participate in the summer of code to work on making the mysqlbinlog tool to understand global transaction IDs. I am familiar with the tool and replication and can write documented C and C++ code. I Look forward to communicating with you soon to get more pointers on the project. Cheers, Becca.

1 0

[Maria-developers] GSOC 2016 self Introduction
by Tapan Halani 02 Mar '16

02 Mar '16

Hello all devs, I am Tapan Halani , an undergrad student from India. I am interested in contributing code to MariaDB and am taking part in GSoC 2016 also. I have already done all the prerequisite code cloning and setup etc. and am going through MDEV-4989 , trying to understand and solve it. Looking forward to have a great time contributing. Thanks Tapan

1 0

[Maria-developers] Igor please review: b6a2441: Remove all comments starting with 'psergey'
by Sergey Petrunia 02 Mar '16

02 Mar '16

Please review the below: ----- Forwarded message from Sergei Petrunia <psergey(a)askmonty.org> ----- Date: Wed, 2 Mar 2016 13:35:11 +0300 (FET) From: Sergei Petrunia <psergey(a)askmonty.org> To: commits(a)mariadb.org, sergey(a)mariadb.com Subject: b6a2441: Remove all comments starting with 'psergey' revision-id: b6a2441edf3bfce381f5dc24f5be23fce9ac36dd parent(s): b05158cc10a75196b5c0bf8dad9360608a2dd5b9 committer: Sergei Petrunia branch nick: 10.1-dbg5 timestamp: 2016-03-02 13:35:11 +0300 message: Remove all comments starting with 'psergey' --- sql/item.cc | 3 +-- sql/item_subselect.cc | 15 +-------------- sql/opt_range.cc | 4 ++-- sql/opt_subselect.cc | 1 - sql/opt_table_elimination.cc | 2 -- sql/sql_base.cc | 4 ++-- sql/sql_join_cache.cc | 7 +------ sql/sql_select.cc | 39 ++------------------------------------- sql/sql_statistics.cc | 9 --------- sql/sql_union.cc | 2 +- 10 files changed, 10 insertions(+), 76 deletions(-) diff --git a/sql/item.cc b/sql/item.cc index d6132cc..8fd45d5 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -4227,8 +4227,7 @@ static bool mark_as_dependent(THD *thd, SELECT_LEX *last, SELECT_LEX *current, DBUG_PRINT("info", ("mark_item: %p lex: %p", mark_item, last)); mark_item->depended_from= last; } - if (current->mark_as_dependent(thd, last, - /** resolved_item psergey-thu **/ mark_item)) + if (current->mark_as_dependent(thd, last, mark_item)) DBUG_RETURN(TRUE); if (thd->lex->describe & DESCRIBE_EXTENDED) { diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc index a5ef81e..e11eafe 100644 --- a/sql/item_subselect.cc +++ b/sql/item_subselect.cc @@ -243,10 +243,6 @@ bool Item_subselect::fix_fields(THD *thd_param, Item **ref) done_first_fix_fields= TRUE; inside_first_fix_fields= TRUE; upper_refs.empty(); - /* - psergey-todo: remove _first_fix_fields calls, we need changes on every - execution - */ } eliminated= FALSE; @@ -1753,7 +1749,6 @@ my_decimal *Item_in_subselect::val_decimal(my_decimal *decimal_value) Check that the right part of the subselect contains no more than one column. E.g. in SELECT 1 IN (SELECT * ..) the right part is (SELECT * ...) */ - // psergey: duplicated_subselect_card_check if (select_lex->item_list.elements > 1) { my_error(ER_OPERAND_COLUMNS, MYF(0), 1); @@ -2172,7 +2167,6 @@ bool Item_allany_subselect::is_maxmin_applicable(JOIN *join) DBUG_ENTER("Item_in_subselect::row_value_transformer"); - // psergey: duplicated_subselect_card_check if (select_lex->item_list.elements != cols_num) { my_error(ER_OPERAND_COLUMNS, MYF(0), cols_num); @@ -3600,7 +3594,6 @@ void subselect_engine::set_row(List<Item> &item_list, Item_cache **row) if (!(row[i]= Item_cache::get_cache(thd, sel_item, sel_item->cmp_type()))) return; row[i]->setup(thd, sel_item); - //psergey-backport-timours: row[i]->store(sel_item); } if (item_list.elements > 1) cmp_type= res_type= ROW_RESULT; @@ -4017,9 +4010,6 @@ int subselect_uniquesubquery_engine::index_lookup() subselect_uniquesubquery_engine::~subselect_uniquesubquery_engine() { - /* Tell handler we don't need the index anymore */ - //psergey-merge-todo: the following was gone in 6.0: - //psergey-merge: don't need this after all: tab->table->file->ha_index_end(); } @@ -4172,9 +4162,6 @@ int subselect_indexsubquery_engine::exec() uint subselect_single_select_engine::cols() { - //psergey-sj-backport: the following assert was gone in 6.0: - //DBUG_ASSERT(select_lex->join != 0); // should be called after fix_fields() - //return select_lex->join->fields_list.elements; return select_lex->item_list.elements; } @@ -5136,7 +5123,7 @@ void check_out_index_stats(JOIN *join) table_map key_used=0; table_map non_key_used= 0; - bzero(&key_start_use, sizeof(key_start_use)); //psergey-todo: safe initialization! + bzero(&key_start_use, sizeof(key_start_use)); bzero(&key_infix_use, sizeof(key_infix_use)); for (order= join->group_list; order; order= order->next) diff --git a/sql/opt_range.cc b/sql/opt_range.cc index ed7e9a5..72a851d 100644 --- a/sql/opt_range.cc +++ b/sql/opt_range.cc @@ -8640,7 +8640,7 @@ static bool remove_nonrange_trees(RANGE_OPT_PARAM *param, SEL_TREE *tree) if (key1->elements != 1) { - key2->use_count+=key1->elements-1; //psergey: why we don't count that key1 has n-k-p? + key2->use_count+=key1->elements-1; key2->increment_use_count((int) key1->elements-1); } if (key1->type == SEL_ARG::MAYBE_KEY) @@ -10104,7 +10104,7 @@ ha_rows check_quick_select(PARAM *param, uint idx, bool index_only, } DBUG_PRINT("exit", ("Records: %lu", (ulong) rows)); - DBUG_RETURN(rows); //psergey-merge:todo: maintain first_null_comp. + DBUG_RETURN(rows); } diff --git a/sql/opt_subselect.cc b/sql/opt_subselect.cc index cb356fb..1a13dd6 100644 --- a/sql/opt_subselect.cc +++ b/sql/opt_subselect.cc @@ -635,7 +635,6 @@ int check_and_do_in_subquery_rewrites(JOIN *join) (oe1, oe2) IN (SELECT ie1, ie2, ie3 ...) TODO why do we have this duplicated in IN->EXISTS transformers? - psergey-todo: fix these: grep for duplicated_subselect_card_check */ if (select_lex->item_list.elements != in_subs->left_expr->cols()) { diff --git a/sql/opt_table_elimination.cc b/sql/opt_table_elimination.cc index 912ef4a..8378e8e 100644 --- a/sql/opt_table_elimination.cc +++ b/sql/opt_table_elimination.cc @@ -757,8 +757,6 @@ void eliminate_tables(JOIN *join) else { DBUG_ASSERT(!tbl->nested_join || tbl->sj_on_expr); - //psergey-todo: is the following really correct or we'll need to descend - //down all ON clauses: ? if (tbl->sj_on_expr) tables_used_on_left |= tbl->sj_on_expr->used_tables(); } diff --git a/sql/sql_base.cc b/sql/sql_base.cc index ac2162b..3d02f9c 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -3965,7 +3965,7 @@ thr_lock_type read_lock_type_for_table(THD *thd, goto end; } DBUG_PRINT("tcache", ("opening table: '%s'.'%s' item: %p", - tables->db, tables->table_name, tables)); //psergey: invalid read of size 1 here + tables->db, tables->table_name, tables)); (*counter)++; /* @@ -8105,7 +8105,7 @@ bool setup_tables(THD *thd, Name_resolution_context *context, Item *item= table_list->jtbm_subselect->optimizer; if (table_list->jtbm_subselect->optimizer->fix_fields(thd, &item)) { - my_error(ER_TOO_MANY_TABLES,MYF(0), static_cast<int>(MAX_TABLES)); /* psergey-todo: WHY ER_TOO_MANY_TABLES ???*/ + my_error(ER_TOO_MANY_TABLES,MYF(0), static_cast<int>(MAX_TABLES)); DBUG_RETURN(1); } DBUG_ASSERT(item == table_list->jtbm_subselect->optimizer); diff --git a/sql/sql_join_cache.cc b/sql/sql_join_cache.cc index f84440a..9f69169 100644 --- a/sql/sql_join_cache.cc +++ b/sql/sql_join_cache.cc @@ -3919,11 +3919,6 @@ int JOIN_TAB_SCAN_MRR::next() If a record in in an incremental cache contains no fields then the association for the last record in cache will be equal to cache->end_pos */ - /* - psergey: this makes no sense where HA_MRR_NO_ASSOC is used. - DBUG_ASSERT(cache->buff <= (uchar *) (*ptr) && - (uchar *) (*ptr) <= cache->end_pos); - */ if (join_tab->table->vfield) update_virtual_fields(join->thd, join_tab->table); } @@ -4591,7 +4586,7 @@ bool JOIN_CACHE_BKAH::prepare_look_for_matches(bool skip_last) { last_matching_rec_ref_ptr= next_matching_rec_ref_ptr= 0; if (no_association && - !(curr_matching_chain= get_matching_chain_by_join_key())) //psergey: added '!' + !(curr_matching_chain= get_matching_chain_by_join_key())) return 1; last_matching_rec_ref_ptr= get_next_rec_ref(curr_matching_chain); return 0; diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 4759af0..878cb8b 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -2861,9 +2861,6 @@ void JOIN::exec_inner() /* Free first data from old join */ - /* - psergey-todo: this is the place of pre-mature JOIN::free call. - */ curr_join->join_free(); if (curr_join->make_simple_join(this, curr_tmp_table)) DBUG_VOID_RETURN; @@ -3001,7 +2998,6 @@ void JOIN::exec_inner() curr_join->select_distinct=0; } curr_tmp_table->reginfo.lock_type= TL_UNLOCK; - // psergey-todo: here is one place where we switch to if (curr_join->make_simple_join(this, curr_tmp_table)) DBUG_VOID_RETURN; calc_group_buffer(curr_join, curr_join->group_list); @@ -6646,12 +6642,6 @@ static void choose_initial_table_order(JOIN *join) jtab_sort_func= straight_join ? join_tab_cmp_straight : join_tab_cmp; } - /* - psergey-todo: if we're not optimizing an SJM nest, - - sort that outer tables are first, and each sjm nest follows - - then, put each [sjm_table1, ... sjm_tableN] sub-array right where - WHERE clause pushdown would have put it. - */ my_qsort2(join->best_ref + join->const_tables, join->table_count - join->const_tables, sizeof(JOIN_TAB*), jtab_sort_func, (void*)join->emb_sjm_nest); @@ -9171,7 +9161,6 @@ static bool create_ref_for_key(JOIN *join, JOIN_TAB *j, !(parent->join_tab_reexec= (JOIN_TAB*) thd->alloc(sizeof(JOIN_TAB)))) DBUG_RETURN(TRUE); /* purecov: inspected */ - // psergey-todo: here, save the pointer for original join_tabs. join_tab= parent->join_tab_reexec; table= &parent->table_reexec[0]; parent->table_reexec[0]= temp_table; table_count= top_join_tab_count= 1; @@ -10060,9 +10049,6 @@ bool TABLE_LIST::is_active_sjm() DBUG_ASSERT(inner_tab->table); current_map= inner_tab->table->map; used_tables2|= current_map; - /* - psergey: have put the -1 below. It's bad, will need to fix it. - */ COND *tmp_cond= make_cond_for_table(thd, on_expr, used_tables2, current_map, /*(inner_tab - first_tab)*/ -1, @@ -11070,11 +11056,6 @@ void check_join_cache_usage_for_tables(JOIN *join, ulonglong options, idx, prev_tab); tab->use_join_cache= MY_TEST(tab->used_join_cache_level); - /* - psergey-merge: todo: raise the question that this is really stupid that - we can first allocate a join buffer, then decide not to use it and free - it. - */ if (join->return_tab) { tab= join->return_tab; @@ -15622,12 +15603,6 @@ bool cond_is_datetime_is_null(Item *cond) @retval false cannot be used */ -/* - psergey-todo: this returns false for int_column='1234' (here '1234' is a - constant. Need to discuss this with Bar). - - See also Field::test_if_equality_guaranees_uniqueness(const Item *item); -*/ static bool test_if_equality_guarantees_uniqueness(Item *l, Item *r) { @@ -18293,10 +18268,6 @@ enum_nested_loop_state rc= evaluate_join_record(join, join_tab, error); } - /* - Note: psergey has added the 2nd part of the following condition; the - change should probably be made in 5.1, too. - */ bool skip_over= FALSE; while (rc == NESTED_LOOP_OK && join->return_tab >= join_tab) { @@ -20956,7 +20927,7 @@ uint find_shortest_key(TABLE *table, const key_map *usable_keys) !(table->file->index_flags(best_key, 0, 1) & HA_CLUSTERED_INDEX))) goto use_filesort; - if (select && // psergey: why doesn't this use a quick? + if (select && table->quick_keys.is_set(best_key) && best_key != ref_key) { key_map tmp_map; @@ -24002,11 +23973,7 @@ void JOIN_TAB::save_explain_data(Explain_table_access *eta, eta->type= tab_type; /* Build "possible_keys" value */ - // psergey-todo: why does this use thd MEM_ROOT??? Doesn't this - // break ANALYZE ? thd->mem_root will be freed, and after that we will - // attempt to print the query plan? append_possible_keys(thd->mem_root, eta->possible_keys, table, keys); - // psergey-todo: ^ check for error return code /* Build "key", "key_len", and "ref" */ if (tab_type == JT_NEXT) @@ -24054,8 +24021,6 @@ void JOIN_TAB::save_explain_data(Explain_table_access *eta, eta->hash_next_key.set(thd->mem_root, & table->key_info[index], table->key_info[index].key_length); - // psergey-todo: ^ is the above correct? are we necessarily joining on all - // columns? } if (!key_info) @@ -24211,7 +24176,7 @@ void JOIN_TAB::save_explain_data(Explain_table_access *eta, eta->push_extra(ET_OPEN_FRM_ONLY); else eta->push_extra(ET_OPEN_FULL_TABLE); - /* psergey-note: the following has a bug.*/ + if (table_list->is_table_read_plan->trivial_show_command || (table_list->is_table_read_plan->has_db_lookup_value() && table_list->is_table_read_plan->has_table_lookup_value())) diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index 28e76ae..b22a2ae 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -3650,15 +3650,6 @@ double get_column_range_cardinality(Field *field, { double avg_frequency= col_stats->get_avg_frequency(); res= avg_frequency; - /* - psergey-todo: what does check for min_value, max_value mean? - min/max_value are set to NULL in alloc_statistics_for_table() and - alloc_statistics_for_table_share(). Both functions will immediately - call create_min_max_statistical_fields_for_table and - create_min_max_statistical_fields_for_table_share() respectively, - which will set min/max_value to be valid pointers, unless OOM - occurs. - */ if (avg_frequency > 1.0 + 0.000001 && col_stats->min_value && col_stats->max_value) { diff --git a/sql/sql_union.cc b/sql/sql_union.cc index c835083..b369ef0 100644 --- a/sql/sql_union.cc +++ b/sql/sql_union.cc @@ -974,7 +974,7 @@ bool st_select_lex_unit::exec() 1st execution sets certain members (e.g. select_result) to perform subquery execution rather than EXPLAIN line production. In order to reset them back, we re-do all of the actions (yes it is ugly): - */ // psergey-todo: is the above really necessary anymore?? + */ join->init(thd, item_list, fake_select_lex->options, result); saved_error= mysql_select(thd, &fake_select_lex->ref_pointer_array, &result_table_list, ----- End forwarded message ----- -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] GSoC 2016 self Introduction
by Charles Muurmu 02 Mar '16

02 Mar '16

Dear MariaDB Developers, I am Charles Muurmu and I wish to participate in GSoC 2016 (and 17 if possible) with MariaDB. First, congratulations for passing a successful application with Google. I have gone through the program time line and students applications start in two weeks, However I am interested in two projects for which I will submit 2 proposals. First, I am interested in the Cassandra storage engine V2 as I have basic knowledge on how to write storage engine plugins as well as working knowledge of cassandra. Also I will be having as backup project the GIS enhancement task which requires adding a new geometry type containing the third coordinate, writing importers and exporters for the extended geometry formats... I have two books guiding me for now: Understanding MySQL Internals by O'reilly media and MySQL plugin Development by Sergei Golubchik. While preparing for student application I will be reading mariadb Knowledge base and ask questions on #maria. I will also try to look an the DataStax C++ Driver which will be used. Thanks, Charles.

1 0

[Maria-developers] [GSoC2016] Contribution to Cassandra SE v2
by Olzhas Kaiyrakhmet 02 Mar '16

02 Mar '16

Hello, I am Olzhas Kaiyrakhmet, computer science student at Ulsan National Institute of Science and Technology, Korea. I am interested in contributing to code to MariaDB and participating in GSoC2016. I do find the project, Cassandra SE v2, very interesting for me. Recently, I worked with Cassandra cluster and did look inside of it code. Tried to do some modifications. Also, I have some experience on contributing to distributive systems and databases as research intern in university. I have a great will to contribute for this project with your assistance. Best wishes, Olzhas kaiyrakhmet

3 2

[Maria-developers] Time delayed replication
by Colin Charles 29 Feb '16

29 Feb '16

Hi! Quick question — we’ve had https://jira.mariadb.org/browse/MDEV-8341 open for quite sometime now, and MySQL 5.6 and 5.7 obviously implement this feature, time delayed replication Are you using it in production? Are you finding that you are missing it in MariaDB Server? How important would it be to you, to have this in MariaDB Server 10.2? Thanks -- Colin Charles, http://bytebot.net/blog/ twitter: @bytebot | skype: colincharles "First they ignore you, then they laugh at you, then they fight you, then you win." -- Mohandas Gandhi

2 1

Re: [Maria-developers] A proposal to deprecate syntax: SELECT a'test'
by Michael Widenius 29 Feb '16

29 Feb '16

Hi! > Hello Monty, all, > > > We discussed this with Sergei and both think that it will be a good idea > to deprecate this syntax: > > SELECT a'test'; > > where 'a' is an identifier and 'test' its alias. Is this dependent on if there is a space or not between a and 'test' ? Note that in MySQL and MariaDB, ' and " are both used for quoting strings and I don't think we can change that without affecting a lot of applications! I think it's ok to require one to have a space between a and 'test', if that would help at all. > This is a non-standard way, and it conflicts with some other important > standard SQL grammar. See below. Note that in MySQL/MariaDB, when not using ANSI mode, ' and " are equal and ` is used for identifiers, so this is expected to be non-standard. For quoting identifiers ` should be used. Of course, if you are using ANSI mode, then things should be as close to ANSI as possible. > Typified literals and a syntax conflict > --------------------------------------- > > An identifier followed by a single-quoted text string is needed > for typified literals: > > SELECT INET6'ffff::ffff'; > > We already support this for temporal literals, which is a part of > the SQL standard: > > SELECT TIME'10:10:10'; > SELECT TIMESTAMP'2001-01-01 10:20:30'; > SELECT DATE'2001-01-01'; > > Note, the conflict already exists in MySQL and MariaDB. > > > This script: > > DROP TABLE IF EXISTS t1; > CREATE TABLE t1 (date date); > INSERT INTO t1 VALUES ('2016-02-19'); > SELECT date'2001-01-01' FROM t1; > > returns: > > +------------------+ > | date'2001-01-01' | > +------------------+ > | 2001-01-01 | > +------------------+ > > Notice, it returns a DATE literal, it does not return the value of > the column t1.date using '2001-01-01' as an alias. As ansi sql doesn't allow you to use date as a column name, this isn't really as big problem as it may seam ;) Anyway, I agree that this can be confusing, but so is having date as a column namn. > Notice the difference: > > SELECT date`2001-01-01` FROM t1; > +------------+ > | 2001-01-01 | > +------------+ > | 2016-02-19 | > +------------+ > > This is confusing. Not really, as you are using different quotes and you get the expected answer. This should be same as if you used " in ANSI SQL, isn't it? > Shift-reduce conflicts > ---------------------- > By the way, the fact that we support single-quoted string as an alias > is the reason for shift-reduce conflicts in sql_yacc.yy, because > TIME/DATE/TIMESTAMP followed by 'string' can be interpreted in two ways: > - a column name followed by an alias > - an SQL-standard time literal What is the SQL-standard way to use an alias? (Sorry, can't access the standard just now): SELECT date "2001-01-01" FROM t1; or SELECT date '2001-01-01' FROM t1; And if you execute both of these queries, will you not get the same confusing result for one of them? As ' and " are identical in MySQL mode, I don't see how you can fix this without causing even more confusion. > What can be used instead > ------------------------ > We have a number of other ways to specify aliases: > > The standard ways: > > SELECT a AS test; > SELECT a "test"; > SELECT a AS "test"; Ok, this answers my question above. The standard says that an alias is a text string. In MySQL / MariaDB a text string can be specified with both ' or ". I don't think it's good idea to make alias a special case where you have to specify the text string only with ". > MySQL/MariaDB extensions: > > > SELECT a `test`; > SELECT a AS `test`; > > That should be enough. Note that this has nothing exclusively to do with alias, but with the MySQL/MariaDB extension that you can use ' instead of " as a string delimiter. > Proposal > -------- > 1. In 10.1 we add a warning when a single-quoted string is used as a > column alias. Something like this should work: > > Single quotes in a select sublist alias are deprecated. Use double > quotes instead. This would break the specification of what a text strings is in MariaDB. It would also be very confusing that text strings needs to be specified differently in different places of the SQL syntax. > 2. In 10.2 we disallow this syntax by default and add either sql_mode or > old_mode to enable the old behavior. As you can already get the behavior you want by specifying ANSI MODE, I don't think this change is necessary. I think it's also bad as it makes it harder to define what a text string is. For example: In MariaDB, a text string can be delimited with ' or ", except in the case of alias when you have to use ' I think it's better to require, as most ANSI SQL databases does, one to quote column names like 'date' and 'time' to ensure they are not mixed with operators or string prefixes. Regards, Monty

2 3

[Maria-developers] Type_handler and INET6
by Alexander Barkov 29 Feb '16

29 Feb '16

Hello Sergei, I'm sending a new version for Type_handler and INET6 for your review. There are three patches attached. 1. inet6-v65-prepare.diff.gz Includes preparatory changes, to avoid different execution paths for different data types. 2. inet6-v65-incremental.diff.gz Actually adds INET6, together with val_raw family methods, last minute changes that did not get into #1. 3. inet6-v65-full.diff.gz is a full patch that includes the previous two. Details on the patches as well as my answers to your previous review suggestions follow below. Thanks. The first patch makes preparatory changes, to avoid different execution paths and adds some helper classes needed for the API. - Adds a class Record_addr, to simplify API (pass just one value instead of 3 separate values for ptr, null_ptr and null_bit) and share some code moving initialization (e.g. for a temporary table purposes) to Record_addr constructors. Derives Field from it. - Adds a class Type_ext_attributes. This collects extra members of Item_type_holder. Currently it covers only extra attributes for GEOMETRY, DECIMAL, ENUM/SET. We'll add a generic "void *extra" pointer here, when user-defined types need even more extra attributes. This class will also be needed inside Item_hybrid_func, which currently have a bug (do not preserve geometry type and srid of the arguments). I'll fix it later. See MDEV-9405. - Adds a class Create_attr, to pass Create_field related attributes to handlers. - Adds a class Type_handler_register, to look up handlers by type code and names. - Continues unification of the code that creates temporary fields for various Items, see ::create_tmp_field() in sql_select.cc. This task is not fully finished. Some more chages are still needed. The intent is to get rid of ::create_tmp_field() and switch to use methods in Item instead. - Adds a data type handler pointer to Arg_comparator. Previously the was only "Item_result m_compare_type". - Splits Item_func_case::fix_length_and_dec() into three separate steps: fix_field_type(), fix_return_arguments(), fix_comparison_arguments(), to incorporate the type handler methods easier. - Splits Item_func_between::val_int() into virtual methods in Type_handler_xxx. - Adds virtual methods to Type_handler, to avoid different execution paths for non-traditional types: calc_pack_length(), check_column_definition(), prepare_column_definition(), make_table_field(), Item_save_in_field(), make_cache_item(), calc_pack_length(), calc_display_length(), Item_type_holder_join_attributes(), Item_func_hex_val_str_ascii(), Item_func_hybrid_field_type_val_xxx(), Item_func_between_fix_length_and_dec(), Item_func_between_val_int(), Item_sum_hybrid_fix_length_and_dec(), - Adds Type_handler_numeric, to share some code in real, int decimal handlers. - Moves field_type_for_temporal_comparison() from Item to Type_handler. - Makes some other changes in data type aggregation in various pieces of the code. This will need some more changes though. - Removes Field::result_merge_type(). Uses Type_handler::cmp_type() and Type_handler::result_type() instead. - Simplifies ::make_field() in field.cc, using Type_handler::make_table_field(). - Removes the global Arg_comparator::comparator_matrix[6][2] and uses methods in Type_handler instead. The second patch actually adds INET6 and: - Adds val_raw family methods into Items - Adds some pieces into the type aggregation code in a few places - Fixes sql_yacc.yy to handle non-traditional types, adds Cast_type and fixes Lex_cast_type_st. - Fixes Type_handler_register to search non-traditional types by name. - Disables equal expression propagation for non-traditional types (the code is not ready yet for this, e.g. Item_equal needs some changes) - Adds the INET6 handler with tests Also, I answer your questions and suggestions that you made during the first review. Please see below. >> > > === added file 'mysql-test/r/type_inet6.result' > > > --- mysql-test/r/type_inet6.result 1970-01-01 00:00:00 +0000 > > > +++ mysql-test/r/type_inet6.result 2014-07-01 11:10:33 +0000 > > > +::ff > > > +::ffff > > > +::0.255.255.255 > > > +::255.255.255.255 > > > +::ff:ffff:ffff > > > +::ffff:255.255.255.255 > > Hmm. A bit unexpected. Do you have a link to some > document that specifies how ipv6 numbers should be parsed and printed? https://en.wikipedia.org/wiki/IPv6_address says: :ffff:0:0/96 — This prefix is designated as an IPv4-mapped IPv6 address. Note, I also checked this in PostgreSQL and it works in the same way: bar=> select inet'::ffff:ffff:ffff'; inet ------------------------ ::ffff:255.255.255.255 > > > +SELECT * FROM t1 WHERE a=0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF; > > > +a > > > +ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff > > > +ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff > > > +Warnings: > > > +Warning 1105 Cast to INET6 converted '\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF' to 'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff' > > Too many warnings. I'd rather use Item_cache or something similar > to have only one actual type conversion and one warning It now creates Item_cache. So there's only one warning. > > > === modified file 'sql/field.h' > > > --- sql/field.h 2014-06-11 08:08:08 +0000 > > > +++ sql/field.h 2014-07-01 10:38:12 +0000 > > > @@ -366,6 +437,10 @@ class Field > > > virtual ~Field() {} > > > /* Store functions returns 1 on overflow and -1 on fatal error */ > > > virtual int store(const char *to, uint length,CHARSET_INFO *cs)=0; > > > + virtual int store_raw(const String *raw, enum_field_types type) > > > + { > > > + return store(raw->ptr(), raw->length(), raw->charset()); > > > + } > > How do you guarantee that the 'raw' is in the correct internal raw format? > Or is it only used after int_to_raw, str_to_raw, etc? raw representation can only be created by a method of the corresponding Type_handler_xxx, or come from "ptr" of the corresponding Field_xxx. Bad raw format means that there is a bug inside Type_handler_xxx. Btw, In the latest patch I changed it to: int store_raw_native(const char *str, uint length); Conversion (when needed) is done outside of Field. > > > +class Field_type_joiner > > It's rather "aggregator", not "joiner". > But in fact, it's not even that, it's a pair of { type, derivation }. > May be it should be Field_type_with_derivation ? :) > And a method Field_type_with_derivation::aggregate(...) It's now Type_handler_hybrid_field_type, which I earlier added in 10.1. There is no "derivation" in this version, as we discussed in Luxembough. Sigh, I'd really really prefer if we had it though. I think when field=expr is compared according to the field data type is clearer than they're compared accoring to the expr datatype. Also, comparing according to field will use the optimizer by default, without having to cast "expr" to the field data type. > > > +class Type_handler > > > +{ > > > +public: > > > + Type_handler() {} > > > + ~Type_handler() {} > > > + virtual const char *field_type_name() const = 0; > > > + virtual uint field_type_name_length() const = 0; > > may be, remove field_ prefix, or even a field_type_ prefix? > the method is still unambiguous, but much shorter and readable. > and, may be, make it non-virtual, like > > ... > private: > LEX_STRING type_name; > public: > Type_handler(const char *name_arg) > { > type_name.str= name_arg; > type_name.length= strlen(name_arg); > } > const char *name() const { return type_name.str; } > const char *name_length() const { return type_name.length; } > ... > > and your new type will do in the constructor: > > Type_handler_inet6(): Type_handler("INET6") { } > > less boilerplate code for the new types, and you know how I'm trying > to avoid boilerplate code in the APIs :) I'm using the very needed new class Name for it. It now looks simpler, I believe. > > > > + virtual enum_field_types field_type() const = 0; > > That doesn't work for loadable types. > I think that we eventually need to do with types what we've done with > storage engines. There was db_type enum, but now the engine is almost always > identified by the pointer to the handlerton. So, for types a pointer to > the Type_handler might be a unique type identifier in the server. > Replacing enum_field_types where possible. Sure, in the end we should get a code that will identify handlers by a pointer rather than field_type(). This will need a few additional steps, which I just did not have a chance to do yet: - Fix all existing Items to return type_handler() instead of field_type() as a type identifier. This will be easier after we add virtual inheritance (or templates) soon, on top of my Type_handler changes. - Get rid of all field_type() calls: split the remaining pieces of the code that use field_type() into more virtual methods in Type_handler. - Fix FRM to store new field types as a textual name rather than a numeric ID. Can we do these steps a separately? Note, it can be done after adding INET6 and UUID, before adding dynamic loadability. > > > + virtual enum_field_types fallback_field_type() const = 0; > > I don't like the idea of fallback_field_type. > You have Field_type_joiner for new types, but you fallback to > Field::field_type_merge() for old types. I'd rather not distinguish > between "old" and "new" types. Just like we have with storage engines or > authentication - there are no "native" and "plugin" storage engines (or > authentication), *all* storage engines are plugins. So there's no code like > > if (is_plugin(engine_type)) > ... > else > ... > > so, I prefer uniform code with no special checks for type handlers. > Implementations differ, but the interface is the same for all types. I nodified the code, so there is no a separate fallback_field_type() any more. But there is still cmp_type(), which is uses in very rare cases, to exchange data to/from other data types: INET6->something, something->INET6. Also, there are still some different execution paths inside the methods that aggregate data types. We'll need to discuss it. I could not come up with a simple single method for aggregation. Also, currently field type aggregation is done in different ways in various pieces of the code (hybrid functions, UNION, comparison). We should try to unify these ways. > > > + virtual Item_result cast_to_int_type() const = 0; > > Why is this abstract? It could have a reasonable default implementation > reducing the amount of boilerplate code in the Type_handler's > > > > + virtual int cmp(Item *a, Item *b) const = 0; > > > + virtual int cmp(String *a, String *b) const = 0; > > These too Not sure that exactly cast_to_int_type() can have a reasonable default. For INET6 and UUID it's DECIMAL_RESULT. For compressed URLs (MDEV-4630) and external ENUM (MDEV-4913) it's more likely to be STRING_RESULT. Anyway, I agree that we should add some reasonable defaults when possible. Perhaps we could add two abstract classes with some defaults: Type_handler | +-- Type_handler_traditional (derive all existing types from this) | +-- Type_handler_binary_fixed_length_storage (derive INET6 and UUID from this) But I'd propose not to hurry with that. We have time before we add loadabale data type plugins. Can we add UUID first? It will show how much INET6 and UUID can share and thus how much can be moved to Type_handler_binary_fixed_length_storage. > > > + virtual int save_in_field(Item_hex_hybrid *item, Field *field) const = 0; > > Why do you need that virtual? It could be always val_raw/store_raw I guess this question is not relevant any more, because we now have handlers for the traditional data types, and they need to treat hex hybrids differenty (as string for Field_str and Field_temporal, and as a longlong for the other datatypes). Note, I modified this piece slighly. It's now a virtual method in Field rather than in Type_handler: int Field::store_hex_hybrid(const char *str, uint length); > > > + virtual int set_cmp_func(Arg_comparator *cmp, Item *a, Item *b) const = 0; > > this can be removed, see below (under Type_handler_inet6::set_cmp_func) I could not get rid of it. Moreover, the traditional types now use this method. For example: bool Type_handler_int_result::set_comparator_func(Arg_comparator *cmp) const { return cmp->set_cmp_func_int(); } So now I have the same execution path for all data types here. > > > + virtual bool varbinary_to_raw(const String *from, String *to) const = 0; > > 1. This most certainly needs a comment. It took me quite a while to > understand what this method is for and why str_to_raw isn't used instead. > A comment should say that it's used when a field binary representation is > specified in a query directly was 0xHHHHHHHH... > > 2. Still, I'm not sure this method is needed. It means a different > behavior for > > where ipv6 = 0xHHHHHH... > > and > > create t1 (b binary(16)); insert t1 values (0xHHHHH....); > ... where ipv6 = a ... > Note, it's the same with what we do for numbers: CREATE OR REPLACE TABLE t1 (a INT, b BLOB); INSERT INTO t1 VALUES (0x31,0x31); SELECT COUNT(*) FROM t1 WHERE a=0x31; -- returns a row SELECT * FROM t1 WHERE a=b; -- returns no rows See below. > an alternative would be to treat any binary string as raw, > directly in str_to_raw. But a drawback would be that > > where ipv6 = "ffff::1" > > and > > where ipv6 = convert("ffff::1" to binary) > > will work differently. And it's not particularly good either. I like the idea to treat all binary strings as raw with INET6, and then with UUID. I think this is not really a drawback. Remember, when we discussed "is [VAR]BINARY a data type or a character set" around the year of 2005, we fortunately decided that they are data types, so we introduced data types [VAR]BINARY as a replacement to "CHAR CHARACTER SET binary". So [VAR]CHAR abd [VAR]BINARY are different data types! It should be OK that INET6 will communicate with two ***different*** datatypes [VAR]CHAR vs [VAR]BINARY differently. If we agree on that, I'll change the code accordingly. We preliminary agreed on IRC to make explicit and implicit conversion between a data type and BINARY use methods in Type_handler. (but I have not added them yet). > > So, it's a tradeoff, and the question is - what is "less gotcha" ? > I don't know. I have a slight preference towards removing varbinary_to_raw() > just to make the API smaller. And thinking about adding it if there would be > user complains. > > Another thought. I'm starting to doubt that we should support direct > raw values in the SQL at all. It might work for ipv6 > > where ipv6 = 0xHHHHHH > > but it certainly won't work for, say, integers: > > where int = 0xHHHHH > > ^^^ this is not raw, but a normal hexadecimal number. > So, unless you introduce a special unambiguous syntax (like RAW(0xHHHHH)), > I think that raw values should not be supported in SQL. This syntax looks useful for me: WHERE inet6_field=0xFFFF; Btw, in case of integer numbers, it's exactly the low level raw format in Field. This is how integer numbers are stored, isn't it? I didn't understand why it won't work for numbers. Note, comparison to a hex hybrid works fine for INET6 in my patch and does not break the existing data types. Numeric comparion goes through Item_hex_hybrid::val_int(). String comparison goes through Item_hex_hybrid::val_str(). INET6 comparison goes through Item_hex_hybrid::val_raw(). This is Arg_comparator::func who decides which method to use. In the future, we could probably unify this: implement val_raw() for the traditional data types, and use val_raw() for comparison instead of the usual methods like val_int(). It's more tricky what should happen in case of VARCHAR-alike or BLOB-alike data types (suppose we'll decide the standard CLOB type in the future). Should val_raw() include the length bytes? I think no, as it will assume duplication of "length", once in String::length(), and one more time in the beginning of String::ptr(). So I was thinking of "raw" as something very close to how the data is stored in Field, but it's not necessary a 100% match. Perhaps we just need a different name than "raw"... I agree we don't need the field-style low level raw format (e.g. with length bytes for a VARCHAR-alike data type). > > > + virtual Item *make_cast(MEM_ROOT *mem_root, Item *arg) const = 0; > > Hmm. This implies an Item_typecast_xxx for every type. > I would be easier to have one Item_typecast_handled, that stores > a pointer to Type_handler. Would be much easier than implementing > new Item_typecast_xxx for every type. Some data types could probably share the same Item_typecast_xxx, but I don't think it's generally doable with a single Item for all data types. There are a few reasons why I prefer a separate method in handler to create a CAST Item. - Different data types can have different set of extra attributes, which the corresponding Item_typecast_xxx should store in. It's easier to store attributes in typed structures rather than in a generic memory pointed by "void *". It will make our life easier when we start debugging in gdb :) - I plan to simplify sql_yacc.yy so the loadable datatypes and the traditional datatypes share the same CAST and field_type grammar. It will be needed soon even for the traditional data types. - A method in handler will help to add the missing CAST types easier, e.g. FLOAT, SMALLINT, etc. It looks natural that every Field_xxx has a corresponding Item_typecast_xxx. > > > + int store_raw(const String *str, enum_field_types type_arg) > > > + { > > > + if (type_arg != type()) > > > + { > > > + set_null(); > > > + return 1; // TODO: go through str/int/real/dec/date? > > > + } > > > + DBUG_ASSERT(str->length() == binary_length()); > > > + memcpy(ptr, str->ptr(), binary_length()); > > > + return 0; > > > + } > > val_raw and store_raw should not be virtual, they are always > identical for all types. Just don't forget the validity check. <offtopic> Btw, it now looks like this: int store_raw_native(const char *str, uint length) { DBUG_ASSERT(length == binary_length()); memcpy(ptr, str, binary_length()); return 0; } I moved type conversion to the upper level. </offtopic> Right, the actual "store" is often a simple memcpy() for fixed length data types like INET6. But it's not generally the case. - It can be different for non-fixed length data types. It's again about the fact that the value of val_raw() and the byte array stored in Field::ptr() can have slighly differrent formats. Variable length types will pass the values using a normal String format, without additional length bytes coded inside the array pointed by String::ptr(). - Also, in case we have a shared method to store, we'll have to have additional virtual methods for validity check, to call inside. There's no sense for two virtual calls. - Also, in some cases it will be possible to use val_raw() instead of val_int(), val_decimal(), val_str(), val_real(), get_date(), and thus share almost duplicate code that's now implemented for different Item_result types. I already mentioned Arg_comparator. But there could also be only one universal Item_cache, etc. > > > + int store_time_dec(MYSQL_TIME *ltime, uint dec) > > > + { > > > + my_decimal tmp; > > > + return store_decimal(date2my_decimal(ltime, &tmp)); > > > + } > > Numerous val_xxx and store_xxx could be replaced by xxx_to_raw > and raw_to_xxx + val_raw and store_raw. You shouldn't introduce > both val_xxx and raw_to_xxx. Same for store_xxx and xxx_to_raw Right. I don't expose raw_to_xxx publically any more. They are now internal protected Type_handler_inet6 routines. > > > > + > > > + uint size_of() const { return sizeof(*this); } > > > + }; > > > + > > > + > > > + class Arg_comparator_inet6: public Arg_comparator > > > + { > > > + public: > > > + int compare_inet6() > > > + { > > > + if ((*a)->val_raw(&value1, cmp_field_type()) || > > > + (*b)->val_raw(&value2, cmp_field_type())) > > > + goto null; > > > + if (set_null) > > > + owner->null_value= 0; > > > + DBUG_ASSERT(value1.length() == Type_handler_inet6::binary_length()); > > > + DBUG_ASSERT(value2.length() == Type_handler_inet6::binary_length()); > > > + return memcmp(value1.ptr(), value2.ptr(), Type_handler_inet6::binary_length()); > > > + null: > > > + if (set_null) > > > + owner->null_value= 1; > > > + return -1; > > > + } > > > + int compare_e_inet6() > > > + { > > > + bool a_is_null= (*a)->val_raw(&value1, cmp_field_type()); > > > + bool b_is_null= (*b)->val_raw(&value2, cmp_field_type()); > > > + if (a_is_null || b_is_null) > > > + return MY_TEST(a_is_null == b_is_null); > > > + DBUG_ASSERT(value1.length() == Type_handler_inet6::binary_length()); > > > + DBUG_ASSERT(value2.length() == Type_handler_inet6::binary_length()); > > > + return MY_TEST(memcmp(value1.ptr(), value2.ptr(), Type_handler_inet6::binary_length()) == 0); > > > + } > > we should try to get rid of one of the functions, see below > ... > ... > ... > > > + int set_cmp_func(Arg_comparator *cmp, Item *a, Item *b) const > > > + { > > > + cmp->set_func(cmp->is_owner_equal_func() ? > > > + (arg_cmp_func) &Arg_comparator_inet6::compare_e_inet6 : > > > + (arg_cmp_func) &Arg_comparator_inet6::compare_inet6, > > > + field_type()); > > Again, boilerplate code, better: > > cmp->set_func(Arg_comparator_inet6::compare_e_inet6, > Arg_comparator_inet6::compare_inet6, > field_type()); > > no "cmp->is_owner_equal_func() ?" - this can be done inside set_func() > Also, I wonder, whether every type really needs two functions, for equal func > and not. Perhaps having only one - compare_e_inet6() should be sufficient and > checks for NULL can be done in the common wrapper? > And, even more, set_cmp_func() is not necessary at all. Type_handler > should only have compare_type() and compare_e_type() virtual methods > (or only one compare_e_type()) and the implementation of set_cmp_func() > becomes identical for all types, and thus doesn't have to be virtual > anymore. One method less to implement. I agree. We should try to do that. Let's try to decompose the functions of the existing data types and the INET6 related functions in a separate patch. > > > > + return 0; > > > + } > > > + void sortlength(struct st_sort_field *order, const Item *item) const > > > + { > > > + order->result_type= item->cmp_type(); > > > + order->length= Type_handler_inet6::binary_length(); > > > + order->suffix_length= 0; > > > + order->need_strxnfrm= 0; > > > + } > > > + bool make_sort_key(struct st_sort_field *order, uchar *to) const > > > + { > > > + Item *item= order->item; > > > + String buf((char *) to, order->length, &my_charset_bin); > > > + /* > > > + make_sortkey() uses str_result(), val_result(), val_int_result(), > > > + val_decimal_result() for the standard result types. > > > + QQ: should we introduce val_raw_result() and use here instead of > > > + val_raw() ? > > Or, perhaps, we should fix make_sortkey() to use val_str, val_real, etc. > I failed to understant why *result() methos are used there. So did I. But as I had to implement the val_raw_result() methods anyway for other purposes, it was easy to use val_raw_result() here, for symmetry with the other types, and in the latest patch I still use _result_ here. It can be fixed later for all data types to use the usual val_xxx() instead. > I've even replaced them with non-*result versions and not a single test > failed. Including the test case that was added when val* methods > were replaced with val*result methods. > > > > + > > > +Type_handler_register::Type_handler_register() > > > +{ > > > + memset(m_handler, 0, sizeof(m_handler)); > > > + m_min_type= 256; > > > + m_max_type= 0; > > > + m_min_length= 256; > > > + m_max_length= 0; > > > + add(&Type_handler_inet6::s_singleton); > > Eh? I thought it's added in the Type_handler_inet6 constructor. I think in the end there should be two separate steps actually: - a constructor for Type_handler_xxx - a method to add it into the registry So the plugin loading routine will first create a handler from a dynamic library, and then put it into the registry. Now I just implemented it by a special "class Type_handler_inet6_singleton".

2 4