- developers - lists.mariadb.org

Re: [Maria-developers] [Commits] d1de640: MDEV-10216: Assertion `strcmp(share->unique_file_name, filename) ||
by Sergey Vojtovich 01 Jul '16

01 Jul '16

Hi Nirbhay, This solution looks better than MyISAM/Aria based one. Still a few doubts inline. I still suggest someone should do second review. On Tue, Jun 21, 2016 at 01:47:15PM -0400, Nirbhay Choubey wrote: > revision-id: d1de6402c8734e9a1929c2384c0cfc07c2ec48c0 (mariadb-10.2.0-83-gd1de640) > parent(s): b2ae32aafdd2787ad456f38833f630182ded25e8 > author: Nirbhay Choubey > committer: Nirbhay Choubey > timestamp: 2016-06-21 13:47:12 -0400 > message: > > MDEV-10216: Assertion `strcmp(share->unique_file_name,filename) || ...skip... > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 0be329e..07d4b42 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -8151,12 +8151,15 @@ static bool fk_prepare_copy_alter_table(THD *thd, TABLE *table, Looks like git confused function names here and for most of other hunks of this patch. I vaguely remember this bug was affecting really old git versions like pre-2.x. > > if (keys_onoff != Alter_info::LEAVE_AS_IS) > { > - if (wait_while_table_is_used(thd, table, extra_func)) > - DBUG_RETURN(true); > + if (!table->s->tmp_table) > + { > + if (wait_while_table_is_used(thd, table, extra_func)) > + DBUG_RETURN(true); > > - // It's now safe to take the table level lock. > - if (lock_tables(thd, table_list, alter_ctx->tables_opened, 0)) > - DBUG_RETURN(true); > + // It's now safe to take the table level lock. > + if (lock_tables(thd, table_list, alter_ctx->tables_opened, 0)) > + DBUG_RETURN(true); > + } > > error= alter_table_manage_keys(table, > table->file->indexes_are_disabled(), Are you sure lock_tables() is not necessary for temporary tables here? It would be nice to make sure this code is covered by a test case. > @@ -8166,43 +8169,57 @@ static bool fk_prepare_copy_alter_table(THD *thd, TABLE *table, > if (!error && alter_ctx->is_table_renamed()) > { > THD_STAGE_INFO(thd, stage_rename); > - handlerton *old_db_type= table->s->db_type(); > - /* > - Then do a 'simple' rename of the table. First we need to close all > - instances of 'source' table. > - Note that if wait_while_table_is_used() returns error here (i.e. if > - this thread was killed) then it must be that previous step of > - simple rename did nothing and therefore we can safely return > - without additional clean-up. > - */ > - if (wait_while_table_is_used(thd, table, extra_func)) > - DBUG_RETURN(true); > - close_all_tables_for_name(thd, table->s, HA_EXTRA_PREPARE_FOR_RENAME, NULL); > - > LEX_STRING old_db_name= { alter_ctx->db, strlen(alter_ctx->db) }; > LEX_STRING old_table_name= > { alter_ctx->table_name, strlen(alter_ctx->table_name) }; > LEX_STRING new_db_name= { alter_ctx->new_db, strlen(alter_ctx->new_db) }; > LEX_STRING new_table_name= > { alter_ctx->new_alias, strlen(alter_ctx->new_alias) }; > - (void) rename_table_in_stat_tables(thd, &old_db_name, &old_table_name, > - &new_db_name, &new_table_name); > > - if (mysql_rename_table(old_db_type, alter_ctx->db, alter_ctx->table_name, > - alter_ctx->new_db, alter_ctx->new_alias, 0)) > - error= -1; > - else if (Table_triggers_list::change_table_name(thd, > - alter_ctx->db, > - alter_ctx->alias, > - alter_ctx->table_name, > - alter_ctx->new_db, > - alter_ctx->new_alias)) > - { > - (void) mysql_rename_table(old_db_type, > - alter_ctx->new_db, alter_ctx->new_alias, > - alter_ctx->db, alter_ctx->table_name, > - NO_FK_CHECKS); > - error= -1; > + if (!table->s->tmp_table) > + { > + handlerton *old_db_type= table->s->db_type(); > + /* > + Then do a 'simple' rename of the table. First we need to close all > + instances of 'source' table. > + Note that if wait_while_table_is_used() returns error here (i.e. if > + this thread was killed) then it must be that previous step of > + simple rename did nothing and therefore we can safely return > + without additional clean-up. > + */ > + if (wait_while_table_is_used(thd, table, extra_func)) > + DBUG_RETURN(true); > + close_all_tables_for_name(thd, table->s, HA_EXTRA_PREPARE_FOR_RENAME, > + NULL); > + > + (void) rename_table_in_stat_tables(thd, &old_db_name, &old_table_name, > + &new_db_name, &new_table_name); > + > + if (mysql_rename_table(old_db_type, alter_ctx->db, alter_ctx->table_name, > + alter_ctx->new_db, alter_ctx->new_alias, 0)) > + error= -1; > + else if (Table_triggers_list::change_table_name(thd, > + alter_ctx->db, > + alter_ctx->alias, > + alter_ctx->table_name, > + alter_ctx->new_db, > + alter_ctx->new_alias)) > + { > + (void) mysql_rename_table(old_db_type, > + alter_ctx->new_db, alter_ctx->new_alias, > + alter_ctx->db, alter_ctx->table_name, > + NO_FK_CHECKS); > + error= -1; > + } > + } > + else > + { > + if ((thd->rename_temporary_table(table, alter_ctx->new_db, > + alter_ctx->new_alias))) > + { > + /* Allocation error, no need to rename it back to the original name. */ > + error= -1; > + } > } > } > A few lines below we adjust MDL locks, is it safe for temporary tables? Previously we didn't binlog temporary table creation if binlog format was row. Now we binlog it independently of binlog format. Previously we did "new_table->s->table_creation_was_logged= table->s->table_creation_was_logged;". Now we don't. > @@ -9623,7 +9639,8 @@ bool mysql_trans_commit_alter_copy_data(THD *thd) > to->file->ha_release_auto_increment(); > if (to->file->ha_external_lock(thd,F_UNLCK)) > error=1; > - if (error < 0 && to->file->extra(HA_EXTRA_PREPARE_FOR_RENAME)) > + if (error < 0 && !to->s->tmp_table && > + to->file->extra(HA_EXTRA_PREPARE_FOR_RENAME)) > error= 1; > thd_progress_end(thd); > DBUG_RETURN(error > 0 ? -1 : 0); Isn't to->s->tmp_table always temporary? Shouldn't we check from->s->tmp_table instead? Regards, Sergey

2 1

Re: [Maria-developers] MDEV-9423: FTWRL and Binlog checkpoint
by Kristian Nielsen 29 Jun '16

29 Jun '16

Nirbhay Choubey <nirbhay(a)mariadb.com> writes: [Cc: maria-developers@, please always keep these discussions on the mailing list] > In Galera cluster, the state transfer scripts perform FTWRL and > copy data along with the last of all available binlog files to the > joiner node. > > After MDEV-181, I understand that the binlog checkpoint can be > in any of the binary log files (and not necessarily the last one). > > This seemingly has caused MDEV-9423, in which the joiner node > complains of the missing binlog file. > > Now the question is : Is FTWRL not sufficient to ensure that the > checkpoint is always the last binlog file? So if I understand correctly, the issue is related to having binlog files available during XA crash recovery. When the binlog file is rotated, there is a small window where both the latest and the previous binlog files are needed for crash recovery. The binlog checkpoint is the earliest binlog file that is needed for crash recovery, and it can be seen from the binlog checkpoint event. So the problem here is that a copy is made just after binlog rotation, and Galera only copies the most recent, mostly-empty binlog file, leaving insufficient information for XA recovery, right? One option to solve this is to always copy the last two binlog files. While it is theoretically possible to have the binlog checkpoint more than two files back, I think it will not occur in practice. Another option is to wait for the binlog checkpoint to reach the current binlog file. You can see this done in the test suite: mysql-test/include/wait_for_binlog_checkpoint.inc The binlog checkpointing happens asynchroneously, I *think* it can complete even while FTWRL is active, but I am not 100% sure though. The checkpoint happens after InnoDB has made its commits durable with fsync() or similar - only after that is it safe to discard the old binlog data and still have correct crash recovery. - Kristian.

3 11

Re: [Maria-developers] [MariaDB/server] MDEV-10298: Systemd hardening (#195)
by Kristian Nielsen 29 Jun '16

29 Jun '16

Sergey Vojtovich <notifications(a)github.com> writes: > Similar to other open source projects, the MariaDB Foundation needs to > have shared ownership of all code that is included in the MariaDB > distribution. The easiest way to achieve this is by submitting your > code under the BSD-new license. > > The other alternative is to sign the code contribution agreement which > can be found here: https://mariadb.com/kb/en/mariadb/mca/ As was concluded in this mailing thread, https://lists.launchpad.net/maria-developers/msg09606.html the third option is to contribute the code under GPLv2. Personally, I would strongly recommend to contribute under GPLv2 only. There are good and important reason why the GPL exists and why it is used by many of the most successful free software projects. Most of the code by core developers is contributed to the project under GPLv2 only, as is most of the major external contributions. Svoj, please update your "contributer request template" to mention prominently the GPLv2 option, and use it in all future cases. eg: ----------------------------------------------------------------------- Similar to other open source projects, the MariaDB Foundation needs to have the right to include and re-distribute under the GPL all code that is included in the MariaDB distribution. The easiest way to achieve this is by submitting your code under the same license as the MariaDB source code, GPLv2. The other alternatives are to submit under the BSD-new license, or to sign the code contribution agreement which can be found here: https://mariadb.com/kb/en/mariadb/mca/. These alternatives both provide further rights for the MariaDB Foundation and its members to use the code in your contribution, including the rights to submit it for inclusion in Oracle MySQL or to use it commercially in proprietary products. Please indicate in a comment below that you are contributing your new code of the whole pull request, including one or several files that are either new files or modified ones, under the GPLv2 license, under the BSD-new license, or that you have filled out the contribution agreement and sent it. ----------------------------------------------------------------------- Thanks, - Kristian.

2 1

Re: [Maria-developers] [MariaDB/server] MDEV-10303 - rsync_sst_rsync: ignore lost+found during database directory transfer (#196)
by Kristian Nielsen 29 Jun '16

29 Jun '16

Sergey Vojtovich <notifications(a)github.com> writes: > Hi Nicolas, > > Thanks for your contribution. JIRA task has been created to track this > pull request: https://jira.mariadb.org/browse/MDEV-10303 > > This task was added to 10.2.2 backlog, which hasn't been scheduled yet. > > Similar to other open source projects, the MariaDB Foundation needs to > have shared ownership of all code that is included in the MariaDB > distribution. The easiest way to achieve this is by submitting your > code under the BSD-new license. > > The other alternative is to sign the code contribution agreement which > can be found here: https://mariadb.com/kb/en/mariadb/mca/ As was concluded in this mailing thread, https://lists.launchpad.net/maria-developers/msg09606.html the third option is to contribute the code under GPLv2. Personally, I would strongly recommend to contribute under GPLv2 only. There are good and important reason why the GPL exists and why it is used by many of the most successful free software projects. Most of the code by core developers is contributed to the project under GPLv2 only, as is most of the major external contributions. - Kristian.

1 0

Re: [Maria-developers] eb9fab7: MDEV-9531: GROUP_CONCAT with ORDER BY inside takes a lot of memory while it's executed
by Sergei Golubchik 28 Jun '16

28 Jun '16

Hi, Sanja! On May 04, Olaksandr Byslkin wrote: still haven't fixed the typo? :) > revision-id: eb9fab76fb4322a7821e6502e6303ed381624429 (mariadb-10.1.13-18-geb9fab7) > parent(s): 732adec0a4c75d99389230feeb0deca0ad668de7 > committer: Oleksandr Byelkin > timestamp: 2016-05-04 19:15:31 +0200 > message: > > MDEV-9531: GROUP_CONCAT with ORDER BY inside takes a lot of memory while it's executed > > Limitation added to Red-Black tree. > > diff --git a/include/my_tree.h b/include/my_tree.h > index f8be55f..f1916b9 100644 > --- a/include/my_tree.h > +++ b/include/my_tree.h > @@ -57,11 +57,14 @@ typedef struct st_tree_element { > } TREE_ELEMENT; > > #define ELEMENT_CHILD(element, offs) (*(TREE_ELEMENT**)((char*)element + offs)) > +#define R_ELEMENT_CHILD(element, offs) ((TREE_ELEMENT**)((char*)element + offs)) what does "R_ELEMENT_CHILD" mean? What's "R"? > > typedef struct st_tree { > TREE_ELEMENT *root,null_element; > TREE_ELEMENT **parents[MAX_TREE_HEIGHT]; > + TREE_ELEMENT *free_element; why do you need a free_element pointer? > uint offset_to_key,elements_in_tree,size_of_element; > + uint elements_limit, del_direction; why do you need to support different directions here? > size_t memory_limit, allocated; > qsort_cmp2 compare; > void *custom_arg; > diff --git a/mysql-test/r/mysqld--help.result b/mysql-test/r/mysqld--help.result > index bef0390..3699b18 100644 > --- a/mysql-test/r/mysqld--help.result > +++ b/mysql-test/r/mysqld--help.result > @@ -240,6 +240,9 @@ The following options may be given as the first argument: > --group-concat-max-len=# > The maximum length of the result of function > GROUP_CONCAT() > + --group-concat-max-mem=# > + The maximum memory used to calculate result of function > + GROUP_CONCAT() No, please. There is group_concat_max_len already, it's all that user needs. There is no reason why one might need to configure a second limit on for the GROUP_CONCAT. > --gtid-domain-id=# Used with global transaction ID to identify logically > independent replication streams. When events can > propagate through multiple parallel paths (for example Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 3

Re: [Maria-developers] 0752333: MDEV-10161: wsrep_sync_wait not enabled when set to 1 in config file
by Sergei Golubchik 27 Jun '16

27 Jun '16

Hi, Nirbhay! On Jun 08, Nirbhay Choubey wrote: > revision-id: 075233312cc5bc6c242f4629f8293d4c67937f02 (mariadb-10.1.14-10-g0752333) > parent(s): c9f05974e618a869563f2360ef1ca910974a4f1c > author: Nirbhay Choubey > committer: Nirbhay Choubey > timestamp: 2016-06-08 16:03:01 -0400 > message: > > MDEV-10161: wsrep_sync_wait not enabled when set to 1 in config file > > Since wsrep_sync_wait & wsrep_causal_reads variables are related, > they are always kept in sync whenever one of them changes. > Same is tried on server start, where wsrep_sync_wait get updated > based on wsrep_causal_reads' value. But, since wsrep_causal_reads > is OFF by default, wsrep_sync_wait's value gets modified and loses > its WSREP_SYNC_WAIT_BEFORE_READ bit. > > Fixed by giving higher precedence to one that's explicitly set to > a non-default value and later to wsrep_sync_wait in case both are > set. > > diff --git a/sql/wsrep_mysqld.cc b/sql/wsrep_mysqld.cc > index d0798ef..fffb0cc 100644 > --- a/sql/wsrep_mysqld.cc > +++ b/sql/wsrep_mysqld.cc > @@ -567,7 +567,39 @@ int wsrep_init() > > wsrep_sst_auth_init(wsrep_sst_auth); > > - wsrep_causal_reads_update(&global_system_variables); > + /* > + On server start, there is no way to detect whether the default value > + for a system variable was set explicitly by the user. > + Now, since wsrep_causal_reads & wsrep_sync_wait are used for same > + purpose, a rule of precedence must be defined to keep their values > + in sync. It goes as following: This is wrong. First, in 10.1 there is a way to detect whether the value was set by a user. I_S.SYSTEM_VARIABLES can show that. But even if you'd detected that, it won't help you do distinguish between mysqld --wsrep-sync-wait=1 --wsrep-causal-reads=0 mysqld --wsrep-sync-wait=0 --wsrep-causal-reads=1 The only correct solution is to handle this in the mysqld_get_one_option switch, updating the dependent option when one of these two is set. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

[Maria-developers] Please review MDEV-10236 Where expression with NOT function gives incorrect result
by Alexander Barkov 27 Jun '16

27 Jun '16

Hi Serg. Please review a patch for MDEV-10236. Thanks.

2 3

Re: [Maria-developers] 3e61493: MDEV-10010 - Recursive call to mysql_rwlock_rdlock for LOCK_system_variables_hash
by Sergei Golubchik 27 Jun '16

27 Jun '16

Hi, Sergey! On Jun 27, Sergey Vojtovich wrote: > revision-id: 3e61493dff068c86b37f2e2ec0d0b6cbe9a255b6 (mariadb-10.1.14-28-g3e61493) > parent(s): 305bbbce6152b0909937659ecba4d51e5b21c094 > committer: Sergey Vojtovich > timestamp: 2016-06-27 15:01:22 +0400 > message: > > MDEV-10010 - Recursive call to mysql_rwlock_rdlock for LOCK_system_variables_hash > > Avoid recursive LOCK_system_variables_hash acquisition in > intern_sys_var_ptr() by pre-syncing dynamic session variables. Looks good, thanks! Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 79716e0: [MDEV-9127] Crash reporter often fails to show the query that crashed
by Sergei Golubchik 27 Jun '16

27 Jun '16

Hi, Vicentiu! On May 30, Vicentiu Ciorbaru wrote: > revision-id: 79716e07f15e8dabca4d2647ff5dbf189935581a (mariadb-10.0.25-6-g79716e0) > parent(s): eaf5376f808b3d71c442fb6b95048c4dcd684b08 > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2016-05-30 23:21:22 +0300 > message: > > [MDEV-9127] Crash reporter often fails to show the query that crashed > > Addreses are not necessarily between heap_start && heap_end. Malloc > calls using mmap can place pointers outside these bounds. In this case, > we'll warn the user that the query pointer is potentially invalid. > However, we'll attempt to print the data anyway after we're done > printing everything else. ok to push Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] d1a7ae2: MDEV-9618 solaris sparc build fails on 10.1.
by Sergei Golubchik 26 Jun '16

26 Jun '16

Hi, Alexey! On Jun 26, Alexey Botchkov wrote: > revision-id: d1a7ae232ef6e35e4f81871e3c9d0d318a5dcef1 (mariadb-10.1.14-28-gd1a7ae2) > parent(s): 305bbbce6152b0909937659ecba4d51e5b21c094 > committer: Alexey Botchkov > timestamp: 2016-06-26 00:11:30 +0400 > message: > > MDEV-9618 solaris sparc build fails on 10.1. > > Compiler on Sparc is strict about the 'const' modifiers > in function declarations and definitions. Meaning > they should be exactly same. ok to push Interestingly enough, I've just read about it few days ago - that const modifiers do *not* have to be exactly the same, the compiler can be lax about it, because these const modifiers do not mean anything for the caller. So, this strictness could be considered a bug in that Solaris C++ compiler. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0