- developers - lists.mariadb.org

Re: [Maria-developers] MDEV-14013
by Alexander Barkov 14 Oct '17

14 Oct '17

Hello Jerome, Thank you very much for you contribution! On 10/11/2017 02:32 PM, jerome brauge wrote: > Hello Alexander, > Here is the patch for MDEV-14013, can you review it ? The patch looks very good. I only found small things: 1. LEX::make_text_literal_concat() thd->variables.character_set_client was used instread of thd->variables.collation_connection 2. LEX::make_text_literal_concat() was somewhat hard to read. I propose to change the logic to do: if (item->type() == Item::NULL_ITEM) { } instead of if (item->type() != Item::NULL_ITEM || !(thd->variables.sql_mode & MODE_EMPTY_STRING_IS_NULL)) I think it looks clearer this way. Also, I added some DBUG_ASSERTs to make more sure that a non-Item_string item is not erroneously cast to Item_string. 3. Also, I found that it's easier to place the new methods into THD instead of LEX. Please find a patch with changes 1,2,3 attached. Do you agree with these changes? Also, can you please add some tests: - SHOW CREATE VIEW into "Test in a view" in empty_string_literal.result.diff - EXPLAIN EXTENDED for the affected grammar: EXPLAIN EXTENDED SELECT ''; EXPLAIN EXTENDED SELECT _latin1''; EXPLAIN EXTENDED SELECT N''; EXPLAIN EXTENDED SELECT '' ''; Thanks! > > Thanks. > > >> -----Message d'origine----- >> De : Alexander Barkov [mailto:bar@mariadb.org] >> Envoyé : mercredi 11 octobre 2017 04:15 >> À : jerome brauge >> Objet : Re: Question about MDEV-10574 >> >> Hello Jerome, >> >> >> On 10/10/2017 04:23 PM, jerome brauge wrote: >>> Hello Alexander, >>> For sql_mode EMPTY_STRING_IS_NULL, do you think that it must cover call >> of statement with use of user variable initialized with an empty string ? >>> >>> Ex : >>> SET sql_mode=default; >>> SET @param=''; >>> >>> SET sql_mode=EMPTY_STRING_IS_NULL; >>> PREPARE test FROM 'select 1,?'; >>> EXECUTE test USING @param; >>> >>> Or >>> >>> delimiter / >>> CREATE OR REPLACE PROCEDURE p1(IN a VARCHAR(50)) BEGIN >>> IF a is null THEN >>> SELECT 'IS NULL'; >>> ELSE >>> select 'IS NOT NULL'; >>> END IF; >>> END; >>> / >>> delimiter ; >>> SET sql_mode=default; >>> SET @a=''; >>> >>> SET sql_mode=EMPTY_STRING_IS_NULL; >>> EXECUTE IMMEDIATE 'CALL p1(?)' USING @a; CALL p1(@a); >>> >>> For me, it's a study case and we can forget it. >> >> I agree. Oracle doesn't have user variables. >> >>> >>> Regards, >>> Jérôme. >>> >>>> -----Message d'origine----- >>>> De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : samedi 7 >>>> octobre 2017 11:44 À : jerome brauge Objet : Re: Patch for MDEV-10574 >>>> >>>> Hello Jerome, >>>> >>>> >>>> On 10/06/2017 01:41 PM, jerome brauge wrote: >>>>> Hello Alexander, >>>>> >>>>>> -----Message d'origine----- >>>>>> De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : jeudi 5 >>>>>> octobre 2017 21:26 À : jerome brauge Objet : Re: Patch for >>>>>> MDEV-10574 >>>>>> >>>>>> Hello Jerome, >>>>>> >>>>>> >>>>>> On 10/05/2017 12:45 PM, jerome brauge wrote: >>>>>>> Hello Alexander, >>>>>>> Have you gone ahead on your reflection about nulls and empty strings >> ? >>>>>> >>>>>> Yes, I think so. >>>>>> >>>>>> One addition: Monty suggested to rename flags to: >>>>>> - EMPTY_STRING_IS_NULL for literals >>>>>> - EMPTY_STRING_IS_NULL_RESULTS for functions. >>>>>> >>>>>> I'm fine with EMPTY_STRING_IS_NULL for literals, but think that >>>>>> EMPTY_STRING_IS_NULL_FUNCTIONS should be more clear than >>>>>> EMPTY_STRING_IS_NULL_RESULTS. >>>>>> Which name do you prefer for functions? >>>>> >>>>> I think like you. >>>>> For me, EMPTY_STRING_IS_NULL_RESULTS sounds like including >>>> RESULTSET >>>>> of select (which is perhaps included in point 4) >>>> >>>> Okey, let's go with _FUNCTIONS then. >>>> >>>>> >>>>>>> >>>>>>> Can I >>>>>>> - redo my pull request for substring_oracle function >>>>>>> - make a patch for first new sql_mode >>>>>> (MODE_EMPTY_AS_NULL_LITERALS) >>>>>> >>>>>> >>>>>> Yes, please. Let's move forward. >>>>>> >>>>>> Let's push the patch for substr_oracle first. >>>>>> Let's keep it return empty strings for the cases when the >>>>>> substring_length parameters is less than 1. >>>>>> It will start automatically returning NULL when we add the new >>>>>> sql_mode for functions. >>>>> >>>>> Pull request is done. >>>> >>>> I have merged it. Thanks! >>>> >>>>> >>>>>> Also, let's go ahead with the part for literals. >>>>> >>>>> Ok, I will make a separate patch. >>>>> Many thanks. >>>>> >>>>>> >>>>>> >>>>>> I have created MDEVs. Please use them in the commit comment and in >>>>>> the >>>>>> tests: >>>>>> >>>>>> MDEV-14012 sql_mode=Oracle: substr(): treat position 0 as position >>>>>> 1 >>>>>> MDEV-14013 sql_mode=EMPTY_STRING_IS_NULL >>>>>> >>>>>> >>>>>> Thanks! >>>>>> >>>>>>> >>>>>>> Regards, >>>>>>> Jérôme. >>>>>>> >>>>>>> >>>>>>>> -----Message d'origine----- >>>>>>>> De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : mercredi >>>>>>>> 27 septembre 2017 08:50 À : jerome brauge Objet : Re: Patch for >>>>>>>> MDEV-10574 >>>>>>>> >>>>>>>> Hello Jerome, >>>>>>>> >>>>>>>> I told to Monty yesterday. >>>>>>>> >>>>>>>> We were going to implement the following transformations: >>>>>>>> >>>>>>>>> We are considering to implement both of the following options >>>>>>>>> (with a >>>>>>>>> switch) >>>>>>>>> >>>>>>>>> Transform Insert >>>>>>>>> - insert values ("") -> insert values (null) >>>>>>>>> >>>>>>>>> Transform Select >>>>>>>>> >>>>>>>>> where v=x => (v <> "" and V=X) >>>>>>>>> where v is null => (v="" or v is null) >>>>>>>>> >>>>>>>> >>>>>>>> See MDEV-10574 for details. >>>>>>>> >>>>>>>> Monty did not fully understand the idea of translating literals >>>>>>>> and >>>>>> functions >>>>>>>> from empty strings to null. He asked if you can explain use cases >>>>>>>> for these transformations. >>>>>>>> >>>>>>>> But we agreed that: >>>>>>>> >>>>>>>> 1. It's OK to have multiple NULL handling flags in sql_mode. >>>>>>>> >>>>>>>> 2. New flags will not be enabled in 10.3, neither in >>>>>>>> sql_mode=DEFAULT, >>>>>> nor in >>>>>>>> sql_mode=ORACLE. So one will have to specify them explicitly: >>>>>>>> set >>>>>>>> >>>>>> >>>> >> sql_mode='ORACLE,MODE_null_transfrormation_flag1,MODE_null_transfor >>>>>>>> mation_flag2'; >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> So I propose we introduce the following flags: >>>>>>>> >>>>>>>> >>>>>>>> 1. MODE_EMPTY_AS_NULL_LITERALS - for literals and PS >> parameters, >>>> as >>>>>> in >>>>>>>> your patch. >>>>>>>> >>>>>>>> 2. MODE_EMPTY_AS_NULL_FUNCTIONS - for functions. >>>>>>>> This should be done as a separate patch. >>>>>>>> Note, your patch currently implements empty-to-null translation >>>>>>>> only for Item_str_func descendants that use >> make_empty_result(). >>>>>>>> This patch should be extended to cover Item_str_func descendants >>>>>>>> which do not use make_empty_result(), as well as >>>>>>>> all other Item_func_or_sum descendants >>>>>>>> (which are not Item_str_func descendants). >>>>>>>> >>>>>>>> >>>>>>>> 3. MODE_EMPTY_AS_NULL_UPDATES >>>>>>>> >>>>>>>> For INSERT and UPDATE transformation, as in MDEV: >>>>>>>> >>>>>>>> insert values ("") -> insert values (null) >>>>>>>> update set a='' -> update set a=null; >>>>>>>> >>>>>>>> >>>>>>>> 4. MODE_EMPTY_AS_NULL_PREDICATES >>>>>>>> >>>>>>>> For predicate transformation, as in MDEV: >>>>>>>> >>>>>>>> where v=x => (v <> "" and V=X) >>>>>>>> where v is null => (v="" or v is null) >>>>>>>> >>>>>>>> >>>>>>>> What do you think about this proposal with multiple flags? >>>>>>>> Note, we can do N1 and N2 now, and add N3 and N4 and later. >>>>>>>> >>>>>>>> Also, can you please write a description why you need N1 and N2. >>>>>>>> Monty thinks that N3 and N4 should cover most use cases. >>>>>>>> So we need a good rationale for Monty and Sergei explaining why >>>>>>>> we >>>>>> adding >>>>>>>> these flags N1 and N2. >>>>>>>> >>>>>>>> >>>>>>>> Thanks! >>>>>>>> >>>>>>>> >>>>>>>> On 09/26/2017 04:33 PM, jerome brauge wrote: >>>>>>>>> Hi Alexander, >>>>>>>>> I'm disappointed. >>>>>>>>> Can we at least keep this part in sql_prepare : >>>>>>>>> >>>>>>>>> /* >>>>>>>>> set_param_str_oracle : convert empty string to null value >>>>>>>>> This allow to bind empty string in JDBC as a null value. >>>>>>>>> Ex : >>>>>>>>> String sel="select coalesce(?,'null param') from dual"; >>>>>>>>> PreparedStatement ps_sel = m_cnx.prepareStatement(sel); >>>>>>>>> ps_sel.setString(1, ""); >>>>>>>>> ResultSet rs = ps_sel.executeQuery(); >>>>>>>>> while (rs.next()) >>>>>>>>> { >>>>>>>>> System.out.println(rs.getString(1)); >>>>>>>>> } >>>>>>>>> Returns : 'null param' >>>>>>>>> */ >>>>>>>>> static void set_param_str_oracle(Item_param *param, uchar >> **pos, >>>>>> ulong >>>>>>>>> len) { >>>>>>>>> ulong length= get_param_length(pos, len); >>>>>>>>> if (length == 0) >>>>>>>>> param->set_null(); >>>>>>>>> else >>>>>>>>> { >>>>>>>>> if (length > len) >>>>>>>>> length= len; >>>>>>>>> param->set_str((const char *) *pos, length); >>>>>>>>> *pos+= length; >>>>>>>>> } >>>>>>>>> } >>>>>>>>> >>>>>>>>> Regards, >>>>>>>>> Jérôme. >>>>>>>>> >>>>>>>>> >>>>>>>>>> -----Message d'origine----- >>>>>>>>>> De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : mardi >>>>>>>>>> 26 septembre 2017 13:34 À : jerome brauge Objet : Re: Patch for >>>>>>>>>> MDEV-10574 >>>>>>>>>> >>>>>>>>>> Hello Jérôme, >>>>>>>>>> >>>>>>>>>> I'm afraid we cannot accept this change at this point of time. >>>>>>>>>> >>>>>>>>>> The problem is that all around the code we assume that NULL is >>>>>>>>>> not equal to ''. >>>>>>>>>> >>>>>>>>>> Changing '' to NULL only in the visible part of the iceberg >>>>>>>>>> (such as in the parser and in >>>>>>>>>> Item_str_func::make_empty_result()) >>>>>>>>>> will likely introduce various anomalies in fundamental things >>>>>>>>>> like joins execution, optimizer, equality propagation, etc. >>>>>>>>>> >>>>>>>>>> Sorry, we don't have resources to dive into this topic deeper now. >>>>>>>>>> Moreover, Oracle's documentation suggests not to reply on the >>>>>>>>>> fact >>>>>> that '' >>>>>>>>>> and NULL are treated as same, as this can change in the future. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 09/20/2017 03:52 PM, jerome brauge wrote: >>>>>>>>>>> Hello Alexander, >>>>>>>>>>> Here is a new version of this patch (minor optimizations) >>>>>>>>>>> >>>>>>>>>>> Regards, >>>>>>>>>>> Jérôme. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> -----Message d'origine----- >>>>>>>>>>>> De : Maria-developers [mailto:maria-developers- >>>>>>>>>>>> bounces+j.brauge=qualiac.com(a)lists.launchpad.net] De la part >>>> de >>>>>>>>>>>> bounces+jerome >>>>>>>>>>>> brauge >>>>>>>>>>>> Envoyé : mardi 19 septembre 2017 16:09 À : 'Alexander Barkov' >>>>>>>>>>>> Cc : MariaDB Developers >>>>>>>>>>>> (maria-developers(a)lists.launchpad.net) >>>>>>>>>>>> Objet : [Maria-developers] Patch for MDEV-10574 >>>>>>>>>>>> >>>>>>>>>>>> Alexander, >>>>>>>>>>>> Here is the patch I was talking about. >>>>>>>>>>>> It not address all aspects of MDEV-10574, especially, empty >>>>>>>>>>>> strings in table column are not view as null. >>>>>>>>>>>> But as in oracle mode, no empty string in columns should be >>>>>>>>>>>> created, an fully oracle application will never fall in this case. >>>>>>>>>>>> >>>>>>>>>>>> Regards, >>>>>>>>>>>> Jérôme. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>>> -----Message d'origine----- >>>>>>>>>>>>> De : jerome brauge >>>>>>>>>>>>> Envoyé : mardi 19 septembre 2017 14:13 À : 'Alexander >> Barkov' >>>>>>>>>>>>> Objet : RE: Oracle strings functions compatibility : substr >>>>>>>>>>>>> >>>>>>>>>>>>> Hello Alexander, >>>>>>>>>>>>> Yes I'm aware. >>>>>>>>>>>>> This point will be treated by my next patch which will also >>>>>>>>>>>>> affect the others string functions (a part of MDEV-10574). >>>>>>>>>>>>> I will refresh this new patch with the current development >>>>>>>>>>>>> branch, and I submit to you. >>>>>>>>>>>>> Regards. >>>>>>>>>>>>> Jérôme. >>>>>>>>>>>>> >>>>>>>>>>>>>> -----Message d'origine----- De : Alexander Barkov >>>>>>>>>>>>>> [mailto:bar@mariadb.org] Envoyé : >>>> mardi >>>>>> 19 >>>>>>>>>>>>>> septembre 2017 13:16 À : jerome brauge Objet : Re: Oracle >>>>>>>>>>>>>> strings functions compatibility : substr >>>>>>>>>>>>>> >>>>>>>>>>>>>> Hi Jerome, >>>>>>>>>>>>>> >>>>>>>>>>>>>> I noticed one more difference in SUBSTR: >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> SELECT SUBSTR('aaa',1,-1) FROM DUAL; >>>>>>>>>>>>>> >>>>>>>>>>>>>> MariaDB returns an empty string. >>>>>>>>>>>>>> Oracle returns NULL. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Here's Oracle's documentation: >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>> >>>>>> >> https://docs.oracle.com/cd/B19306_01/server.102/b14200/functions162. >>>>>>>>>>>>>> ht >>>>>>>>>>>>>> m >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> I think we should do this in the same patch, to avoid >>>>>>>>>>>>>> behavior change in the future. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Would you like to try doing this additional change? >>>>>>>>>>>>>> Or would you like me to make an incremental patch on top >> of >>>>>> yours? >>>>>>>>>>>>>> >>>>>>>>>>>>>> Thanks! >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 09/19/2017 10:33 AM, jerome brauge wrote: >>>>>>>>>>>>>>> Hello Alexander, >>>>>>>>>>>>>>> It's done. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Do you have news from Sergei for MDEV-12874, MDEV- >> 13417 >>>>>> and >>>>>>>>>>>> MDEV- >>>>>>>>>>>>>> 13418 ? >>>>>>>>>>>>>> >>>>>>>>>>>>>> I asked Sergei, awaiting for his answer. I'll let you know. >>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Thank you very much. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> -----Message d'origine----- De : Alexander Barkov >>>>>>>>>>>>>>>> [mailto:bar@mariadb.org] Envoyé : lundi >>>>>>>>>>>>>>>> 18 septembre 2017 18:12 À : jerome brauge Objet : Re: >>>>>>>>>>>>>>>> Oracle strings functions compatibility : substr >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Hello Jerome, >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On 09/18/2017 05:14 PM, jerome brauge wrote: >>>>>>>>>>>>>>>>> Hello Alexander, >>>>>>>>>>>>>>>>> I don't understand how I've missed such an obvious >>>> solution ! >>>>>>>>>>>>>>>>> Here is the new patch. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Thanks. Now it looks simpler. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Can you please do one small thing: >>>>>>>>>>>>>>>> move get_position() from "public:" to "protected:". >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Ok to make a pull request after this change. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Thank you very much! >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Regards. >>>>>>>>>>>>>>>>> Jérôme. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> -----Message d'origine----- De : Alexander Barkov >>>>>>>>>>>>>>>>>> [mailto:bar@mariadb.org] Envoyé : >>>>>> lundi >>>>>>>>>>>>>>>>>> 18 septembre 2017 11:30 À : jerome brauge Objet : Re: >>>>>> Oracle >>>>>>>>>>>>>>>>>> strings functions compatibility : substr >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Hello Jerome, >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> On 08/17/2017 04:43 PM, jerome brauge wrote: >>>>>>>>>>>>>>>>>>> Hello Alexander, >>>>>>>>>>>>>>>>>>> Here is a patch for function SUBSTR in >> sql_mode=oracle >>>>>>>>>>>>>>>>>>> (If position is 0, >>>>>>>>>>>>>>>>>> then it is treated as 1). >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> I'm thinking of how to get a smaller patch. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Wouldn't it be possible to introduce: >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> virtual longlong get_position(); >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> longlong Item_func_substr::get_position() { >>>>>>>>>>>>>>>>>> return args[1]->val_int(); } >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> longlong Item_func_substr_oracle::get_position() >>>>>>>>>>>>>>>>>> { >>>>>>>>>>>>>>>>>> longlong pos= args[1]->val_int(); >>>>>>>>>>>>>>>>>> return pos == 0 ? 1 : pos; } >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> and just replace all calls for args[1]->val_int() to >>>>>> get_position() >>>>>>>> ? >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Thanks! >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>>>>>>> Jérôme. >>>>>>>>>>>>>>>>>>>

