- developers - lists.mariadb.org

[Maria-developers] Please review MDEV-10811 Change design from "Item is Type_handler" to "Item has Type_handler"
by Alexander Barkov 03 Nov '16

03 Nov '16

Hello Alexey, Please review a patch for 10.3 for MDEV-10811 Change design from "Item is Type_handler" to "Item has Type_handler Thanks.

1 0

Re: [Maria-developers] [MariaDB/server] Compressed binary log (#247)
by Kristian Nielsen 03 Nov '16

03 Nov '16

GCSAdmin <notifications(a)github.com> writes: > We add new event types to support compress the binlog as follow: > QUERY_COMPRESSED_EVENT, > WRITE_ROWS_COMPRESSED_EVENT_V1, > UPDATE_ROWS_COMPRESSED_EVENT_V1, > DELETE_POWS_COMPRESSED_EVENT_V1, > WRITE_ROWS_COMPRESSED_EVENT, > UPDATE_ROWS_COMPRESSED_EVENT, > DELETE_POWS_COMPRESSED_EVENT > Thanks for the patch! Overall, I'm much impressed with the quality, I see a lot of attention to detail. Below, I have a number of comments and suggestions, but they are all minor for a patch of this complexity. A number of the comments are style fixes or simple changes, and I found it easier to just do the changes to the source for you to review. I hope that is ok. I rebased the series without intermediate merge to simplify review to a single patch. The rebase with my suggested changes on top is here: https://github.com/knielsen/server/commits/GCSAdmin-10.2-binlog-compressed2… Please check the changes I made and let me know if you disagree with anything or I misunderstood something. Changes are mostly: - A bunch of code style (indentation, lines <= 80 chars, clarify comments, and so on). - Change LOG_EVENT_IS_QUERY() etc. from macros to static inline functions. - check_event_type() updated (new code merged into 10.2 recently). - Minor .result file update, also from code merged into 10.2 recently. I also have the following questions/suggestions: 1. I think the code should sanity-check the compressed event header, to check that it does not access outside the event buffer in corrupt events. Eg. if lenlen is 7 and there is less than 7 bytes available in the event. I know that replication code in general is not robust to corrupt events, but it seems best that new code avoids overflowing buffers in case of corrupt event data. 2. There are some places where the compressed event types are not added to switch() or if() statements, mainly related to minimal binlog images, I think: Rows_log_event::read_write_bitmaps_cmp(), Rows_log_event::get_data_size(), Rows_log_event::do_apply_event(), Rows_log_event::write_data_body(). I *think* the reason for that is that the SQL thread never sees the compressed events (they are uncompressed by the IO thread), but I would like your confirmation that my understanding is correct. 3. Did you think about testing that BINLOG statements with compressed events can execute correctly? This happens with mysqlbinlog | mysql, when there are (compressed) row-based events in the binlog. I'm wondering if this could expose compressed events to code that normally runs in the SQL thread and expects to have events uncompressed for it by the IO thread? A few detailed comments on the patch below (most comments are done as changes in the above-linked git branch). Otherwise, after answer to above 3 questions, I think the patch looks good to go into 10.2. - Kristian. ----------------------------------------------------------------------- > +#define BINLOG_COMPRESSED_HEADER_LEN 1 > +#define BINLOG_COMPRESSED_ORIGINAL_LENGTH_MAX_BYTES 4 > +/** > + Compressed Record > + Record Header: 1 Byte > + 0 Bit: Always 1, mean compressed; > + 1-3 Bit: Reversed, compressed algorithm??Always 0, means zlib > + 4-7 Bit: Bytes of "Record Original Length" > + Record Original Length: 1-4 Bytes > + Compressed Buf: I did not understand this. Why "reversed"? It seems to be bits 0-2 that have the bytes of "Record Original Length" and bit 7 that has the '1' bit meaning compression enabled? Maybe this can be clarified. > +int query_event_uncompress(const Format_description_log_event *description_event, bool contain_checksum, > + const char *src, char* buf, ulong buf_size, bool* is_malloc, > + char **dst, ulong *newlen) > +{ > + ulong len = uint4korr(src + EVENT_LEN_OFFSET); > + const char *tmp = src; > + > + DBUG_ASSERT((uchar)src[EVENT_TYPE_OFFSET] == QUERY_COMPRESSED_EVENT); > + > + uint8 common_header_len= description_event->common_header_len; > + uint8 post_header_len= description_event->post_header_len[QUERY_COMPRESSED_EVENT-1]; > + > + tmp += common_header_len; > + > + uint db_len = (uint)tmp[Q_DB_LEN_OFFSET]; > + uint16 status_vars_len= uint2korr(tmp + Q_STATUS_VARS_LEN_OFFSET); > + > + tmp += post_header_len + status_vars_len + db_len + 1; > + > + uint32 un_len = binlog_get_uncompress_len(tmp); > + *newlen = (tmp - src) + un_len; This is one place I would like to see a check on the data in the event. Maybe just that 'len' is larger than 1+(tmp[0]&7). Or maybe simply that len is at least 10, the minimal size for compressed events. > +int row_log_event_uncompress(const Format_description_log_event *description_event, bool contain_checksum, > + const char *src, char* buf, ulong buf_size, bool* is_malloc, > + char **dst, ulong *newlen) > +{ > + uint32 un_len = binlog_get_uncompress_len(tmp); > + *newlen = (tmp - src) + un_len; Another place where I'd like a check against looking outside the event buffer. > +int binlog_buf_uncompress(const char *src, char *dst, uint32 len, uint32 *newlen) > +{ > + if((src[0] & 0x80) == 0) > + { > + return 1; > + } > + > + uint32 lenlen = src[0] & 0x07; > + uLongf buflen = *newlen; > + if(uncompress((Bytef *)dst, &buflen, (const Bytef*)src + 1 + lenlen, len) != Z_OK) Again check on length.

5 12

[Maria-developers] A few character set incompatible changes in 10.2
by Alexander Barkov 02 Nov '16

02 Nov '16

Hello Ian, In 10.2 we made a few changes under terms of these bug reports: MDEV-9874 LOAD XML INFILE does not handle well broken multi-byte characters MDEV-9823 LOAD DATA INFILE silently truncates incomplete byte sequences MDEV-9842 LOAD DATA INFILE does not work well with a TEXT column when using sjis MDEV-9811 LOAD DATA INFILE does not work well with gbk in some cases MDEV-9824 LOAD DATA does not work with multi-byte strings in LINES TERMINATED BY when IGNORE is specified The idea is that the LOAD FILE behavior is now more consistent with INSERT/UPDATE behavior, to store as much data as possible. When some broken byte sequence is found, now LOAD data replaces broken bytes to question marks and keeps loading the value. In older versions LOAD truncated the value on the leftmost broken byte. So suppose I create a file with these bytes: SELECT CONCAT('aaa',0xF09F988E,'bbb') INTO OUTFILE '/tmp/test.txt'; where {{0xF09F988E}} is UTF8MB4 encoding for the character "U+1F60E SMILING FACE WITH SUNGLASSES". and now erroneously load it as a 3-byte utf8: DROP TABLE IF EXISTS t1; CREATE TABLE t1 (a VARCHAR(10) CHARACTER SET utf8); LOAD DATA INFILE '/tmp/test.txt' INTO TABLE t1 CHARACTER SET utf8; SHOW WARNINGS; SELECT * FROM t1; (notice CHARACTER SET utf8 instead of CHARACTER SET utf8mb4 in LOAD). In 5.5 the above script would return: +---------+------+-------------------------------------------------------------------------+ | Level | Code | Message | +---------+------+-------------------------------------------------------------------------+ | Warning | 1366 | Incorrect string value: '\xF0\x9F\x98\x8Ebb...' for column 'a' at row 1 | +---------+------+-------------------------------------------------------------------------+ +------+ | a | +------+ | aaa | +------+ In 10.2 it returns the same warning, but loads more data: +------------+ | a | +------------+ | aaa????bbb | +------------+ Valerii suggests that we document these changes more precisely, mentioning that these are actually incompatible changes! The sad thing is that there is even yet another different behavior in 10.0.26: MDEV-11217 Regression: LOAD DATA INFILE started to fail with an error We're currently thinking what to do with that.

1 0

[Maria-developers] Replication compatibility MySQL/MariaDB
by Daniel Black 02 Nov '16

02 Nov '16

Folks, At the MariaDB Developer meetup there was some discussion about the limits of compatibility especially regarding replication. I've started a section on this page: https://mariadb.com/kb/en/mariadb/mariadb-vs-mysql-compatibility/ If you can identify what can/cannot work in various replication modes between MySQL and MariaDB please document it on this page. If you cannot edit this for some reason reply to the MariaDB Discuss list and eventually the results can be correlated and the page can be updated. As usual other material changes to improving the KB documentation is welcome. Daniel

1 0

[Maria-developers] MDEV-7660 MySQL WL#6671 "Improve scalability by not using thr_lock.c locks for InnoDB tables"
by Sergei Golubchik 29 Oct '16

29 Oct '16

Hi, Sergey, I think it's fine. I had a few questions though, see below: > commit 47aa5f9 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Fri May 6 13:44:07 2016 +0400 > > MDEV-7660 - MySQL WL#6671 "Improve scalability by not using thr_lock.c locks > for InnoDB tables" > > Don't use thr_lock.c locks for InnoDB tables. > > Let HANDLER READ call external_lock() even if SE is not going to be locked by > THR_LOCK. This fixes at least main.implicit_commit failure. > > Removed tests for BUG#45143 and BUG#55930 which cover InnoDB + THR_LOCK. To > operate properly these tests require code flow to go through THR_LOCK debug > sync points, which is not the case after this patch. These tests are removed > by WL#6671 as well. An alternative is to port them to different storage engine. > > For the very same reason partition_debug_sync test was adjusted to use MyISAM. > > diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc > index e8ade81..76107ae 100644 > --- a/sql/sql_handler.cc > +++ b/sql/sql_handler.cc > @@ -752,11 +752,12 @@ bool mysql_ha_read(THD *thd, TABLE_LIST *tables, > tables->table= table; // This is used by fix_fields > table->pos_in_table_list= tables; > > - if (handler->lock->lock_count > 0) > + if (handler->lock->table_count > 0) > { > int lock_error; > > - handler->lock->locks[0]->type= handler->lock->locks[0]->org_type; > + if (handler->lock->lock_count > 0) > + handler->lock->locks[0]->type= handler->lock->locks[0]->org_type; I don't understand this code in mysql_ha_read() at all :( even before your changes > /* save open_tables state */ > TABLE* backup_open_tables= thd->open_tables; > diff --git a/storage/xtradb/handler/ha_innodb.h b/storage/xtradb/handler/ha_innodb.h > index 2027a59..efb8120 100644 > --- a/storage/xtradb/handler/ha_innodb.h > +++ b/storage/xtradb/handler/ha_innodb.h > @@ -218,6 +218,7 @@ class ha_innobase: public handler > bool can_switch_engines(); > uint referenced_by_foreign_key(); > void free_foreign_key_create_info(char* str); > + uint lock_count(void) const; > THR_LOCK_DATA **store_lock(THD *thd, THR_LOCK_DATA **to, > enum thr_lock_type lock_type); > void init_table_handle_for_HANDLER(); > > commit 5645626 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Tue May 24 12:25:56 2016 +0400 > > MDEV-7660 - MySQL WL#6671 "Improve scalability by not using thr_lock.c locks > for InnoDB tables" > > - InnoDB now acquires shared lock for HANDLER ... READ Why? > - LOCK TABLES now disables autocommit implicitely > - UNLOCK TABLES now re-enables autocommit implicitely if it was disabled by > LOCK TABLES > - adjusted test cases to this new behavior > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 3091bd6..7d39484 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -2824,6 +2824,8 @@ Locked_tables_list::unlock_locked_tables(THD *thd) > request for metadata locks and TABLE_LIST elements. > */ > reset(); > + if (thd->variables.option_bits & OPTION_AUTOCOMMIT) > + thd->variables.option_bits&= ~(OPTION_NOT_AUTOCOMMIT); 1. Was it possible - before your change - for OPTION_AUTOCOMMIT and OPTION_NOT_AUTOCOMMIT to be out of sync? 2. What if someone changes @@autocommit under LOCK TABLES? Do you have a test for that? 3. Do you need to set SERVER_STATUS_AUTOCOMMIT here? > } > > > diff --git a/mysql-test/t/innodb_mysql_lock.test b/mysql-test/t/innodb_mysql_lock.test > index cb57c09..85ba418 100644 > --- a/mysql-test/t/innodb_mysql_lock.test > +++ b/mysql-test/t/innodb_mysql_lock.test > @@ -150,14 +150,16 @@ let $wait_condition= > --source include/wait_condition.inc > LOCK TABLES t1 READ; > SELECT release_lock('bug42147_lock'); > +let $wait_condition= > + SELECT COUNT(*) > 0 FROM information_schema.processlist > + WHERE state = 'executing' > + AND info = 'INSERT INTO t1 SELECT get_lock(\'bug42147_lock\', 60)'; > +--source include/wait_condition.inc > +UNLOCK TABLES; I don't understand the original test case. But after your changes it actually makes sense :) > > connection default; > --reap > > -connection con2; > -UNLOCK TABLES; > - > -connection default; > disconnect con2; > DROP TABLE t1; > > diff --git a/mysql-test/r/partition_explicit_prune.result b/mysql-test/r/partition_explicit_prune.result > index 765803d..7b9c53d 100644 > --- a/mysql-test/r/partition_explicit_prune.result > +++ b/mysql-test/r/partition_explicit_prune.result > @@ -281,7 +281,7 @@ UNLOCK TABLES; > SELECT * FROM INFORMATION_SCHEMA.SESSION_STATUS > WHERE VARIABLE_NAME LIKE 'HANDLER_%' AND VARIABLE_VALUE > 0; > VARIABLE_NAME VARIABLE_VALUE > -HANDLER_COMMIT 2 > +HANDLER_COMMIT 3 why is that? > HANDLER_READ_RND_NEXT 52 > HANDLER_TMP_WRITE 72 > HANDLER_WRITE 2 > diff --git a/mysql-test/suite/handler/handler.inc b/mysql-test/suite/handler/handler.inc > index b1e881f..8cad6a5 100644 > --- a/mysql-test/suite/handler/handler.inc > +++ b/mysql-test/suite/handler/handler.inc > @@ -1091,6 +1091,12 @@ connection default; > --reap > drop table t2; > > +# This test expects "handler t1 read a next" to get blocked on table level > +# lock so that further "drop table t1" can break the lock and close handler. > +# This notification mechanism doesn't work with InnoDB since it bypasses > +# table level locks. what happens for InnoDB then? I'd expect "handler t1 read a next" to get blocked inside the InnoDB. what does then "drop table t1" do? > +if ($engine_type != 'InnoDB') > +{ > --echo # > --echo # Bug #46224 HANDLER statements within a transaction might > --echo # lead to deadlocks Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 8

[Maria-developers] Patch for unaligned word access in CONNECT storage engine
by Kristian Nielsen 28 Oct '16

28 Oct '16

Who should be contacted about issues in the CONNECT storage engine? The attached patch is from Debian Bug#838914 https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=838914 Apparently the code does direct unaligned accesses of word data. This works fine on the x86 architecture, but on some other architectures (like MIPS), it causes a bus error. In other places in the server code, the similar issue is handled correctly with uint4korr() and similar macros (though these also deal with byte order). I think the patch (or something similar) is good and should be upstreamed. But I am not sure how the CONNECT storage engine is maintained - should this go directly into MariaDB? If there is an upstream maintained CONNECT storage engine, probably it should preferably go there first? - Kristian.g

2 3

[Maria-developers] MariaDB connectors - dynamic libraries for Fedora (and all OSs that forks from it)
by Michal Schorm 27 Oct '16

27 Oct '16

Hello all, I am new guy, who tries to add MariaDB connectors to Fedora. (And in the future, probabbly most of the MariaDB stuff will rest on me in Fedora) So far I encountered one great issue. In Fedora, we only use dynamic libraries (which makes, for example, maintaining different packages with same library much much easier). But you guys, you don't seem like you are all for it. Right now, I stopped at mariadb-connector-odbc package. There is an issue, which makes it unusable when build as a dynamic library. In that moment, it uses stuff from library of connector C, but connector C library seems to keep its secrets to itself - concretely wrapper functions. Here are impossible solutions: * use static libraries in Fedora (can't - dynamic libys are one of the fundamental things in Fedora, same priority as use only FOSS) * link odbc connector towards mariadb-libs (can't - connectors are meant as replacement for mariadb-libs) Here are some solutions that are at least not denied by deafult: * start to support building all of your components as dynamic libraries (Best solution for us (for Fedora), because the closer to upstream code we are, the better. Needless to say, over 40 OSs forks somehow from Fedora) * export everything from C connector library (I assume, that is not what you want to do) * export just what is used by other components - mainly wrappers - some exaples for ODBC connector: my_no_flags_free, my_malloc, my_charset_utf8_general_ci, my_strndup, my_realloc, my_strdup, my_snprintf (Compromise for both sides, but can rise more issues later) Are there any reasons to not support dynamic libraries? What could be the best solution from your point of view? Cheers, Michal -- Michal Schorm Core Services - Databases Team mail: mschorm(a)redhat.com Brno-IRC: mschorm

2 1

Re: [Maria-developers] [Commits] b976f9f: MDEV-11126: Crash while altering persistent virtual column
by Sergei Golubchik 26 Oct '16

26 Oct '16

Hi, Jan! Ok to push! On Oct 25, Jan Lindström wrote: > revision-id: b976f9f9ed8ce8d023b80f4d09c5ad5f74aae1fb (mariadb-10.0.27-14-gb976f9f) > parent(s): 3321f1adc74b54e7534000c06eeca166730ccc4a > author: Jan Lindström > committer: Jan Lindström > timestamp: 2016-10-25 15:08:15 +0300 > message: > > MDEV-11126: Crash while altering persistent virtual column > > Problem was that if old virtual column is computed and stored there > was no check if new column is really virtual column. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 65b0617: MDEV-10846 Running mysqldump backup twice returns error: Table
by Sergei Golubchik 25 Oct '16

25 Oct '16

Hi, Alexey! Ok to push, thanks! Just fix the test first (see below) On Oct 22, Alexey Botchkov wrote: > revision-id: 65b0617836390bb76104cb37094e16bafa6da8b2 (mariadb-10.0.27-12-g65b0617) > parent(s): fb38d2642011c574cc9103ae1a1f9dd77f7f027e > committer: Alexey Botchkov > timestamp: 2016-10-22 12:08:15 +0400 > message: > > MDEV-10846 Running mysqldump backup twice returns error: Table > 'mysql.proc' doesn't exist. > > The mysql_rm_db() doesn't seem to expect the 'mysql' database > to be deleted. Checks for that added. > Also fixed the bug MDEV-11105 Table named 'db' has weird side effect. > The db.opt file now removed separately. > > --- > mysql-test/r/drop.result | 6 ++++++ > mysql-test/t/drop.test | 9 +++++++++ > sql/sql_db.cc | 26 +++++++++++++++++++++----- > 3 files changed, 36 insertions(+), 5 deletions(-) > > diff --git a/mysql-test/r/drop.result b/mysql-test/r/drop.result > index c23ffbe3..ee1758f 100644 > --- a/mysql-test/r/drop.result > +++ b/mysql-test/r/drop.result > @@ -209,3 +209,9 @@ INSERT INTO table1 VALUES (1); > ERROR 42S02: Unknown table 't.notable' > DROP TABLE table1,table2; > # End BUG#34750 > +# > +# MDEV-11105 Table named 'db' has weird side effect. > +# > +CREATE DATABASE mysqltest; > +CREATE TABLE mysqltest.t1(id INT); huh? the table name is supposed to be 'db' here :) > +DROP DATABASE mysqltest; > diff --git a/sql/sql_db.cc b/sql/sql_db.cc > index e89c3d9..0a3ff64 100644 > --- a/sql/sql_db.cc > +++ b/sql/sql_db.cc > @@ -784,7 +784,7 @@ bool mysql_alter_db(THD *thd, const char *db, HA_CREATE_INFO *create_info) > bool mysql_rm_db(THD *thd,char *db,bool if_exists, bool silent) > { > ulong deleted_tables= 0; > - bool error= true; > + bool error= true, rm_mysql_schema; > char path[FN_REFLEN + 16]; > MY_DIR *dirp; > uint length; > @@ -809,6 +809,18 @@ bool mysql_rm_db(THD *thd,char *db,bool if_exists, bool silent) > length= build_table_filename(path, sizeof(path) - 1, db, "", "", 0); > strmov(path+length, MY_DB_OPT_FILE); // Append db option file name > del_dbopt(path); // Remove dboption hash entry > + /* > + Now remove the db.opt file. > + The 'find_db_tables_and_rm_known_files' doesn't remove this file > + if there exists a table with the name 'db', so let's just do it > + separately. We know this file exists and needs to be deleted anyway. ah, thanks, I see now. I wondered why a table 'db' would matter. > + */ > + if (my_delete_with_symlink(path, MYF(0)) && my_errno != ENOENT) > + { > + my_error(EE_DELETE, MYF(0), path, my_errno); > + DBUG_RETURN(true); here I don't think this error is quite correct. nowhere else mysql_rm_db() seems to fail with EE_DELETE. It always says "Error dropping database (can't rmdir './exp_db', errno: 39 "Directory not empty")" May be you should add this EE_DELETE as a warning? > + } > + > path[length]= '\0'; // Remove file name Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] new PLUGIN API.
by Alexey Botchkov 25 Oct '16

25 Oct '16

Hi, Sergei. I'd like to draw your attention to this old issue: https://jira.mariadb.org/browse/MDEV-7389 The idea was to make a bigger thing - to modify the plugin API so it is easier to use and let user to do more. Particularly to notify warnings to the audit plugins for this 7389 task. That was done with this task: https://jira.mariadb.org/browse/MDEV-5313 Just to refresh our memory: I proposed to get rid off the API versions and version-dependent memory structures that are used to transfer data to and from the plugin. All we need to do is adding new 'audit_plugin_service'. Which is just the normal service that offer methods to the auditing plugin to send commands to the server and get the server data. You can look at the patch http://lists.askmonty.org/pipermail/commits/2016-February/009025.html So, Serg, do you have anything to say on that subject? Best regards. HF

3 4