February 2015 - developers - lists.mariadb.org

[Maria-developers] PLEASE REVIEW: (MDEV-7574) Security definer views don't work with CONNECT ODBC tables
by Alexander Barkov 26 Jul '15

26 Jul '15

Hello Sergei, Please review a patch that fixes the problem described in MDEV-7574. The idea is that a SELECT from a view over a CONNECT table now checks FILE privileges of the view definer (unless CREATE VIEW states SQL SECURITY INVOKER). It looks like a very good idea and gives more possible security options. The administrator can create a VIEW and give access to it to some user, without giving FILE privilege to this user. I'm not really sure about the patch, but it seems to do the trick :… [View More]) From what I understood, FILE_ACL is written (among the other privileges) into thd->security_ctx.privilege in TABLE_LIST::prepare_security(). In case of a DEFINER view, thd->security_ctx.privilege is filled exactly with the definer privileges, and to the invoker privileges otherwise. So inside ha_connect::check_privileges() the fact that there is FILE_ACL in thd->security_ctx.privilege means that TABLE_LIST::prepare_security() was previously called and FILE_ACL is set to DEFINER or INVOKER, according to the view definition. This is exactly what we need. I'm not sure about the opposite: if there is no FILE_ACL in thd->security_ctx.privilege, what does it mean? Does it mean that there is no FILE_ACL for the effective user? Or can it also mean that TABLE_LIST::prepare_security() was not called? Thanks. On 02/12/2015 04:04 AM, Olivier Bertrand wrote: > Hi Alexander, > > Can you take care of this issue? I know you wrote the > ha_connect::check_privileges function and I personnally have not the > faintest idea about how this must be checked. > > Thanks, > Olivier > > > -------- Message transféré -------- > Sujet : [JIRA] (MDEV-7574) Security definer views don't work with > CONNECT ODBC tables > Date : Wed, 11 Feb 2015 23:21:00 +0200 (EET) > De : Elena Stepanova (JIRA) <jira(a)mariadb.atlassian.net> > Pour : bertrandop(a)gmail.com > > > > [https://mariadb.atlassian.net/browse/MDEV-7574?page=com.atlassian.jira.plug… ] > > Elena Stepanova reassigned MDEV-7574: > ------------------------------------- > > Assignee: Olivier Bertrand > Fix Version/s: 10.0 > >> Security definer views don't work with CONNECT ODBC tables >> ---------------------------------------------------------- >> >> Key: MDEV-7574 >> URL:https://mariadb.atlassian.net/browse/MDEV-7574 >> Project: MariaDB Server >> Issue Type: Bug >> Components: Storage Engine - Connect >> Affects Versions: 10.0.16 >> Reporter: Geoff Montee >> Assignee: Olivier Bertrand >> Labels: connect-engine >> Fix For: 10.0 >> >> >> One possible way to get around the requirement for having the FILE privilege to access ODBC tables with CONNECT would be to have them called indirectly via a security definer view. However, it does not currently work. >> Create a security definer view to access the ODBC table, then create a new user: >> {code} >> [gmontee@localhost ~]$ mysql -u root tmp >> Reading table information for completion of table and column names >> You can turn off this feature to get a quicker startup with -A >> Welcome to the MariaDB monitor. Commands end with ; or \g. >> Your MariaDB connection id is 16 >> Server version: 10.0.15-MariaDB-log MariaDB Server >> Copyright (c) 2000, 2014, Oracle, SkySQL Ab and others. >> Type 'help;' or '\h' for help. Type '\c' to clear the current input statement. >> MariaDB [tmp]> SHOW CREATE TABLE datetime_table; >> +----------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ >> | Table | Create Table | >> +----------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ >> | datetime_table | CREATE TABLE `datetime_table` ( >> `id` int(10) NOT NULL, >> `modifiedon` datetime DEFAULT NULL >> ) ENGINE=CONNECT DEFAULT CHARSET=latin1 CONNECTION='DSN=connect_test_azure;UID=connect_test;PWD=Password1' `TABLE_TYPE`='ODBC' `TABNAME`='dbo.datetime_table' | >> +----------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ >> 1 row in set (0.00 sec) >> MariaDB [tmp]> DROP USER 'connecttest'@'localhost'; >> Query OK, 0 rows affected (0.00 sec) >> MariaDB [tmp]> CREATE OR REPLACE >> -> DEFINER = CURRENT_USER >> -> SQL SECURITY DEFINER >> -> VIEW datetime_view >> -> AS SELECT * FROM datetime_table; >> Query OK, 0 rows affected (0.00 sec) >> MariaDB [tmp]> CREATE USER 'connecttest'@'localhost'; >> Query OK, 0 rows affected (0.00 sec) >> MariaDB [tmp]> GRANT SELECT ON datetime_view TO 'connecttest'@'localhost'; >> Query OK, 0 rows affected (0.00 sec) >> MariaDB [tmp]> \q >> Bye >> {code} >> Now connect with the new user, and try to use the view: >> {code} >> [gmontee@localhost ~]$ mysql -u connecttest tmp >> Reading table information for completion of table and column names >> You can turn off this feature to get a quicker startup with -A >> Welcome to the MariaDB monitor. Commands end with ; or \g. >> Your MariaDB connection id is 17 >> Server version: 10.0.15-MariaDB-log MariaDB Server >> Copyright (c) 2000, 2014, Oracle, SkySQL Ab and others. >> Type 'help;' or '\h' for help. Type '\c' to clear the current input statement. >> MariaDB [tmp]> SELECT * FROM datetime_view; >> ERROR 1045 (28000): Access denied for user 'connecttest'@'localhost' (using password: NO) >> MariaDB [tmp]> \q >> Bye >> {code} >> It didn't work, so give the user privileges on the underlying ODBC table: >> {code} >> [gmontee@localhost ~]$ mysql -u root tmp >> Reading table information for completion of table and column names >> You can turn off this feature to get a quicker startup with -A >> Welcome to the MariaDB monitor. Commands end with ; or \g. >> Your MariaDB connection id is 18 >> Server version: 10.0.15-MariaDB-log MariaDB Server >> Copyright (c) 2000, 2014, Oracle, SkySQL Ab and others. >> Type 'help;' or '\h' for help. Type '\c' to clear the current input statement. >> MariaDB [tmp]> GRANT FILE ON *.* TO 'connecttest'@'localhost'; >> Query OK, 0 rows affected (0.00 sec) >> MariaDB [tmp]> GRANT SELECT ON datetime_table TO 'connecttest'@'localhost'; >> Query OK, 0 rows affected (0.00 sec) >> MariaDB [tmp]> \q >> Bye >> {code} >> Now try using the view again: >> {code} >> [gmontee@localhost ~]$ mysql -u connecttest tmp >> Reading table information for completion of table and column names >> You can turn off this feature to get a quicker startup with -A >> Welcome to the MariaDB monitor. Commands end with ; or \g. >> Your MariaDB connection id is 19 >> Server version: 10.0.15-MariaDB-log MariaDB Server >> Copyright (c) 2000, 2014, Oracle, SkySQL Ab and others. >> Type 'help;' or '\h' for help. Type '\c' to clear the current input statement. >> MariaDB [tmp]> SELECT * FROM datetime_view; >> +----+---------------------+ >> | id | modifiedon | >> +----+---------------------+ >> | 1 | 2014-01-01 00:00:00 | >> | 2 | 2016-01-01 00:00:00 | >> +----+---------------------+ >> 2 rows in set (0.24 sec) >> {code} > > > > -- > This message was sent by Atlassian JIRA > (v6.4-OD-14-082#64012) > > > [View Less]