2 2

Re: [Maria-developers] [Commits] e0a1e63: MDEV-9619: Assertion `null_ref_table' failed in virtual table_map Item_direct_view_ref::used_tables() const on 2nd execution of PS
by Sergei Golubchik 13 Oct '17

13 Oct '17

Hi, Oleksandr! On Feb 18, Oleksandr Byelkin wrote: > revision-id: e0a1e632e4e6084cd989f217943618f4e9ac9179 (mariadb-5.5.54-25-ge0a1e63) > parent(s): 29d78dbb44ee9890b6bc28873344f20fc9157928 > committer: Oleksandr Byelkin > timestamp: 2017-02-18 17:47:31 +0100 > message: > > MDEV-9619: Assertion `null_ref_table' failed in virtual table_map > Item_direct_view_ref::used_tables() const on 2nd execution of PS > > Refer left expression indirectly in case it changes from execution to > execution. Looks reasonable. Ok to push with these changes: 1. replace - DBUG_ASSERT(null_ref_table); + DBUG_ASSERT(fixed); as discussed. 2. Remove bool Item_direct_view_ref::fix_fields(THD *thd, Item **reference) { - DBUG_ASSERT(1); these two changes should, probably, be done in a separate cleanup commit. 3. > diff --git a/sql/item.h b/sql/item.h > index 89155ac..dff7bc4 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -1543,6 +1543,8 @@ class Item { > virtual void mark_as_condition_AND_part(TABLE_LIST *embedding) {}; > }; > > +typedef Item (*Item_ptr); > + remove this, your patch compiles just fine without it. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] Please review MDEV-10802 TIMESTAMP NOT NULL field with explicit_defaults_for_timestamp and NO_ZERO_DATE shouldn't throw error
by Alexander Barkov 13 Oct '17

