August 2017 - developers - lists.mariadb.org

Re: [Maria-developers] MDEV-13439: Database permissions are not enough to run a subquery with GROUP BY within a view
by Sergei Golubchik 09 Aug '17

09 Aug '17

Hi, Oleksandr! On Aug 07, Oleksandr Byelkin wrote: > revision-id: cf66c30ce592ad88d447568272bdced2046f2906 (mariadb-10.2.7-20-gcf66c30ce59) > parent(s): bcc10a5a447805ce64aa13ee6a037c1618219616 > committer: Oleksandr Byelkin > timestamp: 2017-08-07 13:42:35 +0200 > message: > > MDEV-13439: Database permissions are not enough to run a subquery with GROUP BY within a view > > The bug is result adding ability to have derived tables inside views. > Fixed checks should be a switch between view/derived or select derived and information schema. > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 77f502d3a7b..9bffcbe6a9e 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -7594,8 +7594,11 @@ bool check_grant(THD *thd, ulong want_access, TABLE_LIST *tables, > /* > It is subquery in the FROM clause. VIEW set t_ref->derived after > table opening, but this function always called before table opening. > + > + NOTE: is_derived() can't be used here bacause subquery in this case typo "bacause" > + the FROM clase (derived tables) can be not be marked yet. > */ > - if (!t_ref->referencing_view) > + if (t_ref->is_anonymous_derived_table() || t_ref->schema_table) > { > /* > If it's a temporary table created for a subquery in the FROM > diff --git a/sql/sql_derived.cc b/sql/sql_derived.cc > index 649f745fdc4..8f8d051a976 100644 > --- a/sql/sql_derived.cc > +++ b/sql/sql_derived.cc > @@ -822,10 +822,12 @@ bool mysql_derived_prepare(THD *thd, LEX *lex, TABLE_LIST *derived) > table->derived_select_number= first_select->select_number; > table->s->tmp_table= INTERNAL_TMP_TABLE; > #ifndef NO_EMBEDDED_ACCESS_CHECKS > - if (derived->referencing_view) > + if (derived->is_view()) > table->grant= derived->grant; > else > { > + DBUG_ASSERT(derived->is_derived()); > + DBUG_ASSERT(derived->is_anonymous_derived_table()); Hmm, if derived->is_derived() is always true here, why there's an if() below? If it's redundant - please, remove it. > table->grant.privilege= SELECT_ACL; > if (derived->is_derived()) > derived->grant.privilege= SELECT_ACL; Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] commit_checkpoint_request() vs. thd_get_durability_property() (in relation to MDEV-11937)
by Kristian Nielsen 08 Aug '17

08 Aug '17

Monty asked me to fix MDEV-11937. This particular one is a performance regression in InnoDB commit. But there is a wider problem that I thought I should explain, so it can be perhaps avoided in the future. MariaDB and MySQL use different mechanisms for storage engines to avoid having to fsync during commit when binlog is enabled. In MariaDB, storage engines implement the commit_checkpoint_request() handlerton method. In MySQL, storage engines call thd_get_durability_property() to check if they can avoid fsync. The problem is that somehow the thd_get_durability_property() function was introduced into MariaDB code, but it is completely non-functional. So now there is code in InnoDB, TokuDB and RocksDB that calls this function and does not work correctly. This lead to performance regression due to extra fsync() calls. This seems to me a serious problem. Now new code can be merged and compile fine, where in reality it is wrong. There really should not be two separate and different mechanisms for the same thing, and certainly not with one of them non-functional. The "expected" approach would be to remove thd_get_durability_property() and update storage engines to use the corresponding MariaDB APIs (commit_ordered() and commit_checkpoint_request()). This should not be hard. A simple commit_checkpoint_request() implementation can just fsync all transactions immediately (similar to what MySQL does). A more detailed implementation can avoid any extra fsyncs, and just asynchroneously notify the upper layer with commit_checkpoint_notify_ha() later when such fsync happens normally (this is what InnoDB does). See comments in handler.h for details. Or was the intention to eventually replace the whole MariaDB binlog group commit implementation with the MySQL one, to make MariaDB less divergent? This would require a number of changes to MariaDB binlog and replication. The binlog recovery code should be replaced (MySQL does not have the ability to recover from more than one binlog). The binlog group commit code must be replaced as well, and the commit_ordered() mechanism removed. I think this would also require a re-design of the MariaDB in-order parallel replication. MySQL has some optional mechanism for in-order, but my understanding is that it is not sufficient to do optimistic parallel replication. I am not intimately familiar with the MySQL code though. I hope this helps. The wider problem behind MDEV-11937 is one of policy more than one of code bugs, so not too much else I can do to address it. ----------------------------------------------------------------------- Incidentally, I noticed some code in InnoDB trx0trx.cc: /* We set the HA_IGNORE_DURABILITY during prepare phase of binlog group commit to not flush redo log for every transaction here. So that we can flush prepared records of transactions to redo log in a group right before writing them to binary log during flush stage of binlog group commit. */ So the idea is to do group prepare with the same group of transactions that will later group commit to the binlog. In MariaDB, this concept does not exist. Storage engine prepares are allowed to run in parallel and in any order compared to binlog commit. So the InnoDB group prepare can include more transactions than participate in the binlog commit (but also less, of course). IIRC, the important thing is to ensure that all transactions are durably prepared in storage engines before being written to the binlog. In MariaDB, there is MYSQL_LOG_BIN::group_commit_queue that holds the list of transactions to be group committed to the binlog. A similar mechanism in MariaDB might use the group_commit_queue as the set of transactions to send to storage engines for group prepare. But if we wait for a prepare fsync() after building this list, some transactions that could prepare during this fsync may be unnecesarily delayed to the next binlog commit. The MySQL 5.7 code grabs the list of transactions to binlog group commit, and then flushes the log in _all_ storage engines, unconditionally. That just seems horrybly wrong, but I can't see any other way to read the code (from MYSQL_BIN_LOG::process_flush_stage_queue()): ha_flush_logs(NULL, true); It even does so while holding LOCK_log :-( I guess the MySQL idea is that there is only one storage engine anyway, InnoDB. - Kristian.

3 3

Re: [Maria-developers] 4c26b71c77d: MDEV-8960: Can't refer the same column twice in one ALTER TABLE
by Sergei Golubchik 04 Aug '17

04 Aug '17

Hi, Jan! A question and a suggestion, see below On Jul 27, jan wrote: > revision-id: 4c26b71c77d58fbeb3192e2142ef66efe5f122dd (mariadb-10.0.31-43-g4c26b71c77d) > parent(s): a8c1817846eb9f3fd8738e80db87cff642b9a78b > author: Jan Lindström > committer: Jan Lindström > timestamp: 2017-07-27 13:17:13 +0300 > message: > > MDEV-8960: Can't refer the same column twice in one ALTER TABLE > > Problem was that if column was created in alter table when > it was refered again it was not tried to find from list > of current columns. > > mysql_prepare_alter_table: > There is two cases > (1) If alter table adds a new column and then later alter > changes the field definition, there was no check from > list of new columns, instead an incorrect error was given. > (2) If alter table adds a new column and then later alter > changes the default, there was no check from list of > new columns, instead an incorrect error was given. > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 2eff8fd5e2f..3ba3ec6847e 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -7503,9 +7503,25 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, > { > if (def->change && ! def->field) > { > - my_error(ER_BAD_FIELD_ERROR, MYF(0), def->change, > - table->s->table_name.str); > - goto err; > + /* > + Check if there is modify for newly added field. > + */ > + Create_field *find; > + find_it.rewind(); > + while((find=find_it++)) > + { > + if (!my_strcasecmp(system_charset_info,find->field_name, def->field_name)) > + break; > + } > + > + if (find && !find->field) Can it be here that find->field != NULL? I don't see how. But if yes, the ER_BAD_FIELD_ERROR looks rather weird in that case. If not - simple if (find) would be less confusing. > + find_it.remove(); > + else > + { > + my_error(ER_BAD_FIELD_ERROR, MYF(0), def->change, > + table->s->table_name.str); > + goto err; > + } > } > /* > Check that the DATE/DATETIME not null field we are going to add is > @@ -7571,6 +7587,29 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, > find_it.after(def); // Put column after this > } > } > + /* > + Check if there is alter for newly added field. > + */ > + alter_it.rewind(); > + Alter_column *alter; > + while ((alter=alter_it++)) > + { > + if (!my_strcasecmp(system_charset_info,def->field_name, alter->name)) > + break; > + } > + if (alter) > + { > + if (def->sql_type == MYSQL_TYPE_BLOB) > + { > + my_error(ER_BLOB_CANT_HAVE_DEFAULT, MYF(0), def->change); > + goto err; > + } > + if ((def->def=alter->def)) // Use new default > + def->flags&= ~NO_DEFAULT_VALUE_FLAG; > + else > + def->flags|= NO_DEFAULT_VALUE_FLAG; > + alter_it.remove(); > + } Please, move this into a function. Don't just copy 20 lines around. For example if (apply_alter_column(alter_it, def, field->field_name)) goto err; > } > if (alter_info->alter_list.elements) > { Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] Review 2 of compressed columns
by Michael Widenius 04 Aug '17

04 Aug '17

Hi! Here is a review for the patches marked with fixup! MDEV-11371 - column compression > commit f1a3cd40ecc4c144ac32d43ab5c1e88c8e4fae7e > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Mon Jul 31 16:07:39 2017 +0400 > > fixup! MDEV-11371 - column compression > > In worst case we have to store data length + 1 for compressed data, > we should increase field length by 1 internally so that data is not > truncated. > > diff --git a/mysql-test/t/column_compression.test b/mysql-test/t/column_compression.test > index 256e96c..d3f8481 100644 > --- a/mysql-test/t/column_compression.test > +++ b/mysql-test/t/column_compression.test > @@ -54,8 +54,8 @@ SHOW CREATE TABLE t1; > --cat_file $MYSQLD_DATADIR/test/t1.CSV > DROP TABLE t1; > > ---echo # Test fields with length equal to data length > -CREATE TABLE t1(a VARCHAR(10) COMPRESSED); > +--echo # Test fields that don't fit data > +CREATE TABLE t1(a VARCHAR(9) COMPRESSED); > --error ER_DATA_TOO_LONG > INSERT INTO t1 VALUES(REPEAT('a', 10)); > INSERT INTO t1 VALUES(REPEAT(' ', 10)); Please also insert a correct row, and not only edge cases. (You may do it already, but I can't see that in the diff) > @@ -70,3 +70,12 @@ INSERT INTO t1 VALUES(REPEAT(' ', 255)); > SET column_compression_threshold=DEFAULT; > SELECT a, LENGTH(a) FROM t1; > DROP TABLE t1; > + > +--echo # Corner case: VARCHAR(255) COMPRESSED must have 2 bytes pack length > +CREATE TABLE t1(a VARCHAR(255) COMPRESSED); > +SHOW CREATE TABLE t1; > +SET column_compression_threshold=300; > +INSERT INTO t1 VALUES(REPEAT('a', 255)); > +SET column_compression_threshold=DEFAULT; > +SELECT a, LENGTH(a) FROM t1; > +DROP TABLE t1; Please insert also a row without column_compression_threshold, just to cover all cases in the test case. > diff --git a/sql/field.cc b/sql/field.cc > index 216fb95..e1dc193 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -7555,7 +7555,8 @@ void Field_varstring::sql_type(String &res) const > length= cs->cset->snprintf(cs,(char*) res.ptr(), > res.alloced_length(), "%s(%d)", > (has_charset() ? "varchar" : "varbinary"), > - (int) field_length / charset()->mbmaxlen); > + (int) field_length / charset()->mbmaxlen - > + MY_TEST(compression_method())); > res.length(length); > if ((thd->variables.sql_mode & (MODE_MYSQL323 | MODE_MYSQL40)) && > has_charset() && (charset()->state & MY_CS_BINSORT)) > @@ -10015,6 +10016,8 @@ void Column_definition::create_length_to_internal_length(void) > case MYSQL_TYPE_STRING: > case MYSQL_TYPE_VARCHAR: > length*= charset->mbmaxlen; > + if (sql_type == MYSQL_TYPE_VARCHAR && compression_method()) > + length++; Add a comment: /* Compressed columns needs one extra byte to store the compression method. This byte is invisible to the end user, but not for the storage engine. */ > key_length= length; > pack_length= calc_pack_length(sql_type, length); > break; <cut> > commit 274ab73a89573554d07a6de1deb846ad47eb1004 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Thu Jul 27 13:06:33 2017 +0400 > > fixup! MDEV-11371 - column compression > > Fixed BLOB COMPRESSED -> BLOB COMPRESSED replication. > > The problem was that Field_blob::unpack gets compressed data and calls > Field_blob_compressed::store(), which compressed it again. Fixed by calling > Field_blob::set_ptr() instead of ::store(). > > Note that now no data copy is performed and blob is pointing to data allocated > by replication routines. It used to work this way before 06fb8c2d10, but still > should be reviewed carefully to make sure no copy is actually needed. > patch ok. > commit 93e00970100503c3f365063d9261119f096bee48 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Thu Jul 27 11:44:20 2017 +0400 > > Get rid of Field::do_save_field_metadata() > > It doesn't serve any purpose, but generates extra virtual function call. > ok > commit e1247d4991bb4ebd3f98788c7bfe31a34faf7b69 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Thu Jul 27 10:58:47 2017 +0400 > > fixup! MDEV-11371 - column compression > > This is an addition to "Fixed compressed columns to work with partitioning". > > Mroonga calls key_copy() on write/delete, when column is not in read_set. > Adjust read_set for the duration of val_str(). > > diff --git a/sql/field.cc b/sql/field.cc > index ec82984..ab96ee9 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -7659,8 +7659,12 @@ uint Field_varstring::get_key_image(uchar *buff, uint length, > { > String val; > uint local_char_length; > + my_bitmap_map *old_map; > > + old_map= dbug_tmp_use_all_columns(table, table->read_set); > val_str(&val, &val); > + dbug_tmp_restore_column_map(table->read_set, old_map); > + Shouldn't this be in mronga code and not in MariaDB? Looking at the original code, we didn't check for read_set before either for get_key_image, so I assume this is ok. > local_char_length= val.charpos(length / field_charset->mbmaxlen); > if (local_char_length < val.length()) > val.length(local_char_length); > commit 94040156d8007663beb6785ca653d41b26758301 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Wed Jul 26 17:28:57 2017 +0400 > > fixup! MDEV-11371 - column compression > > Get rid of hardcoded "8" (zlib compression method id) by moving towards > support for multiple compression methods. > commit 6c8b1e563c7456eb0b8ca11a621333237de37ec4 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Tue Jul 25 13:27:07 2017 +0400 > > fixup! MDEV-11371 - column compression > > Fixed compressed columns to work with partitioning. > > Reviewer note: this patch reuses get_key_image()/set_key_image() of > Field_varstring to avoid code duplication. Cons: this adds virtual function > calls val_str() and store(). > ok > commit 9c85ad8d105e74baae0b6aba0db01b4d6aac7ba6 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Thu Jul 20 16:57:46 2017 +0400 > > fixup! MDEV-11371 - column compression ok. The only thing left is now to fix replication to handle compressed columns. We also have agreed to handle the case where the master and slave have different COMPRESSED column attributes for VARCHAR and BLOB. Regards, Monty

1 0

[Maria-developers] Debian 9 support for MaxScale
by Martin Toth 03 Aug '17

03 Aug '17

Hi all, I would like to ask if there is some release date for fixing https://jira.mariadb.org/browse/MXS-1292 . Debian 9 should be included as a build target ASAP. I am installing only D9 systems on new servers and I am unable to use builds for MaxScale, so its unusable in D9. Thanks. Martin

3 7

Re: [Maria-developers] 1 Transaction and several threads
by Jan Lindström 03 Aug '17

03 Aug '17

Hi, If we talk about bulk load I would write directly .ibd file format pages, now as InnoDB has always some order between pages doing this in parallel has challenges, we can read and parse input, sort it, reformat to InnoDB pages, write these pages (maybe you could do this also parallel). To speed this up, I would remove secondary keys that are not needed from table and rebuild them after bulk load. R: Jan On Thu, Aug 3, 2017 at 12:09 PM, Oleksandr Byelkin <sanja(a)mariadb.com> wrote: > Hi! > > I am thinking about efficient inserting in a table using several threads > (LOAD), is it possible (and how difficult if it does) to make inserts with > help of several threads abut inside one InnoDB transaction (maybe one XA > transaction)? >

3 2

[Maria-developers] Git jira integration plugin
by Daniel Black 03 Aug '17

03 Aug '17

For those that haven't seen it. https://www.npmjs.com/package/jira-git

1 0

[Maria-developers] Patch for MDEV-13417 - UPDATE produces wrong values if an updated column is later used as an update source
by jerome brauge 02 Aug '17

02 Aug '17

Hello, Here is a patch for MDEV-13417 (on branch 10.2-ext). Can anyone review it ? Best regards, Jérôme.

1 0