May 2017 - developers - lists.mariadb.org

[Maria-developers] MDEV-10142 - Replace function
by jerome brauge 23 May '17

23 May '17

Hello Alexander, Here is patch for replace function in oracle mode. Do I have to add a test for replication ? Regards, Jérôme.

2 4

Re: [Maria-developers] [Commits] 55a4858fe6f: MDEV-12666: CURRENT_ROLE() does not work in a view
by Alexander Barkov 23 May '17

23 May '17

Hi Vicentiu, It seems that Item_func_sysconst abuses const_charset_converter(). I have the following proposals: - Instead of adding a new code into Item::const_charset_converter(), can you please override Item_func_sysconst::safe_charset_converter()? The direction proposed by Sergei's looks good: Item *safe_charset_converter(THD *thd, CHARSET_INFO *tocs) { + if (thd->lex->context_analysis_only & CONTEXT_ANALYSIS_ONLY_VIEW) + return Item_str_func::safe_charset_converter(thd, tocs); return const_charset_converter(thd, tocs, true, fully_qualified_func_name() } But it probably needs to be extended to handle prepared statement. At PREPARE-time it should not call const_charset_converter(). At EXECUTE-time it should be Ok to call const_charset_converter(). So can you please change Item_func_sysconst::const_item() to report false when in views and during PREPARE-time? And then change safe_charset_converter() as follows: Item *safe_charset_converter(THD *thd, CHARSET_INFO *tocs) { if (!const_item()) return Item_str_func::safe_charset_converter(thd, tocs); return const_charset_converter(thd, tocs, true, fully_qualified_func_name() } - Please add tests for DATABASE() and CURRENT_USER - Please add tests for PS with changing context between PREPARE and EXECUTE. Thanks. On 05/22/2017 06:15 PM, vicentiu wrote: > revision-id: 55a4858fe6f3fdc63a072b93b9bc3d8e51dc5e94 (mariadb-10.0.30-76-g55a4858fe6f) > parent(s): 8d1827f5540a0def82b1e2cbc557245c3e5e14ca > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2017-05-22 17:06:01 +0300 > message: > > MDEV-12666: CURRENT_ROLE() does not work in a view > > The problem lies in how CURRENT_ROLE is defined. The > Item_func_current_role inherits from Item_func_sysconst, which defines > a safe_charset_converter to be a const_charset_converter. > > During view creation, if there is no role previously set, the current_role() > function returns NULL. > > This is captured on item instantiation and the > const_charset_converter call subsequently returns an Item_null. > In turn, the function is replaced with Item_null and the view is > then created with an Item_null instead of Item_func_current_role. > > Without this patch, the first SHOW CREATE VIEW from the testcase would > have a where clause of WHERE role_name = NULL, while the second SHOW > CREATE VIEW would show a correctly created view. > > The solution proposed is to not replace any function with an Item_null, > but instead use an Item_static_string_func. > > --- > .../suite/roles/current_role_view-12666.result | 48 ++++++++++++++++++++ > .../suite/roles/current_role_view-12666.test | 52 ++++++++++++++++++++++ > sql/item.cc | 13 +++++- > 3 files changed, 112 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/suite/roles/current_role_view-12666.result b/mysql-test/suite/roles/current_role_view-12666.result > new file mode 100644 > index 00000000000..525748b4875 > --- /dev/null > +++ b/mysql-test/suite/roles/current_role_view-12666.result > @@ -0,0 +1,48 @@ > +CREATE USER has_role@'localhost'; > +GRANT ALL PRIVILEGES ON *.* TO has_role@'localhost'; > +CREATE ROLE test_role; > +GRANT test_role TO has_role@'localhost'; > +CREATE USER no_role@'localhost'; > +GRANT ALL PRIVILEGES ON *.* TO no_role@'localhost'; > +CREATE TABLE view_role_test ( > +id int primary key, > +role_name varchar(50) > +); > +INSERT INTO view_role_test VALUES (1, 'test_role'); > +CREATE OR REPLACE > +DEFINER = no_role@localhost > +SQL SECURITY INVOKER > +VIEW v_view_role_test > +AS > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > +show create view v_view_role_test; > +View Create View character_set_client collation_connection > +v_view_role_test CREATE ALGORITHM=UNDEFINED DEFINER=`no_role`@`localhost` SQL SECURITY INVOKER VIEW `v_view_role_test` AS select `view_role_test`.`id` AS `id`,`view_role_test`.`role_name` AS `role_name` from `view_role_test` where (`view_role_test`.`role_name` = current_role()) latin1 latin1_swedish_ci > +SET ROLE test_role; > +CREATE OR REPLACE > +DEFINER = no_role@localhost > +SQL SECURITY INVOKER > +VIEW v_view_role_test_2 > +AS > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > +show create view v_view_role_test_2; > +View Create View character_set_client collation_connection > +v_view_role_test_2 CREATE ALGORITHM=UNDEFINED DEFINER=`no_role`@`localhost` SQL SECURITY INVOKER VIEW `v_view_role_test_2` AS select `view_role_test`.`id` AS `id`,`view_role_test`.`role_name` AS `role_name` from `view_role_test` where (`view_role_test`.`role_name` = current_role()) latin1 latin1_swedish_ci > +SELECT CURRENT_USER(); > +CURRENT_USER() > +root@localhost > +SELECT CURRENT_ROLE(); > +CURRENT_ROLE() > +test_role > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > +id role_name > +1 test_role > +SELECT * FROM v_view_role_test; > +id role_name > +1 test_role > +drop user has_role@'localhost'; > +drop user no_role@'localhost'; > +drop role test_role; > +drop table view_role_test; > +drop view v_view_role_test; > +drop view v_view_role_test_2; > diff --git a/mysql-test/suite/roles/current_role_view-12666.test b/mysql-test/suite/roles/current_role_view-12666.test > new file mode 100644 > index 00000000000..86c74260b6e > --- /dev/null > +++ b/mysql-test/suite/roles/current_role_view-12666.test > @@ -0,0 +1,52 @@ > +CREATE USER has_role@'localhost'; > +GRANT ALL PRIVILEGES ON *.* TO has_role@'localhost'; > + > +CREATE ROLE test_role; > +GRANT test_role TO has_role@'localhost'; > + > +CREATE USER no_role@'localhost'; > +GRANT ALL PRIVILEGES ON *.* TO no_role@'localhost'; > + > +CREATE TABLE view_role_test ( > + id int primary key, > + role_name varchar(50) > + ); > + > +INSERT INTO view_role_test VALUES (1, 'test_role'); > + > + > +CREATE OR REPLACE > +DEFINER = no_role@localhost > +SQL SECURITY INVOKER > +VIEW v_view_role_test > +AS > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > + > +show create view v_view_role_test; > + > +SET ROLE test_role; > + > +CREATE OR REPLACE > +DEFINER = no_role@localhost > +SQL SECURITY INVOKER > +VIEW v_view_role_test_2 > +AS > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > + > +show create view v_view_role_test_2; > + > + > +SELECT CURRENT_USER(); > +SELECT CURRENT_ROLE(); > + > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > +SELECT * FROM v_view_role_test; > + > + > +drop user has_role@'localhost'; > +drop user no_role@'localhost'; > +drop role test_role; > + > +drop table view_role_test; > +drop view v_view_role_test; > +drop view v_view_role_test_2; > diff --git a/sql/item.cc b/sql/item.cc > index 4ce8396f71e..2ee3a6d61dc 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -1263,9 +1263,20 @@ Item *Item::const_charset_converter(CHARSET_INFO *tocs, > DBUG_ASSERT(fixed); > StringBuffer<64>tmp; > String *s= val_str(&tmp); > - if (!s) > + > + /* Functions that return NULL during prepare must not be replaced with > + Item_null as they may return non-null values during execution. > + Ex: CURRENT_ROLE() during view creation time can be NULL if no role > + is set. */ > + if (!s && !func_name) > return new Item_null((char *) func_name, tocs); > > + if (!s) > + { > + tmp.set_ascii("", 0); > + s = &tmp; > + } > + > if (!needs_charset_converter(s->length(), tocs)) > { > if (collation.collation == &my_charset_bin && tocs != &my_charset_bin && > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits >

1 0

Re: [Maria-developers] [Commits] b400696: MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff'
by Sergey Petrunia 22 May '17

22 May '17

Hi Varun, Please find my input below. I would like again to voice my dissatisfaction that I have to point out coding style violations in pretty much every patch I get. The other error in the patch is also pretty basic, ok to have it once but I think it is reasonable to expect more polished patches to be submitted for code review. On Sat, May 20, 2017 at 01:50:42AM +0530, Varun wrote: > revision-id: b400696386529fa63e79f723bbb06c05f7bae820 (mariadb-10.2.2-416-gb400696) > parent(s): fff61e31eca83a0a9af7c30e2bcda8309e9c695a > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-05-20 01:49:34 +0530 > message: > > MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > was corrupted, server crashes in opt_sum_query > > extended keys feature disabled if the length of extended key is longer than MAX_KEY_LEN > > --- > mysql-test/r/innodb_ext_key.result | 45 ++++++++++++++++++++++++++++++++++++++ > mysql-test/t/innodb_ext_key.test | 31 ++++++++++++++++++++++++++ > sql/table.cc | 36 +++++++++++++++++++++++++++++- > 3 files changed, 111 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/r/innodb_ext_key.result b/mysql-test/r/innodb_ext_key.result > index 1305be8..66391d6 100644 > --- a/mysql-test/r/innodb_ext_key.result > +++ b/mysql-test/r/innodb_ext_key.result > @@ -1133,5 +1133,50 @@ where index_date_updated= 10 and index_id < 800; > id select_type table type possible_keys key key_len ref rows Extra > 1 SIMPLE t2 range index_date_updated index_date_updated 13 NULL # Using index condition > drop table t0,t1,t2; > +# > +# MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > +# was corrupted, server crashes in opt_sum_query > +CREATE TABLE t1 ( > +pk INT, > +f1 VARCHAR(3), > +f2 VARCHAR(1024), > +PRIMARY KEY (pk), > +KEY(f2) > +) ENGINE=InnoDB CHARSET utf8; > +INSERT INTO t1 VALUES (1,'foo','abc'),(2,'bar','def'); > +SELECT MAX(t2.pk) FROM t1 t2 INNER JOIN t1 t3 ON t2.f1 = t3.f1 WHERE t2.pk <= 4; > +MAX(t2.pk) > +2 > +drop table t1; > +CREATE TABLE t1 ( > +pk1 INT, > +pk2 INT, > +f1 VARCHAR(3), > +f2 VARCHAR(1021), > +PRIMARY KEY (pk1,pk2), > +KEY(f2) > +) ENGINE=InnoDB CHARSET utf8; > +INSERT INTO t1 VALUES (1,2,'2','abc'),(2,3,'3','def'); > +explain format= json > +select * from t1 force index(f2) where pk1 <= 5 and pk2 <=5 and f2 = 'abc' and f1 <= '3'; > +EXPLAIN > +{ > + "query_block": { > + "select_id": 1, > + "table": { > + "table_name": "t1", > + "access_type": "range", > + "possible_keys": ["f2"], > + "key": "f2", > + "key_length": "3070", > + "used_key_parts": ["f2", "pk1"], > + "rows": 1, > + "filtered": 100, > + "index_condition": "t1.pk1 <= 5 and t1.pk2 <= 5 and t1.f2 = 'abc'", > + "attached_condition": "t1.f1 <= '3'" > + } > + } > +} > +drop table t1; > set optimizer_switch=@save_ext_key_optimizer_switch; > SET SESSION STORAGE_ENGINE=DEFAULT; ... > diff --git a/sql/table.cc b/sql/table.cc > index 398383e..7d88eba 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -2104,6 +2104,7 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > for (uint key=0 ; key < keys ; key++,keyinfo++) > { > uint usable_parts= 0; > + KEY* key_first_info; This variable may be used un-initialized. If I change the above line to be KEY* key_first_info=(KEY*)0x1; and run this statement: CREATE TABLE t1a ( pk INT, f1 VARCHAR(3), f2 VARCHAR(1024), f2a VARCHAR(1024), PRIMARY KEY (pk), KEY(f2), KEY(f2a) ) ENGINE=InnoDB CHARSET utf8; Then I get a crash. > keyinfo->name=(char*) share->keynames.type_names[key]; > keyinfo->name_length= strlen(keyinfo->name); > keyinfo->cache_name= > @@ -2118,19 +2119,38 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > keyinfo->name_length+1); > } > > + if(!key) Coding style: 'if (' ... > + key_first_info= keyinfo; > + > if (ext_key_parts > share->key_parts && key) > { > KEY_PART_INFO *new_key_part= (keyinfo-1)->key_part + > (keyinfo-1)->ext_key_parts; > uint add_keyparts_for_this_key= add_first_key_parts; > + uint length_bytes= 0, len_null_byte= 0, ext_key_length= 0; > + Field *field; > > /* > Do not extend the key that contains a component > defined over the beginning of a field. > */ > for (i= 0; i < keyinfo->user_defined_key_parts; i++) > - { > + { Coding style: wrong identation. > uint fieldnr= keyinfo->key_part[i].fieldnr; > + field= share->field[keyinfo->key_part[i].fieldnr-1]; > + > + if (field->null_ptr) > + len_null_byte= HA_KEY_NULL_LENGTH; > + > + if (field->type() == MYSQL_TYPE_BLOB || > + field->real_type() == MYSQL_TYPE_VARCHAR || > + field->type() == MYSQL_TYPE_GEOMETRY) > + { > + length_bytes= HA_KEY_BLOB_LENGTH; > + } > + > + ext_key_length+= keyinfo->key_part[i].length + len_null_byte > + + length_bytes; > if (share->field[fieldnr-1]->key_length() != > keyinfo->key_part[i].length) > { > @@ -2139,6 +2159,20 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > } > } > > + if (add_keyparts_for_this_key) > + { > + for (i= 0; i < add_keyparts_for_this_key; i++) > + { > + uint pk_part_length= key_first_info->key_part[i].store_length; > + if (ext_key_length + pk_part_length > MAX_KEY_LENGTH) > + { > + add_keyparts_for_this_key= i; > + break; > + } > + ext_key_length+= pk_part_length; > + } > + } > + > if (add_keyparts_for_this_key < (keyinfo->ext_key_parts - > keyinfo->user_defined_key_parts)) > { BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 55a4858fe6f: MDEV-12666: CURRENT_ROLE() does not work in a view
by Sergei Golubchik 22 May '17