13 Oct '17

Hi Sergei, Please review a patch for MDEV-10802. Thanks!

2 2

Re: [Maria-developers] [Commits] 06bb7ee: MDEV-10892 - rpl.rpl_semi_sync_uninstall_plugin fails with Assertion `0' failure
by Sergei Golubchik 13 Oct '17

13 Oct '17

Hi, Sergey! On Oct 12, Sergey Vojtovich wrote: > revision-id: 06bb7eef9882536fc1d99b4462a65b53ec9f137e (mariadb-5.5.57-14-g06bb7ee) > parent(s): 93aadda513d8b5c2b49001514e235c4fdd73e08a > committer: Sergey Vojtovich > timestamp: 2017-10-12 15:37:45 +0400 > message: > > MDEV-10892 - rpl.rpl_semi_sync_uninstall_plugin fails with Assertion `0' failure > in buildbot > > Removed plugin_array_version: it is being checked without mutex protection and > thus is prone to data race and race conditions. In effect plugins are not > protected from concurrent destruction. > > Removed state_mask inversion: doesn't seem to make any sense. > > When collecting local plugins list, only add plugins that match state_mask. > > Use plugin ref counting to protect against concurrent plugin destruction. > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 6e04564..512548b 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -262,7 +262,7 @@ int fill_plugins(THD *thd, TABLE_LIST *tables, COND *cond) > TABLE *table= tables->table; > > if (plugin_foreach_with_mask(thd, show_plugins, MYSQL_ANY_PLUGIN, > - ~PLUGIN_IS_FREED, table)) > + ~(PLUGIN_IS_FREED | PLUGIN_IS_DYING), table)) > DBUG_RETURN(1); > > DBUG_RETURN(0); > @@ -5294,7 +5294,8 @@ int fill_schema_engines(THD *thd, TABLE_LIST *tables, COND *cond) > DBUG_ENTER("fill_schema_engines"); > if (plugin_foreach_with_mask(thd, iter_schema_engines, > MYSQL_STORAGE_ENGINE_PLUGIN, > - ~PLUGIN_IS_FREED, tables->table)) > + ~(PLUGIN_IS_FREED | PLUGIN_IS_DYING), > + tables->table)) > DBUG_RETURN(1); > DBUG_RETURN(0); > } agree, ok to push Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] 96e982b: MDEV-7140 - Sporadic buildbot failures in rpl.rpl_semi_sync_uninstall_plugin
by Sergei Golubchik 13 Oct '17

