- developers - lists.mariadb.org

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

24 May '17

Hi Alexey, Thank you very much for your review! Please see my comments below: On 05/23/2017 12:16 AM, Alexey Botchkov wrote: > Firstly I'd say i don't like the idea of the 'virtual check_arguments()' > function. > I belive it's more natural to have a member 'm_check_arguments_function' > that would be set in constructor (or be an argument to the constructor) > and the non-virtual 'check_arguments()' that would just call that member. > This would make Item class structure much nicer and would even save us > memory. > > If you have good arguments for your approach, I have some > comments/questions to your patch :) I'm not sure that a function member is better. 1. It will look like a C fragment in a C++ code. Why is it more natural than a virtual method? 2. It will make the Item size bigger. 3. It will need more coding. Now we have to declare only virtual functions. With a function member, we'll have to do: - declare function anyway - add a code into constructors 4. It will need more CPU for m_check_arguments_function initialization. On the contrary, having a virtual methods costs nothing, except a small initialization in VMTs during mysqld startup. What is your main concern? Are you afraid that I'll have to modify class hierarchy for other Item_xxx_func? It should not be necessary. We'll just have a set of protected methods on the Item_func level, for most typical cases. > > Why this change? > +++ b/mysql-test/t/gis-rtree.test > @@ -61,7 +61,7 @@ while ($1) > let $2=10; > while ($2) > { > - eval DELETE FROM t2 WHERE Within(g, > Envelope(GeometryFromWKB(Point($1 * 10 - 9, $2 * 10 - 9), Point($1 * 10, > $2 * 10)))); > + eval DELETE FROM t2 WHERE Within(g, > Envelope(GeometryFromWKB(Point($1 * 10 - 9, $2 * 10 - 9), 0))); > SELECT count(*) FROM t2; > dec $2; It passed POINT() as the second argiment to GeometryFromWKB. The second argument is for SRID. After my changes it's not allowed. > } > > > It looks weird when the 'check_something' functions returns FALSE if > the argument fits the check. I'd make them returning TRUE in this case > as people normally expect. We have a convention that functions return true on error and false on success. Please have a look into: check_db_name() check_for_broken_triggers() check_unique_table() They all return true on error. I did the same. > > +bool Item_func::check_argument_types_scalar(uint start, uint end) const > +{ > + for (uint i= start; i < end; i++) > + { > + DBUG_ASSERT(i < arg_count); > > In code like this i'd rather do > bool Item_func::check_argument_types_scalar(uint start, uint end) const > { > DBUG_ASSERT(end < arg_count); > > for (uint i= start; i < end; i++) > { > // no DBUG_ASSERT here > ... Agree. > > > > Some typenames feel misleading. I'd suggest changes > Item_real_func_geometry_property -> Item_real_func_arg_geometry > Item_int_func_geometry_property -> Item_int_func_arg_geometry > Item_bool_func_geometry_property -> Item_bool_func_arg_geometry > Item_str_ascii_func_geometry_property -> Item_ascii_func_arg_geometry > Item_geometry_func_geometry_property -> Item_geometry_func_arg_geometry This looks like a property. The first argument is an object of some complex type (GEOMETRY in our case). The other arguments are simple type additional optional parameters. In C++ this would look like: geom->IsSimple(); geom->Buffer(10); OpenGIS also calls these methods "properties", so do we in the documentation: https://dev.mysql.com/doc/refman/5.7/en/gis-class-point.html > > Item_binary_func_geometry_property - do we really need this type? Right, there is only one binary string property: AsWKB(). I added Item_xxx_geometry_property for regularity (to have a symmetric class hierarchy with other "properties" ). Here's the class structure: Item_str_func Item_geometry_func Item_geometry_func_geometry_property Item_func_centroid Item_func_envelope Item_func_boundary Item_func_spatial_decomp Item_func_spatial_decomp_n Item_func_buffer Item_func_pointonsurface Item_geometry_func_importer Item_func_geometry_from_text Item_func_geometry_from_wkb Item_func_geometry_from_json Item_func_point Item_func_spatial_collection Item_func_spatial_operation Item_binary_func_geometry_property Item_func_as_wkb Item_real_func Item_real_func_geometry_property Item_func_x Item_func_y Item_func_area Item_func_glength Item_real_func_geometry_dyadic_operation Item_func_distance Item_int_func Item_int_func_geometry_property Item_func_issimple Item_func_isring Item_func_isclosed Item_func_dimension Item_func_numgeometries Item_func_numinteriorring Item_func_numpoints Item_func_srid Item_func_gis_debug Item_bool_func Item_bool_func_geometry_property Item_func_isempty Item_bool_func_geometry_dyadic_operation Item_func_spatial_relate Item_str_ascii_func Item_str_ascii_func_geometry_property Item_func_as_wkt Item_func_as_geojson Item_func_geometry_type Btw, do you need a program to print this hierarchy? :) I wrote it a few years ago, and use it almost every day. > > Item_geometry_func_importer - do we need this type? I can remove this. It does not have shared methods indeed. > > Item_real_func_geometry_dyadic_operation -> Item_real_func_arg_2_geometries > Item_bool_func_geometry_dyadic_operation -> Item_bool_func_arg_2_geometries It's an operation on two geometries. My names is quite self-descriptive. Also, it tells that: - there is no any other GEOMETRY arguments - there can be optional non-GEOMETRY additional arguments > > > + bool check_arguments() const > + { > + DBUG_ASSERT(arg_count >= 2); > + return check_argument_types_or_binary(&type_handler_geometry, 0, 2); > > the check_argument_types_or_binary() does the check of the arg_count so > no need for the DBUG_ASSERT(). > But i don't insist on removing this line :) I can remove it :) > > > On Mon, May 15, 2017 at 5:56 PM, Alexander Barkov <bar(a)mariadb.org > <mailto:bar@mariadb.org>> wrote: > > Hello Alexey, > > can you please review a patch for MDEV-12803? > > Thanks! > >

1 0

[Maria-developers] Oracle compatibility : Update with same source and target
by jerome brauge 23 May '17

23 May '17

Hello Alexander, In the same spirit of MDEV-12137, can you create a task for update with same source and target ? Regard, Jérôme.

2 3

[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