2 9

[Maria-developers] Memory barrier problem in InnoDB/XtraDB mutex implementation causing hangs
by Kristian Nielsen 27 May '15

27 May '15

I have been debugging some test failures seen in Buildbot: https://mariadb.atlassian.net/browse/MDEV-7026 After some debugging, this is what I think is going on. I will refer to MariaDB 5.5 sources, though the problem is also in 10.0. InnoDB/XtraDB mutexes are taken in mutex_enter_func() and unlocked in mutex_exit_func(). Simplifying for clarity, here are the relevant parts of the logic: ENTER: # First spin wait, hoping to get the mutex without yielding. os_compare_and_swap_ulint(… [View More]&mutex->waiters, 0, 1); if (os_atomic_test_and_set_byte(&mutex->lock_word, 1)) sync_array_wait_event() RELEASE: os_atomic_lock_release_byte(&mutex->lock_word); os_rmb; waiters = *(volatile *)&mutex->waiters; if (waiters) mutex_signal_object(mutex); The interesting point here is how to ensure that we do not lose the wakeup of a waiting thread when the holding thread releases the mutex. The idea is that the waiting thread first sets mutex->waiters=1. Then it checks if the mutex is free - if not, it goes to sleep. The thread that releases the lock first marks the mutex free. Then it checks if there are any waiters, and if so wakes them up. For this to work, we need to ensure that the setting of waiters=1 has completed before the load of lock_word can start - a full memory barrier. Likewise, we need the setting of lock_word to 0 to complete before the load of mutex->waiters - again a full memory barrier. Otherwise we can get something like this: ENTER RELEASE Start store to mutex->waiters Start store to lock_word Load mutex->lock_word Store to lock_word becomes visible Load mutex->waiters Store to waiters becomes visible Check lock word==1 Check waiters==0, skip wakeup Sleep (forever) Or this: ENTER RELEASE Start store to lock_word Start store to mutex->waiters Load mutex->waiters Store to waiters becomes visible Load mutex->lock_word Store to lock_word becomes visible Check lock word==1 Sleep (forever) Check waiters==0, skip wakeup (So both full barriers are needed, the one in ENTER to prevent the first example, the one in RELEASE to prevent the second). It looks like for x86, the full barriers were broken in MariaDB 5.5.40 and 10.0.13. os_compare_and_swap_ulint() is defined as either __sync_bool_compare_and_swap() or atomic_cas_ulong(), both of which implies a full barrier. So ENTER looks ok. os_atomic_lock_release_byte() however is now defined as __sync_lock_release(). This is _not_ a full barrier, just a release barrier. So the RELEASE case seems to be incorrect, allowing the second deadlock example to occur. Previously, RELEASE used os_atomic_test_and_set_byte(), which is __sync_lock_test_and_set(). This is defined in GCC docs as an "aquire" barrier only, which is also wrong, obviously. But it happens to generate an XCHG instruction on x86, which is a full barrier. This is probably why it works on x86 (and probably why it did not work on PowerPC). Here is a small test program, and the assembly from GCC 5.7 on amd64: void test(unsigned *p, unsigned *q) { while (__sync_lock_test_and_set(q, 1)) { } *p++; __sync_lock_release(q); } test: movl $1, %edx .L2: movl %edx, %eax xchgl (%rsi), %eax testl %eax, %eax jne .L2 movl $0, (%rsi) ret As can be seen, the __sync_lock_test_and_set() generates a full barrier, but the __sync_lock_release() generates nothing (as this is sufficient for release semantics for the rather strong memory order on x86). So the InnoDB mutexes look completely broken from MariaDB 5.5.40 and 10.0.13 :-( This is consistent with the observations in MDEV-7026. A thread is waiting for wakeup on a mutex that is already marked free. Apparently this is not a new problem. This comment in mutex_exit_func() makes me want to cry: /* A problem: we assume that mutex_reset_lock word is a memory barrier, that is when we read the waiters field next, the read must be serialized in memory after the reset. A speculative processor might perform the read first, which could leave a waiting thread hanging indefinitely. Our current solution call every second sync_arr_wake_threads_if_sema_free() to wake up possible hanging threads if they are missed in mutex_signal_object. */ So someone found the mutex implementation broken, but instead of fixing it, they implemented a horrible workaround to try and break any deadlocks that might occur. (The reason that the deadlocks are not broken in the MDEV-7026 seems to be that it occurs during startup, before starting the srv_error_monitor_thread() that does the sync_arr_wake_threads_if_sema_free()). Now, what is also interesting is that this seems to explain very nicely the many server lockups we have seen for users that upgrade to recent MariaDB versions. I was doing a very detailed investigation of one such case where the user was able to supply unusually detailed information. I found a thread that was stuck waiting on a mutex, but no other threads seemed to be holding that mutex. This is exactly what would be seen in case of a lost wakeup due to this issue. Furthermore, the sync_arr_wake_threads_if_sema_free() mechanism, as well as the normal InnoDB "long semaphore wait", was itself blocked on that same mutex wait, which explains why they were not effective for the user in that particular case. So it looks like we need to revert the incorrect patch and release new 5.5.40a and 10.0.15 releases ASAP, to solve these hangs for users. And do a proper patch for PowerPC with proper full memory barriers. (Incidentally, the os_rmb in RELEASE seems to also be part of some PowerPC patch, it also looks completely wrong). However, more importantly, this issue clearly shows some fundamental problems in our development procedures that we need to fix ASAP. Just look at it. We have users (and customers!) that suffer from strange hangs. This is their production systems that are down. Support and developers are unable to help them due to the difficulty of debugging rare race conditions in production systems. And all the time the bug was staring us in the face, easily reproducable in our own Buildbot! But we deliberately chose to ignore those failures. The only thing that matters is "when can you push", and getting more releases out. Never mind what kind of crap is then pushed out to users. I am deeply ashamed of treating our users like this. And extremely unhappy. I even wrote about this (the importance of not ignoring Buildbot failures) just a few weeks ago. This was completely ignored, I got zero replies, and the weekly calls continued with mindless juggling of Jira numbers and buggy releases. As a result, our users suffer. We need to stop this. Please? - Kristian. [View Less]

4 16

Re: [Maria-developers] Rev 4407: MDEV-6916: Upgrade from MySQL to MariaDB breaks already created views
by Sergei Golubchik 25 Apr '15

25 Apr '15