13 Oct '17

Hi, Sergey! On Oct 12, Sergey Vojtovich wrote: > revision-id: 96e982bb1a72db47a41b97bb632b595b347d2956 (mariadb-5.5.57-14-g96e982b) > parent(s): 93aadda513d8b5c2b49001514e235c4fdd73e08a > committer: Sergey Vojtovich > timestamp: 2017-10-12 14:42:17 +0400 > message: > > MDEV-7140 - Sporadic buildbot failures in rpl.rpl_semi_sync_uninstall_plugin > > Removed plugin_array_version: it is being checked without mutex protection and > thus is prone to data race and race conditions. In effect plugins are not > protected from concurrent destruction. > > Removed state_mask inversion: doesn't seem to make any sense. > > When collecting local plugins list, only add plugins that match state_mask. > > Use plugin ref counting to protect against concurrent plugin destruction. ok to push Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MDEV-13530 VARBINARY doesn't convert to to BLOB for sizes 65533, 65534 and 65535
by Alexander Barkov 12 Oct '17

12 Oct '17

Hi Sergei, Please review a patch for MDEV-13530. Thanks!

1 0

Re: [Maria-developers] 46b93fc: MDEV-13676: Field "create Procedure" is NULL, even if the the user has role which is the definer. (SHOW CREATE PROCEDURE)
by Sergei Golubchik 11 Oct '17

