- developers - lists.mariadb.org

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

13 Mar '15

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

1 0

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

13 Mar '15

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

2 5

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

12 Mar '15

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

10 14

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

12 Mar '15

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

2 2

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

11 Mar '15

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

1 0

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

11 Mar '15

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

1 0

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

11 Mar '15

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

2 1

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

11 Mar '15

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

2 1

Re: [Maria-developers] f954bb7: MDEV-7671: Cache VIEW definitions in the TDC
by Sergei Golubchik 11 Mar '15

11 Mar '15

Hi, Sanja! On Mar 10, sanja(a)mariadb.com wrote: > revision-id: f954bb7fdc527cf816253fe0e7adb4239c368624 > parent(s): f7d196bb2e605766668895027c48728f03803734 > committer: Oleksandr Byelkin > branch nick: server > timestamp: 2015-03-10 12:52:45 +0100 > message: > > MDEV-7671: Cache VIEW definitions in the TDC > > (changes of backported patch are very small: strlen removed, error > processing fixed, view open statistics added) Ok to push Regards, Sergei

1 0

[Maria-developers] GSOC 2015: Support for popular formats on Dynamic columns.
by arlene seuleu 11 Mar '15

11 Mar '15

Hi I am Arlene Flavie a software Engineering student and I wish to make my contribution to opensource through Mariadb. I have build mariadb from a git clone and ran the tests. I am interested in working on adding routines to import and export popular data formats to and from dynamic columns. I am currently studying the dynamic column data structures in mysql/ma_dyncol.** and I think what Oleksandr Byeklin proposed makes sense to me. Thats using methods like xxx_init() and xxx_named() to play with the old dynamic column blob format. I will be applying to work on this task for Gsoc 2015 and im hoping to learn much in open source development. Cheers.

2 1