Hi, Sanja! On Feb 09, sanja(a)askmonty.org wrote: > At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916-check_view/ > > ------------------------------------------------------------ > revno: 4407 > revision-id: sanja(a)askmonty.org-20150209014828-kx3cr36hgrvjhtrg > parent: svoj(a)mariadb.org-20150114135038-v50g2cul4vce63h8 > committer: sanja(a)askmonty.org > branch nick: work-maria-5.5-MDEV-6916-check_view > timestamp: Mon 2015-02-09 02:48:28 +0100 > message: … [View More]> MDEV-6916: Upgrade from MySQL to MariaDB breaks already created views > > CHECK/REPAIR commands and mysql_upgradesupport for upgrade from MySQL server support. > === modified file 'client/mysql_upgrade.c' > --- a/client/mysql_upgrade.c 2014-03-27 21:26:58 +0000 > +++ b/client/mysql_upgrade.c 2015-02-09 01:48:28 +0000 > @@ -150,6 +151,14 @@ static struct my_option my_long_options[ > &opt_not_used, &opt_not_used, 0, GET_BOOL, NO_ARG, 1, 0, 0, 0, 0, 0}, > {"version", 'V', "Output version information and exit.", 0, 0, 0, > GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}, > + {"mysql-upgrade", 'y', > + "Skip automatic detection MySQL and assume that we upgrade it", > + &opt_mysql_upgrade, &opt_mysql_upgrade, 0, GET_BOOL, NO_ARG, > + 0, 0, 0, 0, 0, 0}, > + {"skip-mysql-upgrade", 'Y', > + "Skip view algorithm upgrade from MySQL", > + &opt_skip_mysql_upgrade, &opt_skip_mysql_upgrade, 0, GET_BOOL, NO_ARG, > + 0, 0, 0, 0, 0, 0}, I remember I've already commented on that in my previous review. Here you are, again: === Those two are very bad option names. First, $ mysql_upgrade --mysql-upgrade looks plain weird. $ mysql_upgrade --skip-mysql-upgrade is even worse. And what about $ mysql_upgrade --mysql-upgrade --skip-mysql-upgrade Second, "skip" is the prefix automatically recognized by my_getopt. Normally, --skip-xxx is the same as --xxx=0. But now you're introducing --skip-mysql-upgrade which is very different from --skip-mysql-upgrade. While simultaneously adding support for --skip-skip-mysql-upgrade. A better option would be --fix-view-algorithm = { AUTO | ALWAYS | NEVER } === In fact, I'd rather remove this option completely. You can simply add if (!is_mysql()) return; to run_mysqlcheck_views() > {"version-check", 'k', "Run this program only if its \'server version\' " > "matches the version of the server to which it's connecting, (enabled by " > "default); use --skip-version-check to avoid this check. Note: the \'server " > @@ -344,6 +353,14 @@ get_one_option(int optid, const struct m > case OPT_DEFAULT_AUTH: /* --default-auth */ > add_one_option(&conn_args, opt, argument); > break; > + case 'y': > + opt_mysql_upgrade= 1; > + add_option= FALSE; > + break; > + case 'Y': > + opt_skip_mysql_upgrade= 1; > + add_option= FALSE; This is a wrong way of implementing command-line options. Let my_getopt do its job and don't set values manually. > + break; > } > > if (add_option) > @@ -754,6 +771,23 @@ static int run_mysqlcheck_upgrade(void) > NULL); > } > > +static int run_mysqlcheck_views(void) > +{ > + if (!opt_mysql_upgrade) > + return 0; > + verbose("Phase 0: Fixing views"); Again, was in my previous review: === No "Phase 0" please, renumber all phases to start from 1. === > + print_conn_args("mysqlcheck"); > + return run_tool(mysqlcheck_path, > + NULL, /* Send output from mysqlcheck directly to screen */ > + "--no-defaults", > + ds_args.str, > + "--all-databases", > + "--mysql-upgrade", > + opt_verbose ? "--verbose": "", > + opt_silent ? "--silent": "", > + "2>&1", > + NULL); > +} > > static int run_mysqlcheck_fixnames(void) > { > === modified file 'client/mysqlcheck.c' > --- a/client/mysqlcheck.c 2014-02-17 10:00:51 +0000 > +++ b/client/mysqlcheck.c 2015-02-09 01:48:28 +0000 > @@ -196,6 +197,9 @@ static struct my_option my_long_options[ > NO_ARG, 0, 0, 0, 0, 0, 0}, > {"version", 'V', "Output version information and exit.", 0, 0, 0, GET_NO_ARG, > NO_ARG, 0, 0, 0, 0, 0, 0}, > + {"mysql-upgrade", 'y', > + "Fix view algorithm view field if it is not new MariaDB view.", > + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}, > {0, 0, 0, 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0} > }; > > @@ -332,7 +336,13 @@ get_one_option(int optid, const struct m > case 'v': > verbose++; > break; > - case 'V': print_version(); exit(0); > + case 'V': > + print_version(); exit(0); > + break; > + case 'y': > + what_to_do= DO_REPAIR; > + opt_mysql_upgrade= 1; > + break; See above. > case OPT_MYSQL_PROTOCOL: > opt_protocol= find_type_or_exit(argument, &sql_protocol_typelib, > opt->name); > @@ -595,7 +605,15 @@ static int process_all_tables_in_db(char > for (end = tables + 1; (row = mysql_fetch_row(res)) ;) > { > if ((num_columns == 2) && (strcmp(row[1], "VIEW") == 0)) > - continue; > + { > + if (!opt_mysql_upgrade) > + continue; > + } > + else > + { > + if (opt_mysql_upgrade) > + continue; Eh? If --mysql-upgrade is specified you skip all tables and only upgrade views? Then the option should absolutely be called --fix-view-algorithm > + } > > end= fix_table_name(end, row[0]); > *end++= ','; > @@ -756,10 +782,12 @@ static int handle_request_for_tables(cha > if (opt_upgrade) end = strmov(end, " FOR UPGRADE"); > break; > case DO_REPAIR: > - op= (opt_write_binlog) ? "REPAIR" : "REPAIR NO_WRITE_TO_BINLOG"; > + op= ((opt_write_binlog || opt_mysql_upgrade) ? > + "REPAIR" : "REPAIR NO_WRITE_TO_BINLOG"); Really? For view upgrades you want it to be written to binlog? This is a very questionable idea. > if (opt_quick) end = strmov(end, " QUICK"); > if (opt_extended) end = strmov(end, " EXTENDED"); > if (opt_frm) end = strmov(end, " USE_FRM"); > + if (opt_mysql_upgrade) end = strmov(end, " FROM MYSQL"); > break; > case DO_ANALYZE: > op= (opt_write_binlog) ? "ANALYZE" : "ANALYZE NO_WRITE_TO_BINLOG"; > @@ -776,14 +804,17 @@ static int handle_request_for_tables(cha > if (opt_all_in_1) > { > /* No backticks here as we added them before */ > - query_length= sprintf(query, "%s TABLE %s %s", op, tables, options); > + query_length= sprintf(query, "%s %s %s %s", op, > + (opt_mysql_upgrade ? "VIEW" : "TABLE"), this is wrong, because mysqlcheck --mysql-upgrade (or, really, mysqcheck --fix-view-algorithm=xxx) should check/repair *both* tables and views. > + tables, options); > table_name= tables; > } > else > { > char *ptr, *org; > > - org= ptr= strmov(strmov(query, op), " TABLE "); > + org= ptr= strmov(strmov(query, op), > + (opt_mysql_upgrade ? " VIEW " : " TABLE ")); > ptr= fix_table_name(ptr, tables); > strmake(table_name_buff, org, min((int) sizeof(table_name_buff)-1, > (int) (ptr - org))); > > === modified file 'mysql-test/r/view.result' > --- a/mysql-test/r/view.result 2014-12-21 18:23:28 +0000 > +++ b/mysql-test/r/view.result 2015-02-09 01:48:28 +0000 ... > +check view v1,v2,v3,v4,v5 FOR UPGRADE; > +Table Op Msg_type Msg_text > +test.v1 check Note view 'test.v1' has no field mariadb server in its .frm file > +test.v1 check status needs repair > +test.v2 check Note view 'test.v2' has no field mariadb server in its .frm file > +test.v2 check status needs repair > +test.v3 check Note view 'test.v3' has no field mariadb server in its .frm file > +test.v3 check status needs repair > +test.v4 check note View text checksum failed > +test.v5 check status OK > +repair view v1,v2,v3,v4,v5 FROM MYSQL; where's the test for "repair view" without FROM MYSQL ? > +Table Op Msg_type Msg_text > +test.v1 repair status view is repaired > +test.v2 repair status view is repaired > +test.v3 repair status view is repaired > +test.v4 repair status view is repaired > +test.v5 repair status OK > > === modified file 'sql/handler.h' > --- a/sql/handler.h 2015-01-13 18:28:03 +0000 > +++ b/sql/handler.h 2015-02-09 01:48:28 +0000 > @@ -42,6 +42,7 @@ > > // the following is for checking tables > > +#define HA_ADMIN_VIEW_REPAIR_IS_DONE 2 > #define HA_ADMIN_ALREADY_DONE 1 > #define HA_ADMIN_OK 0 > #define HA_ADMIN_NOT_IMPLEMENTED -1 > @@ -56,6 +57,7 @@ > #define HA_ADMIN_NEEDS_UPGRADE -10 > #define HA_ADMIN_NEEDS_ALTER -11 > #define HA_ADMIN_NEEDS_CHECK -12 > +#define HA_ADMIN_NEEDS_REPAIR -13 > > /* Bits in table_flags() to show what database can do */ > > === modified file 'sql/share/errmsg-utf8.txt' > --- a/sql/share/errmsg-utf8.txt 2014-05-27 06:45:01 +0000 > +++ b/sql/share/errmsg-utf8.txt 2015-02-09 01:48:28 +0000 > @@ -6565,3 +6565,9 @@ ER_QUERY_EXCEEDED_ROWS_EXAMINED_LIMIT > ER_NO_SUCH_TABLE_IN_ENGINE 42S02 > eng "Table '%-.192s.%-.192s' doesn't exist in engine" > swe "Det finns ingen tabell som heter '%-.192s.%-.192s' i handlern" > +ER_NO_MARIADB_SERVER_FIELD > + eng "view '%-.192s.%-.192s' has no field mariadb server in its .frm file" First, it's "mariadb_version", not "mariadb_server". Second, I'm not sure such a specific condition needs a dedicated error code. And last - we absolutely cannot add new error messages in 5.5, because 10.0 is already GA. I've already written that in my previous review. > +ER_VIEW_REPAIR_IS_DONE > + eng "view is repaired" > +ER_NEEDS_REPAIR > + eng "needs repair" > > === modified file 'sql/sql_admin.cc' > --- a/sql/sql_admin.cc 2014-05-03 16:12:17 +0000 > +++ b/sql/sql_admin.cc 2015-02-09 01:48:28 +0000 > @@ -867,6 +879,22 @@ send_result_message: > fatal_error=1; > break; > } > + case HA_ADMIN_VIEW_REPAIR_IS_DONE: Why did you need that, what's wrong with HA_ADMIN_OK? Btw, I've already written that in my previous review. > + { > + protocol->store(STRING_WITH_LEN("status"), system_charset_info); > + protocol->store(ER(ER_VIEW_REPAIR_IS_DONE), > + strlen(ER(ER_VIEW_REPAIR_IS_DONE)), > + system_charset_info); > + break; > + } > + case HA_ADMIN_NEEDS_REPAIR: Why did you need that, what's wrong with HA_ADMIN_NEEDS_UPGRADE? > + { > + protocol->store(STRING_WITH_LEN("status"), system_charset_info); > + protocol->store(ER(ER_NEEDS_REPAIR), > + strlen(ER(ER_NEEDS_REPAIR)), > + system_charset_info); > + break; > + } > > default: // Probably HA_ADMIN_INTERNAL_ERROR > { > === modified file 'sql/sql_base.cc' > --- a/sql/sql_base.cc 2014-10-31 13:07:29 +0000 > +++ b/sql/sql_base.cc 2015-02-09 01:48:28 +0000 > @@ -3019,12 +3020,17 @@ bool open_table(THD *thd, TABLE_LIST *ta > else if (table_list->open_strategy == TABLE_LIST::OPEN_STUB) > DBUG_RETURN(FALSE); > > + second empty line? > retry_share: > > mysql_mutex_lock(&LOCK_open); > > if (!(share= get_table_share_with_discover(thd, table_list, key, > - key_length, OPEN_VIEW, > + key_length, > + (OPEN_VIEW | > + ((table_list->required_type == > + FRMTYPE_VIEW) ? > + OPEN_VIEW_ONLY : 0)), > &error, > hash_value))) > { > > === modified file 'sql/sql_table.cc' > --- a/sql/sql_table.cc 2015-01-14 11:10:13 +0000 > +++ b/sql/sql_table.cc 2015-02-09 01:48:28 +0000 > @@ -28,7 +28,7 @@ > #include "sql_base.h" // open_table_uncached, lock_table_names > #include "lock.h" // mysql_unlock_tables > #include "strfunc.h" // find_type2, find_set > -#include "sql_view.h" // view_checksum > +#include "sql_view.h" // view_check You've modified only the comment, not the actual C++ code that uses view_checksum in this file. So, either the comment is wrong (view_checksum is not used in this file) and should be removed, or the whole #include is unnecessary. > #include "sql_truncate.h" // regenerate_locked_table > #include "sql_partition.h" // mem_alloc_error, > // generate_partition_syntax, > > === modified file 'sql/sql_view.cc' > --- a/sql/sql_view.cc 2014-12-21 18:23:28 +0000 > +++ b/sql/sql_view.cc 2015-02-09 01:48:28 +0000 > @@ -729,6 +729,26 @@ err: > } > > > +static void make_view_filename(LEX_STRING *dir, char *dir_buff, > + size_t dir_buff_len, > + LEX_STRING *path, char *path_buff, > + size_t path_buff_len, > + LEX_STRING *file, > + TABLE_LIST *view) > +{ > + /* print file name */ > + dir->length= build_table_filename(dir_buff, dir_buff_len - 1, > + view->db, "", "", 0); > + dir->str= dir_buff; > + > + path->length= build_table_filename(path_buff, path_buff_len - 1, > + view->db, view->table_name, reg_ext, 0); > + path->str= path_buff; > + > + file->str= path->str + dir->length; > + file->length= path->length - dir->length; > +} > + > /* number of required parameters for making view */ > static const int required_view_parameters= 15; > > @@ -791,6 +811,81 @@ static File_option view_parameters[]= > static LEX_STRING view_file_type[]= {{(char*) STRING_WITH_LEN("VIEW") }}; > > > +int mariadb_fix_view(THD *thd, TABLE_LIST *view, bool wrong_checksum, > + bool swap_alg) > +{ > + char dir_buff[FN_REFLEN + 1], path_buff[FN_REFLEN + 1]; > + LEX_STRING dir, file, path; > + DBUG_ENTER("mariadb_fix_view"); > + > + if (view->algorithm == VIEW_ALGORITHM_UNDEFINED && > + !wrong_checksum && view->mariadb_version) > + DBUG_RETURN(HA_ADMIN_OK); Eh? Why do you care what view->algorithm is? It doesn't matter whether it's undefined, merge, or temptable - if mariadb_version is set, the algorithm is always correct. > + make_view_filename(&dir, dir_buff, sizeof(dir_buff), > + &path, path_buff, sizeof(path_buff), > + &file, view); > + /* init timestamp */ > + if (!view->timestamp.str) > + view->timestamp.str= view->timestamp_buffer; > + > + /* check old .frm */ > + { > + char path_buff[FN_REFLEN]; > + LEX_STRING path; > + File_parser *parser; > + > + path.str= path_buff; > + fn_format(path_buff, file.str, dir.str, "", MY_UNPACK_FILENAME); > + path.length= strlen(path_buff); > + if (access(path.str, F_OK)) > + DBUG_RETURN(HA_ADMIN_INVALID); > + > + if (!(parser= sql_parse_prepare(&path, thd->mem_root, 0))) > + DBUG_RETURN(HA_ADMIN_INTERNAL_ERROR); > + > + if (!parser->ok() || !is_equal(&view_type, parser->type())) > + DBUG_RETURN(HA_ADMIN_INVALID); > + } > + > + if (swap_alg && view->algorithm != VIEW_ALGORITHM_UNDEFINED) only if mariadb_version is not set. > + { > + DBUG_ASSERT(view->algorithm == VIEW_ALGORITHM_MERGE || > + view->algorithm == VIEW_ALGORITHM_TMPTABLE); > + if (view->algorithm == VIEW_ALGORITHM_MERGE) > + view->algorithm= VIEW_ALGORITHM_TMPTABLE; > + else > + view->algorithm= VIEW_ALGORITHM_MERGE; > + } > + if (wrong_checksum) > + { > + if (view->md5.length != 32) > + { > + if ((view->md5.str= (char *)thd->alloc(32 + 1)) == NULL) > + DBUG_RETURN(HA_ADMIN_FAILED); > + } > + view->calc_md5(view->md5.str); > + view->md5.length= 32; > + } > + view->mariadb_version= MYSQL_VERSION_ID; > + > + if (sql_create_definition_file(&dir, &file, view_file_type, > + (uchar*)view, view_parameters)) > + { > + sql_print_error("View '%-.192s'.'%-.192s': algorithm swap error.", > + view->db, view->table_name); > + DBUG_RETURN(HA_ADMIN_INTERNAL_ERROR); > + } > + sql_print_information("View '%-.192s'.'%-.192s': algorithm swapped to '%s'", > + view->db, view->table_name, > + (view->algorithm == VIEW_ALGORITHM_MERGE)? > + "MERGE":"TEMPTABLE"); > + > + > + DBUG_RETURN(HA_ADMIN_VIEW_REPAIR_IS_DONE); > +} > + > + > /* > Register VIEW (write .frm & process .frm's history backups) > > @@ -927,17 +1022,9 @@ static int mysql_register_view(THD *thd, > } > loop_out: > /* print file name */ > - dir.length= build_table_filename(dir_buff, sizeof(dir_buff) - 1, > - view->db, "", "", 0); > - dir.str= dir_buff; > - > - path.length= build_table_filename(path_buff, sizeof(path_buff) - 1, > - view->db, view->table_name, reg_ext, 0); > - path.str= path_buff; > - > - file.str= path.str + dir.length; > - file.length= path.length - dir.length; > - > + make_view_filename(&dir, dir_buff, sizeof(dir_buff), > + &path, path_buff, sizeof(path_buff), > + &file, view); > /* init timestamp */ > if (!view->timestamp.str) > view->timestamp.str= view->timestamp_buffer; > @@ -1941,6 +2028,63 @@ int view_checksum(THD *thd, TABLE_LIST * > HA_ADMIN_OK); > } > > +/** > + Check view > + > + @param thd thread handle > + @param view view for check > + @param check_opt check options > + > + @retval HA_ADMIN_OK OK > + @retval HA_ADMIN_NOT_IMPLEMENTED it is not VIEW > + @retval HA_ADMIN_WRONG_CHECKSUM check sum is wrong > +*/ > +int view_check(THD *thd, TABLE_LIST *view, HA_CHECK_OPT *check_opt) > +{ > + int res; > + DBUG_ENTER("view_check"); > + if ((res= view_checksum(thd, view)) != HA_ADMIN_OK) > + DBUG_RETURN(res); > + if (((check_opt->sql_flags & TT_FOR_UPGRADE) && > + !view->mariadb_version)) > + { > + push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_NOTE, > + ER_NO_MARIADB_SERVER_FIELD, > + ER(ER_NO_MARIADB_SERVER_FIELD), > + view->db, > + view->table_name); > + DBUG_RETURN(HA_ADMIN_NEEDS_REPAIR); > + } > + DBUG_RETURN(HA_ADMIN_OK); > +} > + > + > +/** > + Repair view > + > + @param thd thread handle > + @param view view for check > + @param check_opt check options > + > + @retval HA_ADMIN_OK OK > + @retval HA_ADMIN_NOT_IMPLEMENTED it is not VIEW > + @retval HA_ADMIN_WRONG_CHECKSUM check sum is wrong > +*/ > + > +int view_repair(THD *thd, TABLE_LIST *view, HA_CHECK_OPT *check_opt) > +{ > + DBUG_ENTER("view_repair"); > + bool swap_alg= > + ((check_opt->sql_flags & TT_FROM_MYSQL) && > + (!view->mariadb_version)); > + bool wrong_checksum= view_checksum(thd, view); > + if (wrong_checksum || swap_alg) > + { > + DBUG_RETURN(mariadb_fix_view(thd, view, wrong_checksum, swap_alg)); First, don't call functions from DBUG_RETURN, this produces weird debug traces and somebody will have to fix this later. Always int res= mariadb_fix_view(thd, view, wrong_checksum, swap_alg); DBUG_RETURN(res); Alternatively, feel free to fix dbug macros to support function calls in DBUG_RETURN() :) Second, REPAIR VIEW viewname - without FROM MYSQL - should work too, it should add mariadb_version field without changing the algorithm. > + } > + DBUG_RETURN(HA_ADMIN_OK); > +} > + > /* > rename view > > === modified file 'sql/sql_yacc.yy' > --- a/sql/sql_yacc.yy 2014-11-08 18:54:42 +0000 > +++ b/sql/sql_yacc.yy 2015-02-09 01:48:28 +0000 > @@ -1145,6 +1145,7 @@ bool my_yyoverflow(short **a, YYSTYPE ** > %token MULTIPOINT > %token MULTIPOLYGON > %token MUTEX_SYM > +%token MYSQL_SYM > %token MYSQL_ERRNO_SYM > %token NAMES_SYM /* SQL-2003-N */ > %token NAME_SYM /* SQL-2003-N */ > @@ -7191,11 +7192,16 @@ opt_checksum_type: > ; > > repair: > - REPAIR opt_no_write_to_binlog table_or_tables > + REPAIR opt_no_write_to_binlog table_or_view > { > LEX *lex=Lex; > lex->sql_command = SQLCOM_REPAIR; > lex->no_write_to_binlog= $2; > + if (lex->no_write_to_binlog && lex->only_view) > + { > + my_parse_error(ER(ER_SYNTAX_ERROR)); > + MYSQL_YYABORT; Why? REPAIR NO_WRITE_TO_BINLOG VIEW makes perfect sense to me, why did you want to disallow it? > + } > lex->check_opt.init(); > lex->alter_info.reset(); > /* Will be overriden during execution. */ Regards, Sergei [View Less]

4 8

[Maria-developers] TokuDB test suite failures when building at Launchpad
by Otto Kekäläinen 29 Mar '15

29 Mar '15

Hello! Are here any TokuDB devs who could help me debug why TokuDB does repeatedly fails to pass build suite tests when built on Launchpad.net? I repeatedly get these test failures when running MariaDB builds on Launchpad: rpl-tokudb.tokudb_innodb_xa_crash tokudb_bugs.mdev4533 tokudb.simple_join_tokudb_innodb tokudb.truncate_txn_rollback_innodb tokudb_bugs.5733_innodb The build run log can be viewed at https://launchpad.net/~mysql-ubuntu/+archive/ubuntu/mariadb-10.0/+builds?bu… and it links … [View More]

4 15

Re: [Maria-developers] [Commits] 83659a6: MDEV-7006 MDEV-7007: SET STATEMENT and slow log
by Sergei Golubchik 10 Mar '15

10 Mar '15

Hi, Sanja! On Feb 17, sanja(a)mariadb.com wrote: > revision-id: 83659a68642da2eb6e9327f17a0ef730f512aa96 > parent(s): 3e2849d2a01b06a61407b00989c3f981e62dd183 > committer: Oleksandr Byelkin > branch nick: server > timestamp: 2015-02-17 12:54:51 +0100 > message: > > MDEV-7006 MDEV-7007: SET STATEMENT and slow log > fixed embedded server tests > MDEV-7009: SET STATEMENT min_examined_row_limit has no effect > MDEV-6948:SET STATEMENT gtid_domain_id = ... FOR has … [View More]no effect (same for gtid_seq_no and server_id) > > old values of SET STATENENT variables now saved in its own Query_arena and restored later Looks good to me. I didn't check, though, whether you've put every restore correctly. See minor comments below: > diff --git a/mysql-test/t/set_statement_notembedded_binlog.test b/mysql-test/t/set_statement_notembedded_binlog.test > new file mode 100644 > index 0000000..c9c2334 > --- /dev/null > +++ b/mysql-test/t/set_statement_notembedded_binlog.test > @@ -0,0 +1,22 @@ > +--source include/have_log_bin.inc > +--source include/not_embedded.inc > + > +--disable_warnings > +drop table if exists t1; > +drop view if exists t1; > +--enable_warnings This was useful in our early MySQL days, but it's not anymore. I've stopped adding these drop...if exists at the beginning of test files a couple of years ago. mtr has a after-test check to ensure that no test leaves its tables behind. So one can rely on every test starting clean. > + > +--echo # > +--echo # MDEV-6948: SET STATEMENT gtid_domain_id = ... FOR has no effect > +--echo # (same for gtid_seq_no and server_id) > +--echo # > +reset master; > +create table t1 (i int); > +set gtid_domain_id = 10; > +insert into t1 values (1),(2); > +set statement gtid_domain_id = 20 for insert into t1 values (3),(4); > + > +--replace_column 1 x 2 x 3 x 4 x 5 x > +show binlog events limit 5,5; > + > +drop table t1; > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index 0c1c3b3..ee956ed 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -4223,6 +4224,56 @@ int LEX::print_explain(select_result_sink *output, uint8 explain_flags, > return res; > } > > +void LEX::set_arena_for_set_stmt(Query_arena *backup) > +{ > + DBUG_ENTER("LEX::set_arena_for_set_stmt"); > + DBUG_ASSERT(arena_for_set_stmt== 0); > + if (!mem_root_for_set_stmt) > + { > + mem_root_for_set_stmt= new MEM_ROOT(); > + if (!(mem_root_for_set_stmt)) > + DBUG_VOID_RETURN; What happens in OOM case? You return early, but does the caller know that? Or it'll continue, thinking that everything's ok? > + init_sql_alloc(mem_root_for_set_stmt, ALLOC_ROOT_SET, ALLOC_ROOT_SET, > + MYF(MY_THREAD_SPECIFIC)); > + } > + if (!(arena_for_set_stmt= new Query_arena(mem_root_for_set_stmt, Why are you allocating memory for Query_arena, instead of creating it on mem_root_for_set_stmt? > + Query_arena::STMT_INITIALIZED))) > + DBUG_VOID_RETURN; > + DBUG_PRINT("info", ("mem_root: 0x%lx arena: 0x%lx", > + (ulong) mem_root_for_set_stmt, > + (ulong) arena_for_set_stmt)); > + thd->set_n_backup_active_arena(arena_for_set_stmt, backup); > + DBUG_VOID_RETURN; > +} > + > + > +void LEX::reset_arena_for_set_stmt(Query_arena *backup) > +{ > + DBUG_ENTER("LEX::reset_arena_for_set_stmt"); > + DBUG_ASSERT(arena_for_set_stmt); > + thd->restore_active_arena(arena_for_set_stmt, backup); > + DBUG_PRINT("info", ("mem_root: 0x%lx arena: 0x%lx", > + (ulong) arena_for_set_stmt->mem_root, > + (ulong) arena_for_set_stmt)); > + DBUG_VOID_RETURN; > +} > + > + > +void LEX::free_arena_for_set_stmt() > +{ > + DBUG_ENTER("LEX::free_arena_for_set_stmt"); > + if (!arena_for_set_stmt) > + return; > + DBUG_PRINT("info", ("mem_root: 0x%lx arena: 0x%lx", > + (ulong) arena_for_set_stmt->mem_root, > + (ulong) arena_for_set_stmt)); > + arena_for_set_stmt->free_items(); > + delete(arena_for_set_stmt); > + free_root(mem_root_for_set_stmt, MYF(MY_KEEP_PREALLOC)); > + arena_for_set_stmt= 0; > + DBUG_VOID_RETURN; > +} > + > void LEX::restore_set_statement_var() > { > DBUG_ENTER("LEX::restore_set_statement_var"); > diff --git a/sql/sql_lex.h b/sql/sql_lex.h > index 46d667c..ed3c4a0 100644 > --- a/sql/sql_lex.h > +++ b/sql/sql_lex.h > @@ -2677,10 +2688,22 @@ struct LEX: public Query_tables_list > limit_rows_examined_cnt= ULONGLONG_MAX; > } > > + > + inline void free_set_stmt_mem_root() > + { Please, add DBUG_ASSERT(!is_arena_for_set_stmt()); > + if (mem_root_for_set_stmt) > + { > + free_root(mem_root_for_set_stmt, MYF(0)); > + delete mem_root_for_set_stmt; > + mem_root_for_set_stmt= 0; > + } > + } > + > LEX(); > > virtual ~LEX() > { > + free_set_stmt_mem_root(); > destroy_query_tables_list(); > plugin_unlock_list(NULL, (plugin_ref *)plugins.buffer, plugins.elements); > delete_dynamic(&plugins); Regards, Sergei [View Less]

2 1

[Maria-developers] MDEV-6838: Using too big key for internal temp tables
by Vicențiu Ciorbaru 03 Mar '15

03 Mar '15

Hi Sergei and Sergey! I've implemented the fix for MDEV-6838. I'd like a review from both of you regarding the patch. There are a couple of changes made. The main issues are outlined in the commit message. I've changed the way create_key_part_by_field gets called in order to decouple the KEY_PART_INFO creation process from any KEYINFO modifications and also documented what the method does. My other concern is that create_key_part_by_field does not seem to make any sense within the table class … [View More]as it's basically a constructor for KEY_PART_INFO and makes no use of table fields. I suggest we move the code as either a constructor for KEY_PART_INFO, or an init() function. I think that would be better left as a separate patch, but the method itself is very easy to factor out in case you agree with the change. I'm looking forward for your input. Regards, Vicențiu ---------- Forwarded message ---------- From: <vicentiu(a)mariadb.org> Date: 28 February 2015 at 23:58 Subject: 45b6edb: MDEV-6838: Using too big key for internal temp tables To: commits(a)mariadb.org revision-id: 45b6edb158f8101d641f550179ee15df363f686f parent(s): fa87fc733d7855e0e5f9b959ca0bddf772ca57e5 committer: Vicențiu Ciorbaru branch nick: server timestamp: 2015-02-28 23:58:05 +0200 message: MDEV-6838: Using too big key for internal temp tables This bug manifests due to wrong computation and evaluation of keyinfo->key_length. The issues were: * Using table->file->max_key_length() as an absolute value that must not be reached for a key, while it represents the maximum number of bytes possible for a table key. * Incorrectly computing the keyinfo->key_length size during KEY_PART_INFO creation. The metadata information regarding the key such the field length (for strings) was added twice. --- mysql-test/r/table_keyinfo-6838.result | 12 ++++++++++++ mysql-test/t/table_keyinfo-6838.test | 18 ++++++++++++++++++ sql/sql_select.cc | 7 ++++--- sql/structs.h | 1 + sql/table.cc | 34 ++++++++++++++++++++++++++-------- sql/table.h | 2 +- 6 files changed, 62 insertions(+), 12 deletions(-) diff --git a/mysql-test/r/table_keyinfo-6838.result b/mysql-test/r/table_keyinfo-6838.result new file mode 100644 index 0000000..55b0350 --- /dev/null +++ b/mysql-test/r/table_keyinfo-6838.result @@ -0,0 +1,12 @@ +CREATE TABLE t1 (i INT, state VARCHAR(997)) ENGINE=MyISAM; +INSERT INTO t1 VALUES (2,'Louisiana'),(9,'Maine'); +CREATE TABLE t2 (state VARCHAR(997), j INT) ENGINE=MyISAM; +INSERT INTO t2 VALUES ('Louisiana',9),('Alaska',5); +INSERT INTO t2 SELECT t2.* FROM t2 JOIN t2 AS t3 JOIN t2 AS t4 JOIN t2 AS t5 JOIN t2 AS t6; +SET @@max_heap_table_size= 16384; +set @@optimizer_switch='derived_merge=OFF'; +set @@optimizer_switch='extended_keys=ON'; +SELECT * FROM t1 AS t1_1 LEFT JOIN ( t1 AS t1_2 INNER JOIN (SELECT * FROM t2) v2 ON t1_2.i = j ) ON t1_1.state = v2.state LIMIT 1; +i state i state state j +2 Louisiana 9 Maine Louisiana 9 +DROP TABLE t1, t2; diff --git a/mysql-test/t/table_keyinfo-6838.test b/mysql-test/t/table_keyinfo-6838.test new file mode 100644 index 0000000..5f4c9f4 --- /dev/null +++ b/mysql-test/t/table_keyinfo-6838.test @@ -0,0 +1,18 @@ +# Test case for MDEV-6838. +# Due to incorrect key_length computation, this usecase failed with the error +# Using too big key for internal temp table. + +CREATE TABLE t1 (i INT, state VARCHAR(997)) ENGINE=MyISAM; +INSERT INTO t1 VALUES (2,'Louisiana'),(9,'Maine'); + +CREATE TABLE t2 (state VARCHAR(997), j INT) ENGINE=MyISAM; +INSERT INTO t2 VALUES ('Louisiana',9),('Alaska',5); +INSERT INTO t2 SELECT t2.* FROM t2 JOIN t2 AS t3 JOIN t2 AS t4 JOIN t2 AS t5 JOIN t2 AS t6; + +SET @@max_heap_table_size= 16384; +set @@optimizer_switch='derived_merge=OFF'; +set @@optimizer_switch='extended_keys=ON'; + +SELECT * FROM t1 AS t1_1 LEFT JOIN ( t1 AS t1_2 INNER JOIN (SELECT * FROM t2) v2 ON t1_2.i = j ) ON t1_1.state = v2.state LIMIT 1; + +DROP TABLE t1, t2; diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 387fdb3..dabe46e 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -7957,7 +7957,8 @@ static bool create_hj_key_for_table(JOIN *join, JOIN_TAB *join_tab, { Field *field= table->field[keyuse->keypart]; uint fieldnr= keyuse->keypart+1; - table->create_key_part_by_field(keyinfo, key_part_info, field, fieldnr); + table->create_key_part_by_field(key_part_info, field, fieldnr); + keyinfo->key_length += key_part_info->store_length; key_part_info++; } } @@ -15921,7 +15922,7 @@ bool create_internal_tmp_table(TABLE *table, KEY *keyinfo, goto err; bzero(seg, sizeof(*seg) * keyinfo->key_parts); - if (keyinfo->key_length >= table->file->max_key_length() || + if (keyinfo->key_length > table->file->max_key_length() || keyinfo->key_parts > table->file->max_key_parts() || share->uniques) { @@ -16107,7 +16108,7 @@ bool create_internal_tmp_table(TABLE *table, KEY *keyinfo, goto err; bzero(seg, sizeof(*seg) * keyinfo->key_parts); - if (keyinfo->key_length >= table->file->max_key_length() || + if (keyinfo->key_length > table->file->max_key_length() || keyinfo->key_parts > table->file->max_key_parts() || share->uniques) { diff --git a/sql/structs.h b/sql/structs.h index ae71819..9840cec 100644 --- a/sql/structs.h +++ b/sql/structs.h @@ -76,6 +76,7 @@ typedef struct st_key_part_info { /* Info about a key part */ */ uint16 store_length; uint16 key_type; + /* Fieldnr begins counting from 1 */ uint16 fieldnr; /* Fieldnum in UNIREG */ uint16 key_part_flag; /* 0 or HA_REVERSE_SORT */ uint8 type; diff --git a/sql/table.cc b/sql/table.cc index cfa0b6c..7df2ae6 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -5968,18 +5968,32 @@ bool TABLE::alloc_keys(uint key_count) } -void TABLE::create_key_part_by_field(KEY *keyinfo, - KEY_PART_INFO *key_part_info, +/** + @brief + Populate a KEY_PART_INFO structure with the data related to a field entry. + + @param key_part_info The structure to fill. + @param field The field entry that represents the key part. + @param fleldnr The number of the field, count starting from 1. + + TODO: This method does not make use of any table specific fields. It + could be refactored to act as a constructor for KEY_PART_INFO instead. +*/ + +void TABLE::create_key_part_by_field(KEY_PART_INFO *key_part_info, Field *field, uint fieldnr) -{ +{ key_part_info->null_bit= field->null_bit; key_part_info->null_offset= (uint) (field->null_ptr - (uchar*) record[0]); key_part_info->field= field; key_part_info->fieldnr= fieldnr; key_part_info->offset= field->offset(record[0]); - key_part_info->length= (uint16) field->pack_length(); - keyinfo->key_length+= key_part_info->length; + /* + field->key_length() accounts for the raw length of the field, excluding + any metadata such as length of field or the NULL flag. + */ + key_part_info->length= (uint16) field->key_length(); key_part_info->key_part_flag= 0; /* TODO: The below method of computing the key format length of the @@ -5991,17 +6005,20 @@ void TABLE::create_key_part_by_field(KEY *keyinfo, */ key_part_info->store_length= key_part_info->length; + /* + The total store length of the key part is the raw length of the field + + any metadata information, such as its length for strings and/or the null + flag. + */ if (field->real_maybe_null()) { key_part_info->store_length+= HA_KEY_NULL_LENGTH; - keyinfo->key_length+= HA_KEY_NULL_LENGTH; } if (field->type() == MYSQL_TYPE_BLOB || field->type() == MYSQL_TYPE_GEOMETRY || field->real_type() == MYSQL_TYPE_VARCHAR) { key_part_info->store_length+= HA_KEY_BLOB_LENGTH; - keyinfo->key_length+= HA_KEY_BLOB_LENGTH; // ??? key_part_info->key_part_flag|= field->type() == MYSQL_TYPE_BLOB ? HA_BLOB_PART: HA_VAR_LENGTH_PART; } @@ -6124,7 +6141,8 @@ bool TABLE::add_tmp_key(uint key, uint key_parts, if (key_start) (*reg_field)->key_start.set_bit(key); (*reg_field)->part_of_key.set_bit(key); - create_key_part_by_field(keyinfo, key_part_info, *reg_field, fld_idx+1); + create_key_part_by_field(key_part_info, *reg_field, fld_idx+1); + keyinfo->key_length += key_part_info->store_length; (*reg_field)->flags|= PART_KEY_FLAG; key_start= FALSE; key_part_info++; diff --git a/sql/table.h b/sql/table.h index 7a1e380..8324454 100644 --- a/sql/table.h +++ b/sql/table.h @@ -1269,7 +1269,7 @@ struct TABLE bool add_tmp_key(uint key, uint key_parts, uint (*next_field_no) (uchar *), uchar *arg, bool unique); - void create_key_part_by_field(KEY *keyinfo, KEY_PART_INFO *key_part_info, + void create_key_part_by_field(KEY_PART_INFO *key_part_info, Field *field, uint fieldnr); void use_index(int key_to_save); void set_table_map(table_map map_arg, uint tablenr_arg) [View Less]

2 1

[Maria-developers] passwordless mariadb root login with auht_socket in Debian
by Otto Kekäläinen 03 Mar '15

03 Mar '15

Hello Daniel! It seems there are some issues with the passwordless login patch you sent me some weeks ago (https://github.com/ottok/mariadb-10.0/pull/3) Can you help me debug the failing postinst script? I added 'set -x' to the mariadb-server-10.0.postinst script and when it is run during the configure phase it will exit with code 1 and make dpkg abort the installation. Last lines of execution: SET sql_log_bin=0; CREATE TABLE IF NOT EXISTS plugin (name char(64) COLLATE utf8_bin NOT NULL … [View More]

4 15

[Maria-developers] MDEV-7286 TRIGGER: CREATE OR REPLACE, CREATE IF NOT EXISTS
by Alexander Barkov 27 Feb '15

27 Feb '15

Hi Serg, Please review a patch for MDEV-7286. Thanks.

4 5

[Maria-developers] MDEV-7281 EVENT: CREATE OR REPLACE
by Alexander Barkov 26 Feb '15

26 Feb '15

Hi Sergei, Thanks for your suggestions that you gave in the first review. Please review a new patch for MDEV-7281, which addresses your suggestions. >> diff --git a/mysql-test/r/create_drop_event.result b/mysql-test/ /create_drop_event.result > > again, same thing. I don't see a test that the command actually worked. > please use different event definitions in all three create variants. > and add show create event after every create to verify that > create if … [View More]not exists did *not* modify the event, but create or replace > did modify it. > Done. >>> --- a/sql/event_db_repository.cc >>> +++ b/sql/event_db_repository.cc >>> @@ -684,18 +684,28 @@ Event_db_repository::create_event(THD *thd, Event_parse_data *parse_data, >>> DBUG_PRINT("info", ("check existance of an event with the same name")); >>> if (!find_named_event(parse_data->dbname, parse_data->name, table)) >>> { >>> - if (create_if_not) >>> + if (thd->lex->is_create_or_replace()) >>> + { >>> + if ((ret= table->file->ha_delete_row(table->record[0]))) >>> + { >>> + table->file->print_error(ret, MYF(0)); >>> + goto end; >>> + } >>> + } >>> + else if (create_if_not) >>> { >>> *event_already_exists= true; >>> push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE, >>> ER_EVENT_ALREADY_EXISTS, ER(ER_EVENT_ALREADY_EXISTS), >>> parse_data->name.str); >>> ret= 0; >>> + goto end; >>> } >>> else >>> + { >>> my_error(ER_EVENT_ALREADY_EXISTS, MYF(0), parse_data->name.str); >>> - >>> - goto end; >>> + goto end; >>> + } >>> } else >>> *event_already_exists= false; >> >> I wonder whether this works as it should. On create-or-replace you >> delete the old event row and a new one is supposedly inserted. So, the >> table is fine. >> >> But event_already_exists will stay unset, undefined, actually, because >> the caller didn't set it either. If you set it to TRUE, then the caller >> won't update in-memory even list, so your new event won't be enabled. >> If you set it to FALSE, it will try to insert a new event without >> deleting the old one and I don't know what will happen (either both will >> stay active or the insertion fails). >> event_already_exists is now set to "false" on OR REPLACE. The in-memory list is updated in events.cc, in the method Events::create_event(...). This is the new code for this: if (thd->lex->create_info.or_replace() && event_queue) event_queue->drop_event(thd, parse_data->dbname, parse_data->name); >> Do you have a test to verify that after CREATE OR REPLACE EVENT the old >> event no longer exists and the new event does exist and is executed as >> expected? >> Added a test for this. >>> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc >>> index 922f0d7..648fd48 100644 >>> --- a/sql/sql_parse.cc >>> +++ b/sql/sql_parse.cc >>> @@ -3995,9 +3997,8 @@ end_with_restore_list: >>> switch (lex->sql_command) { >>> case SQLCOM_CREATE_EVENT: >>> { >>> - bool if_not_exists= (lex->create_info.options & >>> - HA_LEX_CREATE_IF_NOT_EXISTS); >>> - res= Events::create_event(thd, lex->event_parse_data, if_not_exists); >>> + res= Events::create_event(thd, lex->event_parse_data, >>> + lex->is_create_if_not_exists()); >> >> This doesn't seem to be particularly logical. >> You pass lex->is_create_if_not_exists)() here an argument, >> but you check thd->lex->is_create_or_replace() in the >> Event_db_repository::create_event anyway. I'd suggest not to pass >> lex->is_create_if_not_exists() as an argument, and check both in >> Event_db_repository::create_event(). The "if not exists" flag is not passed to Events::create_event() any more. Thanks. [View Less]

2 1

Re: [Maria-developers] 26f48fa: MDEV-6997: SET STATEMENT last_insert_id FOR ... does not affect the value written to the binlog
by Sergei Golubchik 25 Feb '15

25 Feb '15

Hi, Sanja! On Nov 28, sanja(a)mariadb.com wrote: > revision-id: 26f48fa6c110efc62e17e1edfe6569445387e11d > parent(s): d0d6284cab70af148269d95744f615454cac24bb > committer: Oleksandr Byelkin > branch nick: work-maria-10.1-MDEV-6997-real > timestamp: 2014-11-28 13:52:59 +0100 > message: > > MDEV-6997: SET STATEMENT last_insert_id FOR ... does not affect the value written to the binlog > > Problem was in reinitialization of … [View More]

2 1