11 Oct '17

Hi, Vicentiu! Looks good! A couple of comments, see below. Ok to push after fixing, the second review is not needed. On Oct 09, Vicentiu Ciorbaru wrote: > revision-id: 46b93fc2a2fa198d721813d9b93bbe09d2e36eb3 (mariadb-10.0.32-50-g46b93fc) > parent(s): dbeffabc83ed01112e09d7e782d44f044cfcb691 > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2017-10-09 13:32:40 +0300 > message: > > MDEV-13676: Field "create Procedure" is NULL, even if the the user has role which is the definer. (SHOW CREATE PROCEDURE) > > During show create procedure we ommited to check the current role, if it > is the actual definer of the procedure. In addition, we should support > indirectly granted roles to the current role. Implemented a recursive > lookup to search the tree of grants if the rolename is present. We both had to check the SQL standard to see what the behavior should be. Please add here (in the commit comment) a reference to the exact part of the standard that specifies this behavior. Like SQL Standard 2016, Part ... Section ... View I_S.ROUTINES selects ROUTINE_BODY and its WHERE clause says that the GRANTEE must be either PUBLIC, or CURRENT_USER or in the ENABLED_ROLES. > diff --git a/sql/sp_head.cc b/sql/sp_head.cc > index ea9e1c1..3dd1a65 100644 > --- a/sql/sp_head.cc > +++ b/sql/sp_head.cc > @@ -2588,10 +2588,18 @@ bool check_show_routine_access(THD *thd, sp_head *sp, bool *full_access) > *full_access= ((!check_table_access(thd, SELECT_ACL, &tables, FALSE, > 1, TRUE) && > (tables.grant.privilege & SELECT_ACL) != 0) || > + /* Check if user owns the routine. */ > (!strcmp(sp->m_definer_user.str, > thd->security_ctx->priv_user) && > !strcmp(sp->m_definer_host.str, > - thd->security_ctx->priv_host))); > + thd->security_ctx->priv_host)) || > + /* Check if current role or any of the sub-granted roles > + own the routine. */ > + (sp->m_definer_host.length == 0 && > + (!strcmp(sp->m_definer_user.str, > + thd->security_ctx->priv_role) || > + check_role_is_granted(thd->security_ctx->priv_role, NULL, > + sp->m_definer_user.str)))); Perhaps you could simplify the condition above a little bit, by replacing it with /* Check if current role or any of the sub-granted roles own the routine. */ (sp->m_definer_host.length == 0 && check_role_is_granted(thd->security_ctx->priv_user, thd->security_ctx->priv_host, sp->m_definer_user.str))); Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