22 May '17

Hi, Vicentiu! Another possible approach (that doesn't necessarily penalizes other const functions that happen to return NULL) would be for Item_func_sysconst not to behave as const during view creation. Because they really aren't, sysconst functions can change between view creation and a select from the view. The fix would be, then Item *safe_charset_converter(THD *thd, CHARSET_INFO *tocs) { + if (thd->lex->context_analysis_only & CONTEXT_ANALYSIS_ONLY_VIEW) + return Item_str_func::safe_charset_converter(thd, tocs); return const_charset_converter(thd, tocs, true, fully_qualified_func_name() } And possibly (not sure) other values of context_analysis_only would need a similar treatment here. On May 22, vicentiu wrote: > revision-id: 55a4858fe6f3fdc63a072b93b9bc3d8e51dc5e94 (mariadb-10.0.30-76-g55a4858fe6f) > parent(s): 8d1827f5540a0def82b1e2cbc557245c3e5e14ca > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2017-05-22 17:06:01 +0300 > message: > > MDEV-12666: CURRENT_ROLE() does not work in a view > > The problem lies in how CURRENT_ROLE is defined. The > Item_func_current_role inherits from Item_func_sysconst, which defines > a safe_charset_converter to be a const_charset_converter. > > During view creation, if there is no role previously set, the current_role() > function returns NULL. > > This is captured on item instantiation and the > const_charset_converter call subsequently returns an Item_null. > In turn, the function is replaced with Item_null and the view is > then created with an Item_null instead of Item_func_current_role. > > Without this patch, the first SHOW CREATE VIEW from the testcase would > have a where clause of WHERE role_name = NULL, while the second SHOW > CREATE VIEW would show a correctly created view. > > The solution proposed is to not replace any function with an Item_null, > but instead use an Item_static_string_func. > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MariaDB License Clarification
by Ryan Coe 19 May '17

19 May '17

Hello, My name is Ryan Coe and I've been maintaining the MariaDB package for Buildroot (https://buildroot.org). In commit 577915def8d39223aa23a06f898e11e82e72365a, COPYING.LESSER was removed. Our packaging currently states the MariaDB license as GPL-2.0 (server), GPL-2.0 with FLOSS exception (GPL client library), LGPL-2.0 (LGPL client library). Could I please get clarification on how the different components are licensed? Also, the GitHub page is still referencing COPYING.LESSER in the README. Thank you.

2 1

Re: [Maria-developers] strange IF in translog_flush_buffers()
by Sergei Golubchik 18 May '17

18 May '17

Hi, Sanja! On May 17, Oleksandr Byelkin wrote: > Hi! > > Actually code should work as it is, but it check unnecessary condition and > also very dificult to understand. I propose following fix (comment in it > explains why it work now and why condition is always TRUE) I cannot review it, this is between you and Monty. If you think it's ok, please push it (into 10.0). Or I can push it together with all other similar fixes. > diff --git a/storage/maria/ma_loghandler.c b/storage/maria/ma_loghandler.c > index 86e51cc3526..e680e830703 100644 > --- a/storage/maria/ma_loghandler.c > +++ b/storage/maria/ma_loghandler.c > @@ -7789,8 +7789,23 @@ void translog_flush_buffers(TRANSLOG_ADDRESS *lsn, > translog_force_current_buffer_to_finish(); > translog_buffer_unlock(buffer); > } > - else if (log_descriptor.bc.buffer->prev_last_lsn != LSN_IMPOSSIBLE) > + else > { > + if (log_descriptor.bc.buffer->last_lsn == LSN_IMPOSSIBLE) > + { > + /* > + In this case both last_lsn & prev_last_lsn are LSN_IMPOSSIBLE > + otherwise it will go in the first IF because LSN_IMPOSSIBLE less > + then any real LSN and cmp_translog_addr(*lsn, > + log_descriptor.bc.buffer->prev_last_lsn) will be TRUE > + */ > + DBUG_ASSERT(log_descriptor.bc.buffer->prev_last_lsn == > + LSN_IMPOSSIBLE); > + DBUG_PRINT("info", ("There is no LSNs yet generated => do nothing")); > + translog_unlock(); > + DBUG_VOID_RETURN; > + } > + > /* fix lsn if it was horizon */ > *lsn= log_descriptor.bc.buffer->prev_last_lsn; > DBUG_PRINT("info", ("LSN to flush fixed to prev last lsn: (%lu,0x%lx)", > @@ -7799,13 +7814,6 @@ void translog_flush_buffers(TRANSLOG_ADDRESS *lsn, > TRANSLOG_BUFFERS_NO); > translog_unlock(); > } > - else if (log_descriptor.bc.buffer->last_lsn == LSN_IMPOSSIBLE) > - { > - DBUG_PRINT("info", ("There is no LSNs yet generated => do nothing")); > - translog_unlock(); > - DBUG_VOID_RETURN; > - } > - > /* flush buffers */ > *sent_to_disk= translog_get_sent_to_disk(); > if (cmp_translog_addr(*lsn, *sent_to_disk) > 0) Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MDEV-12803 Improve function parameter data type control
by Alexander Barkov 15 May '17

15 May '17

Hello Alexey, can you please review a patch for MDEV-12803? Thanks!

1 0

[Maria-developers] String functions have their item->maybe_null=true unnecessarily, blocking optimizations
by Sergey Petrunia 15 May '17

15 May '17

Hi Alexander, I was looking at https://jira.mariadb.org/browse/MDEV-11893 and found this piece of code: bool Item_str_func::fix_fields(THD *thd, Item **ref) { bool res= Item_func::fix_fields(thd, ref); /* In Item_str_func::check_well_formed_result() we may set null_value flag on the same condition as in test() below. */ maybe_null= maybe_null || thd->is_strict_mode(); return res; } This has been introduced by this cset: https://github.com/MariaDB/server/commit/af22eb35e577ef17226faf662f2cffc470… Which says: Add Item_str_func::fix_fields() implementation, and set maybe_null to TRUE if we are in the SQL mode that requires some functions to return null even if they normally do not. The patch has only one example of CHAR() function doing that. At the same time, not having NOT-NULL attribute disallows query opimizations (see MDEV-11893 for an example). I think we should have this maybe_null= maybe_null || thd->is_strict_mode(); logic on a case-by-case basis only. I wanted to check with you - are there any other known string functions that, as the patch puts it "return null even if they normally do not"? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

2 1

[Maria-developers] MDEV-11340 Allow multiple alternative authentication methods for the same user
by serg＠mariadb.org 15 May '17

15 May '17

Hi, Ankit! I've started writing down my thoughts about MDEV-11340 "Allow multiple alternative authentication methods for the same user" See MDEV-11340 and feel free to comment on it. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] 8edbb1117a9: MDEV-12741: innodb.ibuf_not_empty failed in buildbot with "InnoDB: Trying to do I/O to a tablespace which does not exist"
by Marko Mäkelä 15 May '17

15 May '17

On Thu, May 11, 2017 at 11:25 AM, jan <jan.lindstrom(a)mariadb.com> wrote: > revision-id: 8edbb1117a9e1fd81fbd08b8f1d06c72efe38f44 (mariadb-10.2.5-168- > g8edbb1117a9) > parent(s): f4df8c00c957146f04db031f1f9f80aab229d1d2 > author: Jan Lindström > committer: Jan Lindström > timestamp: 2017-05-11 11:15:37 +0300 > message: > > MDEV-12741: innodb.ibuf_not_empty failed in buildbot with "InnoDB: Trying > to do I/O to a tablespace which does not exist" > > Background thread is doing ibuf merge, in buf0rea.cc > buf_read_ibuf_merge_pages(). > It first tries to get page_size and if space is not found it deletes them, > but > as we do not hold any mutexes, space can be marked as stopped between that > and buf_read_page_low() for same space. This naturally leads seen error > message on log. > > buf_read_page_low(): Add parameter ignore_missing_space = false that > is passed to fil_io() > [snip] > @@ -178,7 +180,7 @@ buf_read_page_low( > > *err = fil_io( > request, sync, page_id, page_size, 0, page_size.physical(), > - dst, bpage); > + dst, bpage, ignore_missing_space); > > if (sync) { > thd_wait_end(NULL); > Can you please add the parameter to the IORequest? It would seem to be a more appropriate place for this. (Ideally, fil_io() would only take the request as a parameter, but we do not want to do such a big refactoring right now.) Marko

1 0