[Maria-developers] MDEV-14038 ALTER TABLE does not exit on error with InnoDB + bad default function
by Alexander Barkov 11 Oct '17

11 Oct '17

Hello Marko, please review a patch for MDEV-14038. Thanks!

2 1

[Maria-developers] MDEV-13972 crash in Item_func_sec_to_time::get_date
by Alexander Barkov 09 Oct '17

09 Oct '17

Hello Sergei, Please review a patch for MDEV-13972. The patch is for 5.5, as we agreed in slack. Thanks!

2 3

Re: [Maria-developers] [Commits] 0240b866abc: MDEV-13838: Wrong result after altering a partitioned table
by Marko Mäkelä 09 Oct '17

09 Oct '17

On Mon, Oct 9, 2017 at 3:00 PM, jan <jan.lindstrom(a)mariadb.com> wrote: > revision-id: 109463fdb86d (mariadb-10.0.32-54-g0240b866abc) > parent(s): 172cc70bf8c0aea3d2d0c73bcf94f36c172b769a > author: Jan Lindström > committer: Jan Lindström > timestamp: 2017-10-09 14:52:51 +0300 > message: > > MDEV-13838: Wrong result after altering a partitioned table > > Reverted incorrect changes done on MDEV-7367 and MDEV-9469. Fixes properly > also related bugs: > > MDEV-13668: InnoDB unnecessarily rebuilds table when renaming a column and > adding index > MDEV-9469: 'Incorrect key file' on ALTER TABLE > MDEV-9548: Alter table (renaming and adding index) fails with "Incorrect > key file for table" > MDEV-10535: ALTER TABLE causes standalone/wsrep cluster crash > MDEV-13640: ALTER TABLE CHANGE and ADD INDEX on auto_increment column > fails with "Incorrect key file for table..." > This is a very welcome change that looks correct to me. I only have a few comments on the formatting and tests. The patch does not cleanly apply to 10.0 due to some difference in the comments of removed functions. Later, you informed me that this is https://github.com/MariaDB/server/commit/a8db085c51ccfd6df737d9261abac9d2b0… on bb-10.0-MDEV-13838 <https://github.com/MariaDB/server/compare/bb-10.0-MDEV-13838>. +Warnings: > +Note 1831 Duplicate index `c4_2`. This is deprecated and will be > disallowed in a future release. > The intention of the test was not to create duplicated indexes. The "rename column" and "add index" operations were split into two ALTER TABLE in order to work around the limitation that was introduced in a previous incorrect commit. You should have removed the added ALTER TABLE statements instead of adding SHOW CREATE TABLE statements. Please refer to https://github.com/MariaDB/server/commit/f56bd70f51020c67a30415ae381ae93b90… and revert the relevant parts of the test changes. The other tests seem to be adjusted fine. > @@ -171,3 +172,39 @@ ALTER TABLE ticket > SHOW CREATE TABLE ticket; > > DROP TABLE ticket; > + > +# > +# MDEV-13838: Wrong result after altering a partitioned table > +# > + > +CREATE TABLE `t` ( > +`id` bigint(20) unsigned NOT NULL auto_increment, > +`d` date NOT NULL, > +`a` bigint(20) unsigned NOT NULL, > +`b` smallint(5) unsigned DEFAULT NULL, > +PRIMARY KEY (`id`,`d`) > +) ENGINE=InnoDB DEFAULT CHARSET=latin1 COLLATE=latin1_general_cs > STATS_SAMPLE_PAGES=200 > +/*!50500 PARTITION BY RANGE COLUMNS(d) > +(PARTITION p20170913 VALUES LESS THAN ('2017-09-14') ENGINE = InnoDB, > +PARTITION p20170914 VALUES LESS THAN ('2017-09-15') ENGINE = InnoDB, > +PARTITION p20170915 VALUES LESS THAN ('2017-09-16') ENGINE = InnoDB, > +PARTITION p20170916 VALUES LESS THAN ('2017-09-17') ENGINE = InnoDB, > +PARTITION p20170917 VALUES LESS THAN ('2017-09-18') ENGINE = InnoDB, > +PARTITION p99991231 VALUES LESS THAN (MAXVALUE) ENGINE = InnoDB) */; > Please clean up the CREATE TABLE statement. Remove trailing space and unnecessary quotes and comments. > +insert into t(d,a,b) values ('2017-09-15',rand()*10000,rand()*10); > +insert into t(d,a,b) values ('2017-09-15',rand()*10000,rand()*10); > + > +replace into t(d,a,b) select '2017-09-15',rand()*10000,rand()*10 from t > t1, t t2, t t3, t t4, t t5, t t6, t t7, t t8, t t9, t t10, t t11, t t12, t > t13, t t14; > Does the table really have to be this big for the bug to repeat? Can you please reduce the test? Shouldn’t it work with 1 or 2 partitions and a few records in each? > + > +select count(*) from t where d ='2017-09-15'; > + > +ALTER TABLE t CHANGE b c smallint(5) unsigned , ADD KEY idx_d_a (d, a); > +SHOW CREATE TABLE t; > +analyze table t; > + > +select count(*) from t where d ='2017-09-15'; > +select count(*) from t force index(primary) where d ='2017-09-15'; > + > +DROP TABLE t; > + > Please do not end files in empty lines. > @@ -1625,13 +1606,9 @@ innobase_create_index_def( > index->ind_type |= DICT_FTS; > } > > - if (!new_clustered) { > - altered_table = NULL; > - } > - > for (i = 0; i < n_fields; i++) { > - innobase_create_index_field_def( > - altered_table, &key->key_part[i], > &index->fields[i], fields); > + innobase_create_index_field_def(new_clustered, > + altered_table, &key->key_part[i], > &index->fields[i]); > } > > DBUG_VOID_RETURN; > Do not add the first parameter to the same line with the function name. Instead, add it on a line on its own. In this way, it follows the indentation rules set by (defun innodb-cc-mode (&optional map) "InnoDB adjustments to cc-mode. In MAP, binds C-c C-c to 'compile." (if (and (buffer-file-name) (string-match "/\\(innobase\\|xtradb\\).*/.*[ihc]\\'" (buffer-file-name))) (progn (setq mode-name (concat "Ib" mode-name) c-basic-offset 8 c-label-minimum-indentation 0) (add-to-list 'c-offsets-alist '(c . 0)) (add-to-list 'c-offsets-alist '(arglist-intro . +)) (add-to-list 'c-offsets-alist '(label . [0])))) (if map (define-key map "\C-c\C-c" 'compile))) > @@ -2121,7 +2097,7 @@ struct ha_innobase_inplace_ctx : public > inplace_alter_handler_ctx > /** mapping of old column numbers to new ones, or NULL */ > const ulint* col_map; > /** new column names, or NULL if nothing was renamed */ > - const char** col_names; > + const char** col_names; > /** added AUTO_INCREMENT column position, or ULINT_UNDEFINED */ > const ulint add_autoinc; > /** default values of ADD COLUMN, or NULL */ > Please do not change TAB to spaces. > @@ -3067,7 +3043,7 @@ prepare_inplace_alter_table_dict( > > ctx->add_index[a] = row_merge_create_index( > ctx->trx, ctx->new_table, > - &index_defs[a], ctx->col_names); > + &index_defs[a]); > > add_key_nums[a] = index_defs[a].key_number; > Consider joining the line after new_table. Best regards, Marko

1 0